Skip to content

Expose ssl_select_cert_disable_ech#479

Open
sharrington22 wants to merge 1 commit intocloudflare:masterfrom
sharrington22:sharrington/ech-fallback
Open

Expose ssl_select_cert_disable_ech#479
sharrington22 wants to merge 1 commit intocloudflare:masterfrom
sharrington22:sharrington/ech-fallback

Conversation

@sharrington22
Copy link

ssl_select_cert_disable_ech should be returned by the certificate selection callback when it wants ECH fallback to occur (falling back from the inner client hello to the outer client hello). This exposes it for both the sync and the async callback paths.

Copy link
Member

@ghedo ghedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few minor comments, lgtm otherwise.

/// A fatal error to be returned from async select certificate callbacks.
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub struct AsyncSelectCertError;
pub enum AsyncSelectCertError {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really an "error" anymore, and since we are changing the type anyway we might as well rename it as well. Maybe "AsyncSelectCertResult" or "AsyncSelectCertAction" or something along those lines.

@@ -230,7 +237,12 @@ where

/// A fatal error to be returned from async select certificate callbacks.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs to be updated to reflect the fact that it's not really a fatal error anymore.

/// A fatal error to be returned from async select certificate callbacks.
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub struct AsyncSelectCertError;
pub enum AsyncSelectCertError {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also maybe make this enum non_exhaustive so future changes won't break API, but not 100% sure that makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants