Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

verify_certificate_identity fails when subjectAltName includes otherName #324

Open
alexjfisher opened this issue Jan 3, 2025 · 3 comments · May be fixed by #325
Open

verify_certificate_identity fails when subjectAltName includes otherName #324

alexjfisher opened this issue Jan 3, 2025 · 3 comments · May be fixed by #325

Comments

@alexjfisher
Copy link

verify_certificate_identity relies on a naive string-based approach to extract DNS: (and IP:) entries from the subjectAltName extension. However, the SAN is truly an ASN.1 structure, and when the library stringifies it, otherName entries and semicolons (or other delimiters) will appear in the output. This leads to the regex split call misreading the line, causing verify_certificate_identity to fail on certificates that include otherName (common in Active Directory environments, for example).

For instance, a SAN might be stringified like:

"subjectAltName = critical, otherName:[1.3.6.1.4.1.311.20.2.3, [CONTEXT 0][email protected]];DNS:host1.example.com, DNS:example.com, DNS:MYDOMAIN"

(See

jruby-openssl/src/main/java/org/jruby/ext/openssl/X509Extension.java

Line 518 in 976a3f5

if ( other ) val.append(';'); else val.append(',').append(' ');
for where this happens?)

When split(/,\s+/) is applied to this string (see https://github.com/jruby/jruby-openssl/blob/976a3f5152b36129ad478175473bd63345286450/lib/openssl/ssl.rb#L273C9-L273C32), the returned array is

[
  "subjectAltName = critical",
  "otherName:[1.3.6.1.4.1.311.20.2.3",
  "[CONTEXT 0][email protected]];DNS:host1.example.com",
  "DNS:example.com", "DNS:MYDOMAIN"
]

The DNS:host1.example.com entry will not be found and extracted by the regex in if /\ADNS:(.*)/ (https://github.com/jruby/jruby-openssl/blob/976a3f5152b36129ad478175473bd63345286450/lib/openssl/ssl.rb#L277C11-L277C26)
@alexjfisher
Copy link
Author

@kares In f18c794 it looks like you might have tested the MRI openssl implementation and decided the custom approach was still needed???

There does seem to be an issue using the version from https://github.com/ruby/openssl as-is. Specifically, san.value (https://github.com/ruby/openssl/blob/e5153dbbb4baa568b48bf2caf9af9b79dbfe2f1b/lib/openssl/ssl.rb#L323C54-L323C63) doesn't return a raw string in jruby-openssl compared to ruby-openssl, but a single element Array of OpenSSL::ASN1::OctetString instead??

@alexjfisher
Copy link
Author

I've had a go at adding a test and fixing this issue. I'm not confident in my approach though. PR to follow shortly.

@alexjfisher alexjfisher linked a pull request Jan 3, 2025 that will close this issue
Draft
@kares
Copy link
Member

kares commented Jan 6, 2025

and decided the custom approach was still needed???

yes, unfortunately the ASN.1 parsing isn't 100% complete and there are differences in how the ext.value is presented. there were changes (fixes) to ASN.1 since but still brittle IMO.

changes (e.g. "Same here. san.value is changed to san.value.last.value") in your PR is risky to be put in. those changes would require more tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Support more complex SANs in verify_certificate_identity alexjfisher/jruby-openssl
2 participants
@kares @alexjfisher