-
Notifications
You must be signed in to change notification settings - Fork 175
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
Fix: Decoding of IPFIX templates with Enterprise Number field #68
base: master
Are you sure you want to change the base?
Conversation
goflow fails to correctly decode IPFIX templates with Enterprise Number field. (RFC7011#section-3.2) The patch skips the Enterprise Number field, so the template can be decoded correctly.
Thanks a lot for the bugfix! (also referencing #31 as it was mentioning Enterprise Templates). |
@lspgn Thanks for promptly looking into this. Separately, Is there a more ad-hoc channel to collaborate. (slack or such) |
There is no slack for GoFlow but feel free to email me: louis at cloudflare.com Thank you for the sample, will test it out. |
@lspgn I guess merge is pending for this request. So, is this planned for next release? |
field := Field{} | ||
err = utils.BinaryDecoder(payload, &field) | ||
if field.Type > math.MaxInt16 { | ||
field.Type = field.Type - math.MaxInt16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is off by one, you need
field.Type -= 1 << 15
Because:
1000000000000001
needs to turn into just '1', so you need to subtract
1000000000000000
not
0111111111111111
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, will update. (could've sworn it was in my fork)
if field.Type > math.MaxInt16 { | ||
field.Type = field.Type - math.MaxInt16 | ||
// Skip Enterprise Number field - rfc7011#section-3.2 | ||
_ = payload.Next(4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipping the 4 bytes fixes the parsing, but I think it needs to track that this was a PEN field inside the Field
type. In the data that I have the resulting Type
will be 1 or 2. The later bits will then just treat those fields as as IPFIX_FIELD_octetDeltaCount or IPFIX_FIELD_packetDeltaCount which would be wrong.
Bugfix: issues when reading a partial chunk of protobuf from stdin
Bug: Decoding of IPFIX templates with Enterprise Number field
The goflow IPFIX template decoder isn't aware of the Enterprise Number field, which results in malformed template(s) and processing, when decoding IPFIX templates with Enterprise Number field.
Following is the field specifier format from RFC7011#section-3.2, Figure G :
When goflow tries to decode an IPFIX template with Enterprise Number, It attempts to decode the Enterprise Number field as a regular
Field
in the template.This results in a malformed template. As a result the corresponding IPFIX Datasets would not be processed. (i.e. goflow would not emit any records)
Patch
The patch adds a seperate path for IPFIX template parsing, where it checks for the
Enterprise bit
condition and skips theEnterprise Number
field.This will enable the IPFIX Template(s) to be decoded correctly and the corresponding IPFIX Datasets to be processed.