Repository navigation
Add CLI flag --ip-address - #10495
Conversation
|
ohemorange is gonna take the lead on the first review here, but i do want to flag a concern i have with this PR that i initially put at jsha#2 (comment). this PR works by passing IP addresses as the domain parameter of get_chall_pref in the plugin API. i personally don't think we should do this. i think we should fix this before we offer this feature as i think otherwise we'll be tempted to just ignore the problem because the code "works" to try and offer a solution instead of just pointing out a problem tho, i have some ideas on how we can solve this. first, i do not think we should make a breaking change to our plugin API if we can at all help it. this is essentially the only part of certbot's API that essentially everyone who relies on it uses (we have way more 3rd party authenticators than anything else) and i think any migration would be quite painful i think the idea i like best would be to create a new interface named something like the other idea i had is to just change the name of the parameter in with all that said, if jsha and ohemorange don't think this issue needs to be addressed first, i'm open to arguments and otherwise won't bring it up again unless i'm assigned as the 2nd reviewer here, but i did want to drop a comment sharing my concerns and ideas that y'all can do what you want with |
I'm inclined to go this way. I agree it's a bit hacky, but the other way is a lot of complexity for something that's not that likely to be useful. Skimming through our first-party authenticators, it looks like they all return an unconditional list for |
|
this approach works for me! it does mean using strings containing either a domain or an IP which is less helpful and doesn't match the type of the attribute we recently added to achalls, but i personally think that's a fine tradeoff for maintaining backwards compatibility here. i agree that for all/most 3rd party plugins, this parameter to get_chall_pref doesn't really matter |
|
Just popping in to see if there's an ETA on when this may land in a release. I'd recently addeed WebTransport support to Emscripten in order to update https;//quakejs.com, which previously relied on WebSockets. The master server protocol (which servers use to notify it of their presence) uses IP address/ports rather than host names, so I need to generate SSL certs for their IP address in order to correctly then let clients connect to them via HTTP/3. Right now I've got a hard-coded table of IP addresses -> hosts, but am very excited to dump that in favor of this soon so others can start running servers. |
ohemorange
left a comment
ohemorange
left a comment
There was a problem hiding this comment.
Added a bunch of nits using new-style type indicators, along with some minor suggestions and questions. Only other other is, did you decide to just go with ipv4 only tests and not bother with ipv6?
Yeah I couldn't figure out a way to make IPv6 tests work in the xdist world since there's exactly one loopback address, so I can't pull the same trick I pulled for ipv4 tests. Open to ideas! Also if you spot other test cases we should have I could add those. |
|
i'm not sure if it's worth it, but i shared some thoughts about how we could potentially add ipv6 tests at https://opensource.eff.org/eff-open-source/pl/yip79htid7n3ppski7msnx3nby which i didn't see a response to at the time |
ohemorange
left a comment
ohemorange
left a comment
There was a problem hiding this comment.
Other than potentially adding ipv6 tests in the way that bmw mentioned, this looks good to me
|
I added an IPv6 test: https://github.com/certbot/certbot/pull/10495/changes#diff-f2bc80fb3447503854944506b2737dd6db9402c2f3991d0a5f6b836b0f6872f7R126-R144 I also restored the changes I previously made to proxy.py, since they were again very useful in debugging the IPv6 test. I remembered one important thing about the old code: in the case that a request comes in with no matching host in the mapping, the proxy would crash! and the error would get buried. In the restored version I tweaked a little further: when there's no backend, the proxy now returns 200 instead of 500. That way Pebble will pass through the text of the response body, which is helpful in debugging. |
| This test relies on some tricks. While proxy.py handles _most_ of the HTTP traffic | ||
| for integration test validations, it only binds port 5002 for IPv4 addresses. By | ||
| specifying `::1` on the commandline, we can get `certbot certonly --standalone` to | ||
| bind 5002 only for IPv6 localhost, thus not conflicting with proxy.py. That means | ||
| for this test, Pebble connects directly to the standalone server, not via proxy.py. |
There was a problem hiding this comment.
I don't quite understand this, could you explain more? Or point out where the code is that does this? Does "By specifying ::1 on the commandline" mean in --ip-address or in --http-01-address?
There was a problem hiding this comment.
I wound up rewriting this a few times. Here's the latest (also in the diff, but pasting here for convenience):
"""Test the HTTP-01 challenge with an IPv6 address using standalone authenticator.
This test relies on some tricks. Pebble is configured to do validations on port 5002.
Since multiple integration tests want to handle validation requests, and may run
concurrently, proxy.py handles HTTP traffic for port 5002 and sends it to the appropriate
integration test runner. However, it binds that port for IPv4 only. That is,
GracefulTCPServer doesn't specify `address_family = AF_INET6` when subclassing
socketserver.TCPServer.
For IPv4 integration tests (above), we simply assign each integration test runner a unique
IP address under 127.0.0.0/8, and the proxy knows how to route those IP addresses. Pebble's
validation connects to the proxy because all of 127.0.0.0/8 is defined to be loopback.
However, under IPv4 there is exactly one loopback address, so we can't use the same trick.
Instead, we ensure that this is the only test that cares about IPv6, and bind [::1]:5002
for IPv6 (via `--http-01-address`). When Pebble reaches out to validate `::1`, it reaches
this test runner rather than the proxy.
There was a problem hiding this comment.
This is much more helpful, thank you! This explanation works for me.
bmw
left a comment
bmw
left a comment
There was a problem hiding this comment.
in addition to my inline comments, as a note to both you and myself, i have similar questions to ohemorange about the changes to proxy.py that i think should be addressed
| checker = ocsp.RevocationChecker() | ||
|
|
||
| config_sans = set(config.domains) | ||
| config_sans = set(config.domains + config.ip_addresses) |
There was a problem hiding this comment.
related to the change here, i think we should update this code to also include "matching" in certbot's output if config.ip_addresses is set
certbot/certbot/src/certbot/_internal/cert_manager.py
Lines 361 to 362 in d01a6db
this matches what we do with --domain and i worry that without it, it's not clear that --ip-address was used to filter certbot certificates output
There was a problem hiding this comment.
Done. I restructured this a bit; with config.ip_addresses in there it hit a line length limit, and I figured it would be a little clearer as an if/else anyhow (and fit inside the line length limit handily).
| case Sequence(): | ||
| for v in values: | ||
| # This will throw an exception if the IP address doesn't parse. | ||
| namespace.ip_addresses.append(san.IPAddress(str(v))) |
There was a problem hiding this comment.
at least to me, argparse's code and documentation is pretty unintelligible, but i unfortunately don't think argparse will ever give us this kind of sequence. see how DomainsAction handles this above by just converting the argument to a string
i think we should do something similar here unless you've learned something about argparse that i have not
There was a problem hiding this comment.
This bugged me so I tracked it down with some printf debugging and some advice from a bot.
N arguments from the command line will be gathered together into a list.
If the nargs keyword argument is not provided, the number of arguments consumed is determined by the action. Generally this means a single command-line argument will be consumed and a single item (not a list) will be produced.
Actions that do not consume command-line arguments (e.g. 'store_const') set nargs=0.
So those cover the Sequence and None cases. Given we don't set nargs, we will never get Sequence or None here. It bugged1 me that the code was written as if we might receive a Sequence or a None. So I updated both DomainsAction and _IPAddressAction to only handle the single-item case, and specifically type-check and reject other cases.
Footnotes
-
confused ↩
| :raises TypeError: if sans includes IP addresses. | ||
|
|
There was a problem hiding this comment.
i think this may be a leftover from previous work? is a TypeError really raised here in this case?
my memory is we handled this kind of thing in main.py so it didn't need to be handled here
There was a problem hiding this comment.
yep, reverted this diff.
| context.certbot([ | ||
| 'certonly', '--ip-address', context.get_local_ip(), '--standalone', | ||
| ]) | ||
|
|
There was a problem hiding this comment.
nit: can we also add a simple assertion here and in the ipv6 test below? maybe something like:
| assert_cert_count_for_lineage(context.config_dir, context.local_ip, 1) |
| context.certbot([ | ||
| 'certonly', '--ip-address', '::1', '--standalone', | ||
| '--http-01-address', '::1', | ||
| '--http-01-port', '5002', |
There was a problem hiding this comment.
both here and in the comments above, can you use DEFAULT_HTTP_01_PORT from certbot-ci/src/certbot_integration_tests/utils/constants.py all/most of the time? i wasn't immediately sure where this port number was coming from
There was a problem hiding this comment.
Done in the code. For the comment, I think concrete is easier to understand than abstract, and this value hasn't changed in the history of Certbot. I used the specific port number, with some extra verbiage to explain its role; and I think the code using constants.DEFAULT_HTTP_01_PORT helps make it clear too.
|
|
||
| @abstractmethod | ||
| def get_chall_pref(self, domain: str) -> Iterable[type[Challenge]]: | ||
| def get_chall_pref(self, identifier: str) -> Iterable[type[Challenge]]: |
There was a problem hiding this comment.
i'm not sure it really matters but just in case...
how about adding a comment at
stating that the name of the parameter to get_chall_pref has changed over time so it's important the value is always passed as a positional argument instead of a keyword argument?
| macOS's file manager Finder by running something like: | ||
| 4. Configure additional loopback addresses (these are used by IPv4 integration tests): | ||
| `for n in $(seq 2 127) ; do sudo ifconfig lo0 alias "127.0.0.${n}" up ; done`. Note this | ||
| will have to be re-done after boot. You may want to add it as a launch daemon. |
There was a problem hiding this comment.
I would personally love to have instructions or included files to do that easily, but this is probably sufficient for this particular PR.
There was a problem hiding this comment.
maaannnn...this and the conclusion of #10495 (comment) sure is unfortunate imo. i think every additional manual step needed to set up a certbot development environment makes getting started working on the project just a little more grating and i think this one requiring a script that needs to be run as root on every reboot is a particularly gnarly one
this is the only ipv4 test we have and we have no immediate plans to add any more. even if we do add more in the future, i think it wouldn't be a big deal if we had a few ipv4 tests that have to run in sequence on the same worker
because of this, i think my ideal here would be for us to architect a different solution here. i haven't had/taken the time to experiment with this, but my first thought would be to somehow coordinate with pytest, the proxy, and/or the worker that gets the ipv4 test which port requests for the HTTP host header value of 127.0.0.1 should be forwarded to. this worker would be the only one that can run ipv4 tests and the logic of mapping a different IP address to each worker that is currently in this PR would be removed
jsha, if you'd be interested in implementing this or something like it, i think it'd be my personal preference, however, i also get the feeling you and others are itching for this PR to land and what you have here mostly works. assuming i'm correct that you don't especially want to do that work, what if instead of this contributing.rst change we:
- use code like
@pytest.mark.skipif(sys.platform == 'darwin', reason='macOS has one IPv4 loopback address by default')to skip this test on macOS - move text like what you added here to the documentation of the function so in the future macOS devs can run this test locally if they need/want to
what do you think? i'd recommend jsha, @ohemorange, and i all get the same page here before doing any more work here to avoid wasted effort
There was a problem hiding this comment.
Agreed that it should be nice and simple to get started working on the project. I like the idea of a refactoring to the tests - what would you think of eliminating proxy.py entirely and instead running multiple instances of pebble configured to talk to the different test runners directly? Proxy.py adds significant code complexity and also makes the tests less realistic than they otherwise would be. For instance, if there was an HTTP-level weirdness in Python's standalone server, but it was papered over by the proxy, Pebble would never see it.
At any rate, I appreciate the offer to just skip this test on macOS for now. I think that makes sense, particularly given we don't yet have macOS integration tests running on CI.
There was a problem hiding this comment.
oh that's an interesting idea! i think historically that approach wasn't really feasible here as this test harness would run either boulder or pebble depending on how it's configured and i think boulder is too heavyweight for that, but we have since dropped testing with boulder. if we think pebble is lightweight enough that an average CI runner or dev laptop could reasonably handle running multiple instances, i think that would definitely be worth exploring
the only other concern that initially comes to mind is how to handle the OCSP servers. i assume every pebble instance would need their own OCSP server? are things still fine there from a performance/resource perspective? alternatively we could maybe look into dropping OCSP support, but if we did that i think we'd want to make damn sure certbot shouldn't continue supporting it for non-Let's Encrypt CAs. i'm assuming that every pebble instance could share the same DNS server without issues
do you have the time and interest to look into this as a follow up PR? i'll be honest and say i personally don't think this work has to be done now, but i certainly would appreciate it being done if you're at all inclined to do it
There was a problem hiding this comment.
Pebble is pretty lightweight and I think that would work.
how to handle the OCSP servers.
We could do one OCSP server per Pebble. But this could also have a rethink. The integration tests that rely on the OCSP server are often checking one of two things:
- Does Certbot correctly perform the ACME dance for revocation?
- Does Certbot correctly request and parse OCSP responses?
For instance test_revoke_ecdsa_key is testing (1) (though it incidentally tests (2) since it calls certbot certificates to check status). test_ocsp_status_live is testing (2).
For things that test (1), it would suffice to query Pebble's management interface directly. Or even just checking for a 200. For things that test (2) there's no need to talk to Pebble. We can have Certbot's CI-internal OCSP server return exactly the response we want to test.
I'd be maybe interested in ripping out proxy.py since it's been a very frustrating piece of infrastructure to work with, but IMO the more important thing right now is to get webroot and other authentication methods working with IP addresses. Standalone is the worst of our authentication plugins because you have to bring down your server to do it. It would be a mixed message to have that be the only supported means of IP address authentication.
There was a problem hiding this comment.
Anyway, I think it's fine to just skip this test here for now, and agree that fixing up the integration tests to run on mac (which we don't do on ci anyway) is not the most important next thing to do.
There was a problem hiding this comment.
I also think the engineering needed to get this test working on mac as-is (as something that you can setup once and never again) is less than restructuring the whole thing would take. Whether it's the most correct thing to do is a separate question, and may also be valuable.
There was a problem hiding this comment.
the pebble startup time is a significant amount of the test time here
FYI locally it takes 2 seconds; but also there's a minimum 1 second sleep in check_until_timeout. And this is probably parallelizable (i.e. each runner waits independently for its own pebble).
[2026-01-16 11:11:22] => Starting pebble instance deployment...
[2026-01-16 11:11:22] => Waiting for pebble instance to respond...
[2026-01-16 11:11:24] => Finished pebble instance deployment.
There was a problem hiding this comment.
....I bet it's that 1 second sleep that made me think it was slow. A parallelizable 1 second is fine, obviously.
There was a problem hiding this comment.
thanks a lot for the thoughts
i agree that doing this isn't especially high priority, so i just created #10540 to link to this convo from the proxy code so we can maybe consider this in the future when/if we ever go poking around in there again
|
also, just to make sure we're all on the same page, i want to flag that on the certbot call today there was some interest in us actually testing ipv6 cert names on WSL. @zoracon, said she'd be reaching out to you about this jsha since she has a WSL setup we can use, but i think you can also reach out to her first if she hasn't already |
There was a problem hiding this comment.
other than my minor inline comment, the only things i think need to be done here are:
- have my code comment at #10495 (comment) addressed. it was marked as resolved without the change being made? maybe you have commits locally that haven't been pushed up yet?
- ohemorange/zoracon is satisfied with any windows ipv6 cert name testing. i personally think it'd be nice to do this testing before merging/releasing this feature, but i also don't feel the need to block on it. i'm not sure their preferences here
otherwise, this LGTM! thanks for seeing this major feature thru to completion jsha
Co-authored-by: Brad Warren <[email protected]>
Co-authored-by: Brad Warren <[email protected]>
Ah, hm, I thought I pressed the "commit suggestion" button and then "resolve." It looks like only one of those took effect. Thanks for spotting. |
Co-authored-by: Brad Warren <[email protected]>
ohemorange
left a comment
ohemorange
left a comment
There was a problem hiding this comment.
I don't think we need to block on WSL testing, but we should definitely block the release on having done so. Otherwise, lgtm!
this PR is a follow up to my comment at #10495 (comment) i dislike that jsha chose a slightly different column limit than towncrier which caused it to generate awkwardly short lines in our changelog. i also dislike contributors having to remember/know about towncrier's wrapping behavior when writing changelog entries. i'd like us to find a way to not have to worry about this i initially looked into trying to set towncrier's line length limit to a much higher value, but after searching around https://towncrier.readthedocs.io/en/stable/configuration.html, i don't think it's configurable instead, i'm proposing here that we remove the wrapping entirely. wrapping was added in response to the discussion at #10379 (comment). every markdown viewer i'm aware of automatically nicely handles wrapping of long lines in markdown. most also automatically merge manually wrapped lines to wrap based on the user's display/settings. finally, since the changelog is automatically edited, i think it'll be rare for certbot devs to have to manually edit the file because of all this, i think whether the newsfragment was manually wrapped or not is largely irrelevant, we shouldn't worry about it, and we should let people wrap or their newsfragments or not as they see fit i suppose alternatively we could just let towncrier double/awkwardly wrap entries since i'm making the case that the wrapping doesn't matter, but i personally have a slight preference for this approach. let me know what y'all think
bmw
left a comment
bmw
left a comment
There was a problem hiding this comment.
in that case, let's merge this
thanks again jsha!
i don't think the conversation at #10495 (comment) is urgent/important enough to make a github issue for it, but i also feel like it's worth keeping a link around in case any devs have problems with this code in the future. i think there was some good ideas in there i don't think this PR requires two reviews
Update integration test framework so that each worker gets a dedicated localhost IP address to listen on, and requests certificates for that IP address.