Make SSL_set1_host and SSL_add1_host handle IP addresses #9201
Conversation
|
The alternative, of course, is to let I didn't do that that because I was focused on the internal implementation details and instead added But that's the mistake we always make — 'designing' an API based on inward-looking implementation details rather than considering how the user will get it wrong. So now I'm wondering if that was the wrong choice. Especially since the So it might make sense to just make |
|
Ping? |
|
Sorry that this took so long even looking at. There's a lot going on right now... |
|
I can provide that in a follow-up commit. Should It has the advantage that it can theoretically be backported to older release streams to fix this behaviour there, because it doesn't rely on adding the separate new function. (Not that there are DSO compatibility reasons for the policy of not adding symbols in a stable release stream, but that's a different discussion.) |
|
Having the |
|
Will do. They should handle the Do we even want to add |
If I would also suggest to rename |
|
I was going to ditch that too, potentially leaving the PR entirely applicable to older stable releases. But I can keep |
I'm not deep enough into the details, so I'll let @levitte or others decide that. |
|
If |
|
IP address only is achieved by passing only (a textual representation of) an IP address to |
|
The key word was "enforce", i.e. if there's a reason to call a function that only accepts IP addresses. I can't think of a reason to do so, but I might simply not know. |
|
Ah, right. Yeah, I can't see a reason to do that either. Making X509_VERIFY_PARAM transparently accept IP addresses anywhere it accepts hostnames is slightly non-trivial as there can be only one So |
|
OK, how's this? It's slightly asymmetric because But the I do still need to patch the sconnect demo because a simple "truncate at the first colon" doesn't work for IPv6 literals; it should be using In the general case, something will always need to have extracted the IPv6 literal from within the |
|
While playing around with the sconnect demo, I noticed that the compiler complains about the deprecated function SSL_CTX_load_verify_locations(), see issue #10392. |
|
@dwmw2 unfortunately there is not much documentation about the sconnect demo. A (very) short glance at the source code gave me the impression that I could run
in one console, after which netstat would show a listening socket at port 4433:
However, when I ran sconnect in another console, I got the following error message.
What am I missing? Could you please give me a quick introduction how to use the demo? |
|
Ok, the previous failure seems to be an IPv6 problem. Apparently the (Note: both tcpdump captures below are attached as sconnect.zip.) msp@msppc:~$ tcpdump -r sconnect-01.pcap -vv
If I use my hostname instead, I get a handshake failure instead:
The server side terminates with the following error,
and port 4433 remains in
The tcpdump output is not very helpful... msp@msppc:~$ tcpdump -r sconnect-02.pcap -vv
...but luckily wireshark has a little brother for the console: msp@msppc:~$ tshark -r sconnect-02.pcap -V -O tls
BTW: tests were done on the master branch first (commit 26b7cc0). |
|
Automated Ping: This issue is in a state where it requires action by @openssl/committers but the last update was 109 days ago |
While it is true that we support multiple hostnames, but only one IP address, this is not an issue for the The reason for multiple hostnames is partly DANE, partly prior art in Postfix where we can match either the domain or the MX host, ... Most applications only ever set one name, and send that as the SNI. So the case for multiple IPs (given no prior practice of accepting more than one at a time) is pretty weak. So I would not see lack of support for the |
ssl/ssl_lib.c
Outdated
Show resolved
Hide resolved
|
Looks better now. Is there anywhere in "apps", where we need to take care to not also use such hostnames as the implicit SNI, or is that already handled? |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago |
|
This should be ready to go, but needs a rebase to catch up with CHANGES.md that happened in the meantime. When pushing, please rebase CHANGES.md first. |
|
This pull request is ready to merge |
There is a slight mismatch here because X509_VERIFY_PARAM copes only with a single IP address, and doesn't let it be cleared once it's set. But this fixes up the major use case, making things easier for users to get it right. The sconnect demo now works for Legacy IP literals; for IPv6 it needs to fix up the way it tries to split the host:port string, which will happen in a subsequent patch.
Instead of naïvely trying to truncate at the first colon, use BIO_get_conn_hostname(). That handles IPv6 literals correctly, even stripping the [] from around them.
The X509_VERIFY_PARAM can only take a single IP address, although it can have multiple hostnames. When SSL_add1_host() is given an IP address, don't accept it if there is already one configured.
|
Did the rebase to resolve the conflict in CHANGES.md and force-pushed the result. (This action does not require resetting the grace period IMHO.) Since I didn't review this pull request, I'll leave merging to @t8m or @vdukhovni. |
There is a slight mismatch here because X509_VERIFY_PARAM copes only with a single IP address, and doesn't let it be cleared once it's set. But this fixes up the major use case, making things easier for users to get it right. The sconnect demo now works for Legacy IP literals; for IPv6 it needs to fix up the way it tries to split the host:port string, which will happen in a subsequent patch. Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #9201)
Instead of naïvely trying to truncate at the first colon, use BIO_get_conn_hostname(). That handles IPv6 literals correctly, even stripping the [] from around them. Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #9201)
The X509_VERIFY_PARAM can only take a single IP address, although it can have multiple hostnames. When SSL_add1_host() is given an IP address, don't accept it if there is already one configured. Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #9201)
…terals Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #9201)
|
Merged to master. Thank you for the PR and for the patience. |
There is a slight mismatch here because X509_VERIFY_PARAM copes only with a single IP address, and doesn't let it be cleared once it's set. But this fixes up the major use case, making things easier for users to get it right. The sconnect demo now works for Legacy IP literals; for IPv6 it needs to fix up the way it tries to split the host:port string, which will happen in a subsequent patch. Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#9201)
Instead of naïvely trying to truncate at the first colon, use BIO_get_conn_hostname(). That handles IPv6 literals correctly, even stripping the [] from around them. Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#9201)
The X509_VERIFY_PARAM can only take a single IP address, although it can have multiple hostnames. When SSL_add1_host() is given an IP address, don't accept it if there is already one configured. Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#9201)
…terals Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#9201)
We have SSL_set1_hostname() already. It would be reasonable for developers to assume that it can be used for IP literals. It can't. Add the functions that can.
Then use them to fix the sconnect demo, which was failing for IP literals.
Checklist