Tls cert race/core 9178/v25.1.x #175
Merged
+89
−49
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
michael-redpanda
requested review from
pgellert
and removed request for
a team
michael-redpanda
force-pushed
the
tls-cert-race/core-9178/v25.1.x
branch
from
913465f to
96c1345
Compare
|
CI failures in two categories:
|
Should be fixed by #176 |
|
By the end of our three PRs we may get one green run ;) |
michael-redpanda
force-pushed
the
tls-cert-race/core-9178/v25.1.x
branch
from
3d00bdc to
96b4655
Compare
|
CI Failures:
|
BenPope
approved these changes
Feb 24, 2025
Comment on lines
+1108
to
+1116
| auto error_codes = get_all_ossl_errors(); | ||
| if (contains_ossl_error(error_codes, ERR_LIB_SSL, SSL_R_UNEXPECTED_EOF_WHILE_READING)) { |
There was a problem hiding this comment.
Should this pattern be used in other cases where ERR_peek_error() != 0?
In all the other cases, ERR_clear_error() is also used.
There was a problem hiding this comment.
Yeah that's probably fair
pgellert
reviewed
Feb 24, 2025
Signed-off-by: Michael Boquard <michael@redpanda.com>
Created a new make_ossl_error function that takes in a pre-generated list of OpenSSL error codes. This is helpful if the error codes were already pulled from the stack to be analyzed. Created a helper function that drains the openssl error stack and returns a list of errors. Created a contains_ossl_error function that is used to analyze a list of errors and determine if any match the library and error code. Signed-off-by: Michael Boquard <michael@redpanda.com>
Sometimes the verification errors are not the first error in the OpenSSL error stack. This change will drain the error stack into a vector of errors and then check if any of the errors match the certificate verification error codes. This will improve error messaging in situations where the peer certificate fails to verify. Signed-off-by: Michael Boquard <michael@redpanda.com>
Update remaining ERR_peek_error checks to use `get_all_ossl_errors`. Signed-off-by: Michael Boquard <michael@redpanda.com>
This change should address a possible race condition when a using application under heavy load attempts to view the DN of the peer's (client's) certificate. Once the handshake is successful, the function SSL_get1_peer_certificate() should be used to obtain a copy of the peer's certificate. The verify_callback() mechanism is still used to hold the last seen certificate that is being verified. This is necessary for error handling in order to inform a user application of which certificate failed verification. Customers have witnessed authorization failures because sometimes the wrong certificate was returned purporting to be the client's certificate. This was traced to a possible race condition between fetching the certificate and the callback function holding the wrong certificate at the time. Signed-off-by: Michael Boquard <michael@redpanda.com>
Without this change, our CI is testing against the GnuTLS implementation. Updated the install-dependencies script to include OpenSSL development headers. Signed-off-by: Michael Boquard <michael@redpanda.com>
More recent versions of OpenSSL are stricter about which extensions are required. Updated CA generation to use the `v3_ca` extensions found in cert.cfg.in. Signed-off-by: Michael Boquard <michael@redpanda.com>
michael-redpanda
force-pushed
the
tls-cert-race/core-9178/v25.1.x
branch
from
96b4655 to
7d7c0c8
Compare
|
Force push 7d7c0c8:
|
pgellert
approved these changes
Feb 24, 2025
michael-redpanda
merged commit 534c1a8
into
redpanda-data:v25.1.x
13 of 14 checks passed
This was referenced Feb 24, 2025
michael-redpanda
added a commit
to michael-redpanda/redpanda
that referenced
this pull request
Feb 24, 2025
Updated reference to pull in fix introduced by redpanda-data/seastar#175 Signed-off-by: Michael Boquard <michael@redpanda.com>
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: CORE-9178
This addresses a possible race condition that some customers have witnessed where Seastar was not returning the correct certificate and this caused authorization failures on the broker. The fix is to use the correct OpenSSL API (SSL_get1_peer_certificate) rather than the cert held by the callback.
Note: We will backport this to v24.3 and v24.2 to address this bug in all OpenSSL implementations