nits ldap: improve some rust style#11525
Conversation
| Self::from_intermediate_response(msg) | ||
| } | ||
| _ => ProtocolOp::Unknown, | ||
| ldap_parser::ldap::ProtocolOp::AbandonRequest(_) => ProtocolOp::Unknown, |
There was a problem hiding this comment.
Why do we not do anything with this kind of request ?
There was a problem hiding this comment.
As I reported in a previous PR, I omitted this request because it is not handled correctly by the parser, and it gives an error when it shouldn’t, see here: rusticata/ldap-parser#6
There was a problem hiding this comment.
Thanks and sorry for the repetition
From what I understand, the rusticate parser errors when it should not.
But when it does not error and returns some AbandonRequest, Suricata should still handle it, right ?
There was a problem hiding this comment.
No pb!
The point is that it always returns an error, and we will never have the abandon request parsed correctly until the parser is fixed.
There was a problem hiding this comment.
Are you sure it always returns an error ?
There was a problem hiding this comment.
Does not look the case to me from the error
parsing failed: Error(Ber(UnexpectedClass { expected: Some(ContextSpecific), actual: Application }))
Looks like it could return a AbandonRequest with ContextSpecific tags instead of an Application
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11525 +/- ##
==========================================
- Coverage 82.54% 82.49% -0.05%
==========================================
Files 923 923
Lines 248460 248459 -1
==========================================
- Hits 205083 204960 -123
- Misses 43377 43499 +122
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: ERROR: QA failed on SURI_TLPW2_autofp_suri_time. ERROR: QA failed on SURI_TLPR1_suri_time.
Pipeline 21664 |
|
Information: QA ran without warnings. Pipeline 21782 |
|
Good on this ? |
I would like to take another look before getting it merged. |
friendly ping on this @glongo ? |
I will try to look at it this week, sorry for the delay. |
|
@glongo any further thoughts? |
|
Looks good to me and I think it should be merged. |
|
Merged in #11958, thanks! |
4 participants
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/1199
Describe changes:
cc @glongo