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

Set scope fields to 1 in receiver template descriptors. #390

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

dchristle
Copy link

This change is necessary for the Options Template used for receiver information to be valid. See RFC7011-3.4.2.2.

The Scope Field Count MUST NOT be zero.

Without this change, IPFIX parsers may report a malformed template, e.g. Wireshark shows the template in OpenWebRX packets is malformed due to the scoped field count of 0.

@jketterl
Copy link
Owner

jketterl commented Nov 3, 2024

I'm not sure if you're aware, but this implementation doesn't really need to conform to IPFIX standards. PSKreporter uses IPFIX as the base, but doesn't really implement all aspects of it. As such, I don't really see the issue in Wireshark complaining about not being able to parse the packet. What really matters is whether or not PSKreporter can parse the packet, and from what I can tell, that works perfectly fine.

Please check the development documentation: https://pskreporter.info/pskdev.html

As you can see, the "error" is present in the examples on that site, too.

@dchristle dchristle force-pushed the dchristle/scope_fields branch from 6c20d81 to 6e052e6 Compare November 5, 2024 07:18
@dchristle
Copy link
Author

I'm not sure if you're aware, but this implementation doesn't really need to conform to IPFIX standards. PSKreporter uses IPFIX as the base, but doesn't really implement all aspects of it. As such, I don't really see the issue in Wireshark complaining about not being able to parse the packet. What really matters is whether or not PSKreporter can parse the packet, and from what I can tell, that works perfectly fine.

Please check the development documentation: https://pskreporter.info/pskdev.html

As you can see, the "error" is present in the examples on that site, too.

Yes, it's true it doesn't need to comply with the RFC for PSKReporter to accept it. But the non-compliance does make analysis with utilities like Wireshark or some IPFIX libraries more difficult, since they may treat the template as malformed. I submitted an update to the website & it's now reflected there. Another change it now recommends is to advance the sequence number by the number of flows (instead of 1); I think your code already does that, which is excellent. 😄

For context, I've been using packet captures/Wireshark to analyze why some OpenWebrx payloads are getting fragmented. Since the template is malformed, I couldn't see the deserialized values at a glance & had to check them using the raw bytes manually.

I believe I found the fragmentation issue - I submitted a PR: #391

@jketterl
Copy link
Owner

jketterl commented Nov 5, 2024

I submitted an update to the website & it's now reflected there. Another change it now recommends is to advance the sequence number by the number of flows (instead of 1); I think your code already does that, which is excellent. 😄

I will check the website. The main question for me is whether it works with pskreporter.

I actually had a bit of a back and forth about the sequence numbers with Philip... See #341 😉

@dchristle
Copy link
Author

The main question for me is whether it works with pskreporter.

It should work without issue. There's no code on the PSKReporter backend that checks it & spot uploaders using scope field count = 1 are successfully uploading spots. Code-wise, the Net::Flow library (link) is used & the flow_decode() treats all fields identically w/o considering the template's scope field count.

@jketterl
Copy link
Owner

alright then, let's merge this. thank you.

@jketterl jketterl merged commit 640c5b0 into jketterl:develop Dec 11, 2024
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