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.

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

Closed
wants to merge 2 commits into from

Conversation

headius
Copy link
Member

@headius headius commented Nov 2, 2017

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.

@headius
Copy link
Member Author

headius commented Nov 2, 2017

@headius
Copy link
Member Author

headius commented Nov 2, 2017

Tagging a couple other folks that might be able to help: @nahi @tarcieri

@headius
Copy link
Member Author

headius commented Nov 2, 2017

Copy link
Member

@kares kares left a 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)

Copy link
Member

@mkristian mkristian left a 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)) {
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

@original-brownbear
Copy link

@headius can I do anything to help move this along?
The lack of stable Java9 support is becoming a big issue for Logstash and I'm happy to help in any way I can to move this forward :)
The patch here makes sense to me in an of itself.
But for Java9 in general, it looks like there's also a build issue with the Maven GEM wagon (that depends on JRuby) not working on Java9 before this is merged + released, making a proper Java9 test build impossible?

=> If you have any concrete steps in mind for resolving this situation, I'm happy to offer my time for that :)

@kares
Copy link
Member

kares commented Feb 12, 2018

@original-brownbear this was an attempt to resolve a (system) CA cert issue.
at the time of the patch it didn't simply reproduce reliably (maybe related to ca.cert update on nix systems), but now there are test failures and those occur with or without this patch.
we can work-around the wagon build issue eventually, so I wouldn't worry about that much as release is blocked by that cert verification failure - not sure if it makes sense to release jossl as is (likely not).

@original-brownbear
Copy link

original-brownbear commented Feb 12, 2018

@kares I was able to reproduce the failure with Oracle build 9.0.4+11 but was able to reproduce this issue with 9.0.1. It seems there have been a bunch of fixes the the cert management in 9.0.4+ that seem related http://www.oracle.com/technetwork/java/javase/9-0-4-relnotes-4021191.html.

Maybe we should try upping Travis to 9.0.4 and see how that goes build wise?

@kares
Copy link
Member

kares commented Feb 12, 2018

@original-brownbear thanks, still Java 8 is the primary target. recall trying latest at some point wout luck.
not sure if smt changed since - maybe worth a re-check.
if its resolved (test 💚) we need to find out which version and issue a warning for users running 'old' 8.

Copy link
Member

@enebo enebo left a 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?

@headius
Copy link
Member Author

headius commented Feb 12, 2018

Anyone checked with a more recent Java 8? If it got fixed in 9.0.4 it may have been backported too.

@albertogg
Copy link

albertogg commented Feb 20, 2018

Hi, this is still happening in alpine as of version:

jruby 9.1.15.0 (2.3.3) 2017-12-07 929fde8 OpenJDK 64-Bit Server VM 25.151-b12 on 1.8.0_151-b12 +jit [linux-x86_64]

@ThomasKoppensteiner
Copy link

Is this PR useful for the current issues around the expired Let's Encrypt cert? #236

@headius
Copy link
Member Author

headius commented Oct 11, 2021

@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?

@kares
Copy link
Member

kares commented Oct 12, 2021

yeah, I recalled and revisited this.
but the change presented here won't cut it, at least not for the "Let's Encrypt" case (already verified that).
anyhow, I was on the path of porting the whole OpenSSL 1.1 logic but than got into revisiting the 1.0 logic (under the assumption that having X509_V_FLAG_TRUSTED_FIRST would be sufficient - it isn't).
so I am back looking at build_chain, which also motivated this PR, while also trying to minimize the changeset to be able to push a fix in a patch release.

@kares
Copy link
Member

kares commented Oct 25, 2021

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

@kares kares closed this Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On ubuntu 16.04, "certificate verify failed" errors for www.google.com
7 participants