Decouple stream bypass from TLS encrypted bypass v9.2 #12655
Conversation
Decouple app.protocols.tls.encryption-handling and stream.bypass. There's no apparent reason why encrypted TLS bypass traffic should depend on stream bypass, as these are unrelated features. Ticket: 6788
lukashino
requested review from
catenacyber,
jufajardini,
victorjulien and
a team
as code owners
|
digging up a comment from PR #12388 :
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12655 +/- ##
=======================================
Coverage 80.77% 80.77%
=======================================
Files 932 932
Lines 259517 259549 +32
=======================================
+ Hits 209629 209661 +32
Misses 49888 49888
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
WARNING:
Pipeline 24863 |
|
Are QA results expected ? (should we set label requires-baseline ? ) |
| @@ -203,13 +224,24 @@ impl SSHState { | |||
| parser::MessageCode::NewKeys => { | |||
| hdr.flags = SSHConnectionState::SshStateFinished; | |||
| if ohdr.flags >= SSHConnectionState::SshStateFinished { | |||
| unsafe { | |||
There was a problem hiding this comment.
Why are we changing the default ?
SSH by default did like SSH_HANDLE_ENCRYPTION_BYPASS here : APP_LAYER_PARSER_NO_INSPECTION | APP_LAYER_PARSER_NO_REASSEMBLY | APP_LAYER_PARSER_BYPASS_READY
| # hassh: no | ||
|
|
||
| # What to do when the encrypted communications start: | ||
| # - default: keep tracking but stop inspection |
There was a problem hiding this comment.
Should not be named default but how it behaves like stop_inspect_only, right ?
| @@ -55,6 +55,8 @@ | |||
|
|
|||
| /* HASSH fingerprints are disabled by default */ | |||
| #define SSH_CONFIG_DEFAULT_HASSH false | |||
| /* Bypassing the encrypted part of the connections */ | |||
| #define SSH_CONFIG_DEFAULT_ENCRYPTION_BYPASS SSH_HANDLE_ENCRYPTION_DEFAULT | |||
There was a problem hiding this comment.
nit : why do we need this define instead of reusing SSH_HANDLE_ENCRYPTION_DEFAULT directly ?
|
|
||
| static mut ALPROTO_SSH: AppProto = ALPROTO_UNKNOWN; | ||
| static HASSH_ENABLED: AtomicBool = AtomicBool::new(false); | ||
|
|
||
| static ENCRYPTION_BYPASS_ENABLED: AtomicU8 = AtomicU8::new(SshEncryptionHandling::SSH_HANDLE_ENCRYPTION_DEFAULT as u8); |
There was a problem hiding this comment.
cc @jasonish is this the good rust way ?
There was a problem hiding this comment.
Yes, and no. Its the recommended way for a global static mutable, just making it a static mutable results in a compiler warning:
- See: rust: don't suppress static_mut_refs globally - v2 #12673
- And: https://doc.rust-lang.org/nightly/edition-guide/rust-2024/static-mut-references.html
But perhaps not the best Suricata way if only modified on initialization, see:
#12665
There was a problem hiding this comment.
https://doc.rust-lang.org/nightly/edition-guide/rust-2024/static-mut-references.html#no_std-one-time-initialization looks pretty similar to what we do now, except adding a wrapper function to return a const (instead of mut)
There was a problem hiding this comment.
Still uses atomics right?
Not really, atomics are here not used for static mut STATE
They just want to add checks for the initialization of the global, but in our cases, it is done right I think.
There was a problem hiding this comment.
Yeah, seems as simple as this: b486d55
| } | ||
|
|
||
| #[no_mangle] | ||
| pub extern "C" fn SCSshEncryptionBypassMode() -> SshEncryptionHandling { |
There was a problem hiding this comment.
unused, should be removed, right ?
Following up on #12573
Redmine ticket: https://redmine.openinfosecfoundation.org/issues/6788
Describe changes:
v9.2:
v9.1:
v9:
v8.1:
v8:
v7
v6
v5
v4
v3
encryption-handlingallowing to choose whether to continue inspection on SSH once it turns encryptedSV_BRANCH=OISF/suricata-verify#2315