Skip to content

Mqtt frames v3#1112

Closed
hsadia538 wants to merge 2 commits intoOISF:masterfrom
hsadia538:mqtt-frames-v3
Closed

Mqtt frames v3#1112
hsadia538 wants to merge 2 commits intoOISF:masterfrom
hsadia538:mqtt-frames-v3

Conversation

@hsadia538
Copy link
Contributor

Ticket: https://redmine.openinfosecfoundation.org/issues/5731

Previous PR: #1065

Describe changes:

  • Add tests for mqtt frames for truncated messages

Requires: OISF/suricata#8513

Copy link
Contributor

@jufajardini jufajardini left a comment

Choose a reason for hiding this comment

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

I have added a few comments. :)
Especially for the truncated test checks, I would like us to check on the frame length, too, so we can ensure things are being calculated as expected.
I am again wondering if we should have a special type for truncated frames, as, for instance, the length for the pdu frame in such case isn't the actual pdu length, just the max that suricata will process, as per MQTT standards.
So, having a truncated frame might let people know what to expect in such cases. Maybe in such circumstances, we could have a trunc_data frame instead of a data frame?

Comment on lines +2 to +7

requires:
features:
- HAVE_LIBJANSSON
files:
- rust/src/mqtt/parser.rs
Copy link
Contributor

Choose a reason for hiding this comment

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

These are not needed :)

- rust/src/mqtt/parser.rs

args:
- -k none
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we usually add a new line between each test.yaml session.

@@ -0,0 +1,55 @@
pcap: ../mqtt-limit-1/input.pcap

requires:
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add the min-version: 7 requirement here, since frame support, so far, is from 7 on, only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright

@hsadia538
Copy link
Contributor Author

I have added a few comments. :) Especially for the truncated test checks, I would like us to check on the frame length, too, so we can ensure things are being calculated as expected. I am again wondering if we should have a special type for truncated frames, as, for instance, the length for the pdu frame in such case isn't the actual pdu length, just the max that suricata will process, as per MQTT standards. So, having a truncated frame might let people know what to expect in such cases. Maybe in such circumstances, we could have a trunc_data frame instead of a data frame?

Sure I can do that . Will update this PR after the new frames PR

@hsadia538
Copy link
Contributor Author

I have added a few comments. :) Especially for the truncated test checks, I would like us to check on the frame length, too, so we can ensure things are being calculated as expected. I am again wondering if we should have a special type for truncated frames, as, for instance, the length for the pdu frame in such case isn't the actual pdu length, just the max that suricata will process, as per MQTT standards. So, having a truncated frame might let people know what to expect in such cases. Maybe in such circumstances, we could have a trunc_data frame instead of a data frame?

Do you also think that we should Have trunc_pdu as well along with trunc_data or just trunc_data

@jufajardini
Copy link
Contributor

I have added a few comments. :) Especially for the truncated test checks, I would like us to check on the frame length, too, so we can ensure things are being calculated as expected. I am again wondering if we should have a special type for truncated frames, as, for instance, the length for the pdu frame in such case isn't the actual pdu length, just the max that suricata will process, as per MQTT standards. So, having a truncated frame might let people know what to expect in such cases. Maybe in such circumstances, we could have a trunc_data frame instead of a data frame?

Do you also think that we should Have trunc_pdu as well along with trunc_data or just trunc_data

🤔 What about asking the others about this?

@hsadia538 hsadia538 mentioned this pull request Feb 13, 2023
@hsadia538
Copy link
Contributor Author

followed by #1123

@hsadia538 hsadia538 closed this Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants