-
Notifications
You must be signed in to change notification settings - Fork 722
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
fix(integrationv2): Skip unsupported client auth tests #5096
Merged
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
76f2371
to
6eabe5a
Compare
6eabe5a
to
71181c3
Compare
71181c3
to
324f6b4
Compare
goatgoose
commented
Feb 7, 2025
Comment on lines
-108
to
-111
if protocol is not None: | ||
for provider_ in providers: | ||
if provider_.supports_protocol(protocol, with_cert=certificate) is False: | ||
return True |
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.
supports_protocol()
is already called on each provider above, on line 85. So now that the certificate check is separate, this duplicate call to supports_protocol()
is no longer needed.
jmayclin
approved these changes
Feb 7, 2025
Co-authored-by: James Mayclin <[email protected]>
dougch
approved these changes
Feb 10, 2025
johubertj
pushed a commit
to johubertj/s2n-tls
that referenced
this pull request
Feb 13, 2025
Co-authored-by: James Mayclin <[email protected]>
johubertj
pushed a commit
to johubertj/s2n-tls
that referenced
this pull request
Feb 13, 2025
Co-authored-by: James Mayclin <[email protected]>
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.
Description of changes:
The client auth integration test is not currently running on PRs, and it fails when I run it. This PR fixes this test and adds it to the integrationv2 buildspec so it will run on PRs.
I think this test would have started failing after #4949, which started allowing RSA PSS certificates to be used with TLS 1.2 in the integration tests. I think this change broke the client auth test because there's currently no logic to prevent an RSA PSS cert from being tested in a scenario in which one peer supports RSA PSS signing and the other doesn't.
This worked before when RSA PSS certificates were only tested in TLS 1.3 since TLS 1.3 tests are skipped if either provider doesn't support TLS 1.3:
s2n-tls/tests/integrationv2/utils.py
Lines 82 to 84 in 806830d
This PR updates the test skipping logic to skip testing RSA PSS certificates if either of the two providers don't support RSA PSS signing.
Test failure investigation
The test fails with the following error:
I think this error is caused by an s2n-tls server that doesn't support RSA PSS signing, and an OpenSSL 1.1.1 client that does support RSA PSS signing. The test certs are being correctly filtered for the server, so s2n-tls is given a normal RSA cert. But the client certificate isn't filtered, so OpenSSL 1.1.1 is given an RSA PSS certificate. I think since OpenSSL 1.1.1 supports RSA PSS certificates, the failure isn't observed until later when OpenSSL doesn't send any client certificate to s2n-tls, likely because s2n-tls isn't negotiating RSA PSS as the signature algorithm.
Call-outs:
It seemed weird that the logic for skipping certificates is in
supports_protocol()
, so I moved this to a separatesupports_certificate()
function.Testing:
The client auth test was re-added to the integrationv2 buildspec for PRs, and should now succeed in this PR. I confirmed that it's now running:
I double checked that we're still testing RSA PSS certificates when s2n-tls is linked to a libcrypto that supports RSA PSS signing. Examples from the aws-lc log:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.