-
Notifications
You must be signed in to change notification settings - Fork 123
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
Add set_ticket_key_callback (SSL_CTX_set_tlsext_ticket_key_cb) #330
base: master
Are you sure you want to change the base?
Changes from all commits
9bf2162
818157a
f6a55ef
9a6e166
cb41dbc
d5ecffd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -903,6 +903,53 @@ pub enum SslInfoCallbackValue { | |
Alert(SslInfoCallbackAlert), | ||
} | ||
|
||
/// Ticket key callback status. | ||
#[derive(Debug, Copy, Clone, PartialEq, Eq)] | ||
pub enum TicketKeyCallbackResult { | ||
/// Abort the handshake. | ||
Error, | ||
|
||
/// Continue with a full handshake. | ||
/// | ||
/// When in decryption mode, this indicates that the peer supplied session ticket was not | ||
/// recognized. When in encryption mode, this instructs boring to not send a session ticket. | ||
/// | ||
/// # Note | ||
/// | ||
/// This is a decryption specific status code when using the submoduled BoringSSL. | ||
Noop, | ||
|
||
/// Resumption callback was successful. | ||
/// | ||
/// When in decryption mode, attempt an abbreviated handshake via session resumption. When in | ||
/// encryption mode, provide a new ticket to the client. | ||
Success, | ||
|
||
/// Resumption callback was successful. Attempt an abbreviated handshake, and additionally | ||
/// provide new session tickets to the peer. | ||
/// | ||
/// Session resumption short-circuits some security checks of a full-handshake, in exchange for | ||
/// potential performance gains. For this reason, a session ticket should only be valid for a | ||
/// limited time. Providing the peer with renewed session tickets allows them to continue | ||
/// session resumption with the new tickets. | ||
/// | ||
/// # Note | ||
/// | ||
/// This is a decryption specific status code. | ||
DecryptSuccessRenew, | ||
} | ||
|
||
impl From<TicketKeyCallbackResult> for c_int { | ||
fn from(value: TicketKeyCallbackResult) -> Self { | ||
match value { | ||
TicketKeyCallbackResult::Error => -1, | ||
TicketKeyCallbackResult::Noop => 0, | ||
TicketKeyCallbackResult::Success => 1, | ||
TicketKeyCallbackResult::DecryptSuccessRenew => 2, | ||
} | ||
} | ||
} | ||
|
||
#[derive(Hash, Copy, Clone, PartialOrd, Ord, Eq, PartialEq, Debug)] | ||
pub struct SslInfoCallbackAlert(c_int); | ||
|
||
|
@@ -1185,6 +1232,43 @@ impl SslContextBuilder { | |
} | ||
} | ||
|
||
/// Configures a custom session ticket key callback for session resumption. | ||
/// | ||
/// Session Resumption uses the security context (aka. session tickets) of a previous | ||
/// connection to establish a new connection via an abbreviated handshake. Skipping portions of | ||
/// a handshake can potentially yield performance gains. | ||
/// | ||
/// An attacker that compromises a server's session ticket key can impersonate the server and, | ||
/// prior to TLS 1.3, retroactively decrypt all application traffic from sessions using that | ||
/// ticket key. Thus ticket keys must be regularly rotated for forward secrecy. | ||
/// | ||
/// # Panics | ||
/// | ||
/// This method panics if this `Ssl` is associated with a RPK context. | ||
#[corresponds(SSL_CTX_set_tlsext_ticket_key_cb)] | ||
pub fn set_ticket_key_callback<F>(&mut self, callback: F) | ||
where | ||
F: Fn( | ||
&SslRef, | ||
&mut [u8; 16], | ||
*mut u8, | ||
*mut ffi::EVP_CIPHER_CTX, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's unfortunate that we have to pass these as raw pointers to users. How feasible is it to create safe bindings for these? If we do this then we'll have another task in our list for the 5.0 major bump There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So based on the docs the callback needs to configure the hmac and enc contexts. For safe bindings I think we would need to expose a safe wrapper around EVP_CipherInit_ex, EVP_DecryptInit_ex and HMAC_Init_ex and also make it extensible enough for various uses that customers might want. From what I can tell its not trivial and would probably be a separate task than this PR since its complex enough. That being said, I am not familiar with the 5.0 major bump initiative so can't comment on if its better to wait for safe bindings or if its ok to merge this now. |
||
*mut ffi::HMAC_CTX, | ||
bool, | ||
) -> TicketKeyCallbackResult | ||
+ 'static | ||
+ Sync | ||
+ Send, | ||
{ | ||
#[cfg(feature = "rpk")] | ||
assert!(!self.is_rpk, "This API is not supported for RPK"); | ||
toidiu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
unsafe { | ||
self.replace_ex_data(SslContext::cached_ex_index::<F>(), callback); | ||
ffi::SSL_CTX_set_tlsext_ticket_key_cb(self.as_ptr(), Some(raw_ticket_key::<F>)) | ||
}; | ||
} | ||
|
||
/// Sets the certificate verification depth. | ||
/// | ||
/// If the peer's certificate chain is longer than this value, verification will fail. | ||
|
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.
The IV is constrained to a max length (https://github.com/google/boringssl/blob/main/ssl/ssl_session.cc#L331-L335) so instead of giving applications a raw pointer we should handle the unsafe conversion to a slice constrained to
ffi::EVP_MAX_IV_LENGTH
ourselvesThere 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.
Yea good call. I wasn't sure if the length was dependent on the cipher.
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.
Just to confirm, are we ok looking at BoringSSL implementation details to derive this API? The docs, quoted above only guarantee a 16 byte key name and a fresh IV (unspecified length).