Skip to content

nits ldap: improve some rust style #11525

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.

Sign up for GitHub

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

Closed
catenacyber wants to merge 1 commit into from
Closed

nits ldap: improve some rust style #11525

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 4 additions & 18 deletions rust/src/ldap/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ impl<'a> From<ldap_parser::ldap::LdapMessage<'a>> for LdapMessage {
ldap_parser::ldap::ProtocolOp::IntermediateResponse(msg) => {
Self::from_intermediate_response(msg)
}
_ => ProtocolOp::Unknown,
ldap_parser::ldap::ProtocolOp::AbandonRequest(_) => ProtocolOp::Unknown,
Copy link
Contributor Author

@catenacyber catenacyber Jul 19, 2024

Choose a reason for hiding this comment

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

Why do we not do anything with this kind of request ?

Copy link
Contributor

@glongo glongo Jul 19, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@catenacyber catenacyber Jul 19, 2024

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

@glongo glongo Jul 19, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@catenacyber catenacyber Jul 26, 2024

Choose a reason for hiding this comment

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

Are you sure it always returns an error ?

Copy link
Contributor

@glongo glongo Jul 26, 2024

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor Author

@catenacyber catenacyber Jul 26, 2024

Choose a reason for hiding this comment

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

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

};
let controls = ldap_msg.controls.map(|ctls| {
ctls.iter()
Expand Down Expand Up @@ -414,6 +414,7 @@ impl LdapMessage {
| ProtocolOp::DelRequest(_)
| ProtocolOp::ModDnRequest(_)
| ProtocolOp::CompareRequest(_)
| ProtocolOp::Unknown // AbandonRequest
| ProtocolOp::ExtendedRequest(_) => {
return true;
}
Expand All @@ -424,23 +425,8 @@ impl LdapMessage {
}

pub fn is_response(&self) -> bool {
match self.protocol_op {
ProtocolOp::BindResponse(_)
| ProtocolOp::SearchResultEntry(_)
| ProtocolOp::SearchResultReference(_)
| ProtocolOp::SearchResultDone(_)
| ProtocolOp::ModifyResponse(_)
| ProtocolOp::AddResponse(_)
| ProtocolOp::DelResponse(_)
| ProtocolOp::ModDnResponse(_)
| ProtocolOp::CompareResponse(_)
| ProtocolOp::ExtendedResponse(_) => {
return true;
}
_ => {
return false;
}
}
// it is either a response or a request
return !self.is_request();
}

fn from_bind_request(msg: ldap_parser::ldap::BindRequest) -> ProtocolOp {
Expand Down