Add API method to get 'hypothetical' alerts #3171
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
|
Benchmark report |
maddeleine
reviewed
Jan 20, 2022
| S2N_TLS_ALERT_CERTIFICATE_REQUIRED = 116, | ||
| S2N_TLS_ALERT_NO_APPLICATION_PROTOCOL = 120, | ||
| } s2n_tls_alert_code; | ||
|
|
There was a problem hiding this comment.
Some of the alerts didn't transfer over. Where did S2N_TLS_ALERT_DECOMP_FAILED go?
There was a problem hiding this comment.
I only added the alerts as of TLS1.3. Some old alerts were dropped in TLS1.3. We can add them back if we need them, but they weren't previously being used.
camshaft
reviewed
Jan 20, 2022
| @@ -138,6 +139,15 @@ int test_count; | |||
| RESET_ERRNO(); \ | |||
| } while(0) | |||
|
|
|||
| #define EXPECT_FAILURE_WITH_ALERT( function_call, err, alert ) \ | |||
|
Benchmark report |
camshaft
approved these changes
Jan 21, 2022
maddeleine
approved these changes
Jan 21, 2022
dougch
pushed a commit
to dougch/s2n-tls
that referenced
this pull request
Jan 28, 2022
dougch
pushed a commit
to dougch/s2n-tls
that referenced
this pull request
Jan 28, 2022
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:
For extra security, s2n-tls sends generic close_notify alerts instead of specific TLS alerts.
However, some QUIC interop/compliance tests require specific alerts. Customers might also appreciate knowing what alert an error corresponds to, although alerts should generally be less specific than s2n-tls errors. Additionally, proving compliance with the TLS RFCs would be easier if we could enforce that a specific alert would be sent if we didn't just send a generic close_notify.
I added a new method to the s2n_quic_support APIs to "translate" connection errors into "protocol" / "hypothetical" TLS alerts. For now it mostly just returns internal_error, but we can add more mappings in the future and enforce that new protocol errors include a mapping.
Call-outs:
Should the new method handle all error types, or just S2N_ERR_T_PROTO? Technically only protocol errors should map to alerts. There's also already an existing "s2n_connection_get_alert" method that returns an alert sent by the peer, if it exists. However, forcing customers to branch on the error type to call different methods seems like a poor user experience, so I'd prefer if s2n handles the branching.
Why not add all the alert mappings immediately? Our protocol errors are a bit of a mess. Some of the current protocol errors don't correspond to actual protocol errors / alerts and will need to be changed to internal errors. Additionally, some code will need to be changed to return different errors /alert in order to match the RFC. To keep this initial code change simple, I stuck to just the first two alerts that QUIC implementations are interested in.
Testing:
Added unit tests for the new API, as well as unit tests to demonstrate how we can enforce RFC alert requirements.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.