Skip to content
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

Suggestions for #3160 #3272

Merged
merged 4 commits into from
Oct 10, 2024
Merged

Suggestions for #3160 #3272

merged 4 commits into from
Oct 10, 2024

Conversation

olix0r
Copy link
Member

@olix0r olix0r commented Oct 10, 2024

commit f78aa64
Author: Oliver Gould [email protected]
Date: Thu Oct 10 14:18:31 2024 +0000

tls: Avoid InsertParam parameter.

We don't actually use InsertParam all that much--only in the TLS server (which
is obviously why it was included here). This change removes the InsertParam in
favor of using a tuple, generally reducing boilerplate.

It turns out that the TLS stack already has a map_target to handle turning the
tuple-target into a Tls type, so it shouldn't be needed.

commit 67c0e84
Author: Oliver Gould [email protected]
Date: Thu Oct 10 14:49:32 2024 +0000

tls: Remove ExtractParam from detect_sni

Similarly, we don't actually care about extracting a timeout from the target.
Using an ExtractParam causes needless boilerplate.

This change updates the stack module to simply take a timeout parameter at
construction time.

commit f6202d6
Author: Oliver Gould [email protected]
Date: Thu Oct 10 15:03:42 2024 +0000

tls: Make the detect_tls module private

We now only need to export the NewDetectSni type. The module reexport is not
necessary.

commit ae88d2e (HEAD -> ver/zd/tls-stack, origin/ver/zd/tls-stack)
Author: Oliver Gould [email protected]
Date: Thu Oct 10 15:20:17 2024 +0000

tls: Reorganize NewDetectRequiredSni under the server module

Because the DetectTls and DetectSni types are so similar -- and implemented in
the context of a server inspecting a provided connection (and not a client
establishing a TLS connection), this change reorganizes the module:

* The DetectSni types are renamed to DetectRequiredSni to better reflect their
  purpose and difference from the DetectTls type.
* The detect_sni module is renamed and moved to server::required_sni. This
  module is private and the relevant types are reexported from the server

We don't actually use InsertParam all that much--only in the TLS server (which
is obviously why it was included here). This change removes the InsertParam in
favor of using a tuple, generally reducing boilerplate.

It turns out that the TLS stack already has a map_target to handle turning the
tuple-target into a Tls type, so it shouldn't be needed.
Similarly, we don't actually care about extracting a timeout from the target.
Using an ExtractParam causes needless boilerplate.

This change updates the stack module to simply take a timeout parameter at
construction time.
We now only need to export the NewDetectSni type. The module reexport is not
necessary.
Because the DetectTls and DetectSni types are so similar -- and implemented in
the context of a server inspecting a provided connection (and not a client
establishing a TLS connection), this change reorganizes the module:

* The DetectSni types are renamed to DetectRequiredSni to better reflect their
  purpose and difference from the DetectTls type.
* The detect_sni module is renamed and moved to server::required_sni. This
  module is private and the relevant types are reexported from the server
  module.
@olix0r olix0r requested a review from a team as a code owner October 10, 2024 15:24
@olix0r olix0r changed the title Ver/zd/tls stack Suggestions for #3160 Oct 10, 2024
Comment on lines +33 to +34
/// - There are no special affordances for mutually authenticated TLS, so we
/// make no attempt to detect the client's identity.
Copy link
Member

Choose a reason for hiding this comment

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

What are the special affordances in the other case? From my understand, in the DetectTLS, we determine whether we terminate based on whether the SNI matches the local identitty. Is that what you are refering to here?

@zaharidichev zaharidichev merged commit 6cb613d into zd/tls-stack Oct 10, 2024
15 checks passed
@zaharidichev zaharidichev deleted the ver/zd/tls-stack branch October 10, 2024 15:52
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