Correctly verify abbreviated IPv6 SANs #185
Conversation
|
Hello Maintainers, We are a heavy user of the Ruby OpenSSL, and we generate some of our certs using IPv6 SANs, and we would find it convenient if the openssl library accepted all RFC-compliant addresses, hence this PR. And, as always, thank you for your excellent work. |
|
@rhenium Could you please review this PR when you have time? Thank you very much. |
|
To reassure you that other OpenSSL packages convert the certificate's SAN's IP bytes to IP address-type (not string or byte array) and convert the hostname to an IP address-type, too: Golang's library https://golang.org/src/crypto/x509/verify.go line 78 converts the hostname to an IP address-type before verification, and the Certificate stores the SANs IPs as IP address-types: https://golang.org/pkg/crypto/x509/#Certificate: |
lib/openssl/ssl.rb
Outdated
| begin | ||
| return true if IPAddr.new(san.value.unpack('n*').map {|e| sprintf("%X", e)}.join(':')) == IPAddr.new(hostname) | ||
| rescue IPAddr::InvalidAddressError | ||
| end | ||
| end |
There was a problem hiding this comment.
I guess this can be simplified to return true if san.value == IPAddr.new(hostname).hton.
There was a problem hiding this comment.
Much nicer, and shorter. I have updated my PR.
Note: I could not remove the begin ... rescue guard: I tried, and I got the the following failure:
Error: test_verify_certificate_identity(OpenSSL::TestSSL): IPAddr::InvalidAddressError: invalid address
...
408 tests, 3850 assertions, 0 failures, 1 errors, 5 pendings, 0 omissions, 0 notifications
98.5294% passed
|
The ipaddr library is a default gem in Ruby 2.5, so it needs to be added to openssl.gemspec as a runtime dependency. |
|
Hi Kazuki, Thank you for taking the time to review my PR and suggest improvements. I have implemented the changes you suggested. Let me know if there's anything else I should do or change. —Brian |
lib/openssl/ssl.rb
Outdated
| begin | ||
| return true if san.value == IPAddr.new(hostname).hton | ||
| rescue IPAddr::InvalidAddressError | ||
| end |
There was a problem hiding this comment.
This works for both IPv4 and v6 addresses. And can you remove the misleading comment "follows GENERAL_NAME_print() [...]" while you are at it?
There was a problem hiding this comment.
Code is applied to both IPv4 and IPv6 SANs.
Misleading comment is removed.
openssl.gemspec
Outdated
| @@ -17,6 +17,7 @@ Gem::Specification.new do |spec| | |||
|
|
|||
| spec.required_ruby_version = ">= 2.3.0" | |||
|
|
|||
| spec.add_runtime_dependency 'ipaddr' | |||
There was a problem hiding this comment.
Use double quotes. The style is not consistent throughout the code base sadly, but the file is using double quotes for all other string literals.
There was a problem hiding this comment.
Fixed. Sorry about that — I should have paid more attention before I cut-and-pasted.
IPv6 SAN-verification accommodates ["zero-compression"](https://tools.ietf.org/html/rfc5952#section-2.2). It also accommodates non-compressed addresses. Previously the verification of IPv6 addresses would fail unless the address syntax matched a specific format (no zero-compression, no leading zeroes). As an example, the IPv6 loopback address, if represented as `::1`, would not verify. Nor would it verify if represented as `0000:0000:0000:0000:0000:0000:0000:0001`; however, both representations are valid, RFC-compliant representations. The library would only accept a very specific representation (i.e. `0:0:0:0:0:0:0:1`). This commit addresses that shortcoming, and ensures that any valid IPv6 representation will correctly verify.
|
Hi Kazuki, Let me know if there's anything else you'd like me to change. For example, let me know if you'd like me to remove the surrounding Thanks, —Brian |
…tificate verification This is a temporary patch until this [PR](ruby/openssl#185) is merged. Ruby OpenSSL's verification for IPv6 is flawed in that in only accepts a very specific format (e.g. "0:0:0:0:0:0:0:1", but not "::1"). This causes SSL verification to mistakenly fail. Signed-off-by: Brian Cunnie <bcunnie@pivotal.io>
|
It looks good and is merged now. Thank you! |
|
Thank you, @rhenium . I appreciate the time & effort you spent coaching me through the pull request process, and I realize you could have made the changes yourself in the fraction of the time you spent with me. I am grateful. |
…tificate verification This is a temporary patch until this [PR](ruby/openssl#185) is merged. Ruby OpenSSL's verification for IPv6 is flawed in that in only accepts a very specific format (e.g. "0:0:0:0:0:0:0:1", but not "::1"). This causes SSL verification to mistakenly fail. Signed-off-by: Brian Cunnie <bcunnie@pivotal.io>
…tificate verification This is a temporary patch until this [PR](ruby/openssl#185) is merged. Ruby OpenSSL's verification for IPv6 is flawed in that in only accepts a very specific format (e.g. "0:0:0:0:0:0:0:1", but not "::1"). This causes SSL verification to mistakenly fail. Signed-off-by: Brian Cunnie <bcunnie@pivotal.io>
IPv6 SAN-verification accommodates "zero-compression". It also accommodates non-compressed addresses.
Previously the verification of IPv6 addresses would fail unless the address syntax matched a specific format (no zero-compression, no leading zeroes).
As an example, the IPv6 loopback address, if represented as
::1, would not verify. Nor would it verify if represented as0000:0000:0000:0000:0000:0000:0000:0001; however, both representations are valid, RFC-compliant representations. The library would only accept a very specific representation (i.e.0:0:0:0:0:0:0:1).This commit addresses that shortcoming, and ensures that any valid IPv6 representation will correctly verify.