Conversation
| enum Context { | ||
| Bindings(ErrorType, &'static str, &'static str), | ||
| Code(s2n_status_code::Type, Errno), | ||
| Code(s2n_status_code::Type, Errno, ConnContext), |
There was a problem hiding this comment.
I'd prefer to punt this down the road a little bit. E.g. if we end up having two things from the connection that we need then we can go ahead and add the context struct. But we already have so many context structs.
So for now I'd prefer to directly store Option<AlertDescription> in the Code variant.
| Code(s2n_status_code::Type, Errno, ConnContext), | |
| Code(s2n_status_code::Type, Errno, Option<AlertDescription>), |
| let result = s2n_shutdown(self.connection.as_ptr(), &mut blocked); | ||
| (&self, result).into_poll().map_ok(|_| self) |
There was a problem hiding this comment.
I worry that this is a bit too "magic". I'd prefer to just directly try and pull the alert from the connection, and manually add it to the error.
| let result = s2n_shutdown(self.connection.as_ptr(), &mut blocked); | |
| (&self, result).into_poll().map_ok(|_| self) | |
| let result = s2n_shutdown(self.connection.as_ptr(), &mut blocked); | |
| if let Err(e) = &mut result { | |
| e.attach_alert(self.connection.alert()); | |
| } | |
| result.into_poll().map_ok(|_| self) |
There was a problem hiding this comment.
I can make it more explicit, but I'm against always adding the alert to the error. Only ErrorType::Alert should log the alert. If errors always include an alert, I worry it'll be misunderstood the way errno currently is. Maybe there's currently no way for a connection to have an alert stored but return a different error, but that doesn't seem like a necessary or reliable assumption.
Maybe that's not what you're suggesting here, but that's what "attach_alert" reads as to me. For that reason I'd prefer something more vague like "attach_conn_info".
There was a problem hiding this comment.
Agreed on making it more explicit, but I like keeping the alert conditional on ErrorType::Alert to avoid potential confusion
| } | ||
|
|
||
| #[test] | ||
| fn handshake_error_includes_alert() -> Result<(), Box<dyn std::error::Error>> { |
There was a problem hiding this comment.
Can we also update this test to check that the alert is getting surfaced?
s2n-tls/bindings/rust/standard/integration/src/handshake_failure_errors.rs
Lines 103 to 107 in f9f9282
| debug.alert = conn.alert().map(|a| a.into()); | ||
| } |
There was a problem hiding this comment.
I just noticed s2n_connection_get_alert actually consumes the alert. So you can't call it twice. So if s2n-tls itself calls it, customers won't be able to get the alert themselves 😬
Need to fix that so this isn't a breaking change. s2n_connection_get_alert might also need some warnings on it, since getters don't usually mutate...
| let result = s2n_shutdown(self.connection.as_ptr(), &mut blocked); | ||
| (&self, result).into_poll().map_ok(|_| self) |
There was a problem hiding this comment.
Agreed on making it more explicit, but I like keeping the alert conditional on ErrorType::Alert to avoid potential confusion
| let mut pair = TestPair::from_config(&config); | ||
|
|
||
| const ALERT_CODE: u8 = 45; | ||
| const ALERT: &[u8] = &[21, 3, 3, 0, 2, 1, ALERT_CODE]; |
There was a problem hiding this comment.
nit: What do you think about annotating the bytes? Something like
| const ALERT: &[u8] = &[21, 3, 3, 0, 2, 1, ALERT_CODE]; | |
| const ALERT: &[u8] = &[ | |
| 21, // TLS record type: alert | |
| 3, 3, // TLS version 1.2 (record layer) | |
| 0, 2, // payload length: 2 bytes | |
| 1, // alert level: fatal | |
| ALERT_CODE, // alert description | |
| ]; | |
Goal
Make the actual alert received available when s2n-tls reports an ErrorType::Alert error.
Why
ErrorType::Alert is not helpful without the actual alert code, and implementing error handling logic everywhere it's required is tedious. It's also fun when you don't notice the alert isn't being logged until the code has been deployed 😬
How
I add an option to provide the connection when converting low level return values into Errors. If the connection is provided, then information like the alert can be retrieved and stored on the Error.
Callouts
Testing
I added a unit test that intentionally sends a hard coded plain text alert as part of the handshake, then ensures that the proper alert description is reported.
Related
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.