-
Notifications
You must be signed in to change notification settings - Fork 81
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.
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
Replace incoming certs with newer versions from trust store. #149
Conversation
The logic here is derived from pieces of logic in OpenSSL's x509_vfy.c build_chain function, specifically this logic removing remaining untrusted certs and this logic replacing a wire cert with one from the trust store. |
Also relevant, the discussion about why this change was made in OpenSSL: http://openssl.6102.n7.nabble.com/openssl-dev-openssl-org-2634-Cross-signed-certs-rejected-by-OpenSSL-because-root-cert-not-base-of-chn-td56649.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💚 for digging deep (into OpenSSL code)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uncertain I am.
if (ok == X509Utils.X509_LU_X509) { | ||
// replace old with new and clear rest of untrusted | ||
Certificate certificate = (Certificate) objTmp[0]; | ||
if (certificate.x509.equals(skCert)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sure I am missing bits and pieces. the question for me is what cert-chain do I need to send to get it trusted. so I need the skCert
to match the subject-principal of a trusted certificate and then I need to satisfy this equals method.
from the supplied threads I figured that we basically want to replace self-signed certificates in the middle of the chain by some trusted certificate from local trust-store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkristian Yes, that's exactly it. I'm working on getting the right things to compare to allow an untrusted cert from the wire to properly match a trusted cert in the local store. Need to read more of the OpenSSL code to understand exactly how much needs to be the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@headius I was pounding a bit more on this. maybe this is just OK as is when there new trusted certificate chain gets verified after this. after all that is what we want to achieve: a trusted verified chain from the server certificate to root certificate where root comes from our root certificate store.
@headius can I do anything to help move this along? => If you have any concrete steps in mind for resolving this situation, I'm happy to offer my time for that :) |
@original-brownbear this was an attempt to resolve a (system) CA cert issue. |
@kares I was able to reproduce the failure with Oracle Maybe we should try upping Travis to |
@original-brownbear thanks, still Java 8 is the primary target. recall trying latest at some point wout luck. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I never replied to this because I never understood how we knew we could trust the change in certificates. I don't have confidence I can say this is right or wrong. Is what we get by getBySubject guaranteed to be trusted?
Anyone checked with a more recent Java 8? If it got fixed in 9.0.4 it may have been backported too. |
Hi, this is still happening in alpine as of version:
|
Is this PR useful for the current issues around the expired Let's Encrypt cert? #236 |
@ThomasKoppensteiner It may be but the reason it was never merged is because I was not confident that it was the correct (i.e. safe and secure) change to make. @kares Ping... this is the change I mentioned from back when I was trying to fix this revoked cert issue. Perhaps it helps? |
yeah, I recalled and revisited this. |
as seen with #236 this logic isn't enough (also a similar 'minimal' attempt at #240 wasn't sufficient) -> with jruby-openssl >= 0.11.0 this issue is expected to be resolved due porting of the alt-chain verification logic from OpenSSL 1.1.1 |
Fixes jruby/jruby#4802
This tries to emulate what OpenSSL does in X509_vfy.c when building the untrusted certificate chain, replacing incoming certs with newer versions from the local store.