britram / python-ipfix

IPFIX implementation for Python 3.x
GNU Lesser General Public License v3.0
40 stars 20 forks source link

Sequence number & export time order switched #15

Closed brettdh closed 6 years ago

brettdh commented 9 years ago

When I send IPFIX constructed with MessageBuffer.to_bytes, I see the following in wireshark: image The value recorded as the seqno is actually the export timestamp, and vice versa. I found the relevant code in message.py, in MessageBuffer.to_bytes:

        # Update message header in buffer
        _msghdr_st.pack_into(self.mbuf, 0, 10, self.length,
                             self.sequence, self.export_epoch, self.odid)

As expected, sequence precedes export_epoch in the pack_into call. This is also the case with read_message:

(version, self.length, self.sequence, self.export_epoch, self.odid) = \
                _msghdr_st.unpack_from(self.mbuf, 0)

Wireshark's expectations match RFC 7011.

I'm fixing this (and adding a test) in my 2.7 branch (private for the moment). I'd be happy to cherry-pick those changes into a pull request when they're done -- or I can hand that off for now; either way is fine.

brettdh commented 9 years ago

Actually, it seems like the testutils.py test covers this, after I update the base64 string to swap those fields.

britram commented 9 years ago

This should already have been fixed in the lastest release...

On Fri, Oct 10, 2014 at 9:29 PM, Brett Higgins notifications@github.com wrote:

Actually, it seems like the testutils.py test covers this, after I update the base64 string to swap those fields.

— Reply to this email directly or view it on GitHub https://github.com/britram/python-ipfix/issues/15#issuecomment-58704842.

brettdh commented 9 years ago

I guess it wasn't? master has the same code in these spots.

On Sat, Oct 11, 2014 at 2:38 AM, Brian Trammell notifications@github.com wrote:

This should already have been fixed in the lastest release... On Fri, Oct 10, 2014 at 9:29 PM, Brett Higgins notifications@github.com wrote:

Actually, it seems like the testutils.py test covers this, after I update the base64 string to swap those fields.

— Reply to this email directly or view it on GitHub https://github.com/britram/python-ipfix/issues/15#issuecomment-58704842.


Reply to this email directly or view it on GitHub: https://github.com/britram/python-ipfix/issues/15#issuecomment-58740148

britram commented 9 years ago

Oops. 0f2b491 on develop, not merged back. Thanks for the catch.

On Sat, Oct 11, 2014 at 2:51 PM, Brett Higgins notifications@github.com wrote:

I guess it wasn't? master has the same code in these spots.

On Sat, Oct 11, 2014 at 2:38 AM, Brian Trammell notifications@github.com

wrote:

This should already have been fixed in the lastest release... On Fri, Oct 10, 2014 at 9:29 PM, Brett Higgins notifications@github.com

wrote:

Actually, it seems like the testutils.py test covers this, after I update the base64 string to swap those fields.

— Reply to this email directly or view it on GitHub < https://github.com/britram/python-ipfix/issues/15#issuecomment-58704842>.


Reply to this email directly or view it on GitHub: https://github.com/britram/python-ipfix/issues/15#issuecomment-58740148

— Reply to this email directly or view it on GitHub https://github.com/britram/python-ipfix/issues/15#issuecomment-58748489.

brettdh commented 9 years ago

Ah, I see. It looks like the 0.9.7 release (just this fix, as far as I can tell, but a few more commits including the version bump) was not merged to master either.

nickbroon commented 7 years ago

It would definitely be good to see the 0.9.7 release merged to the master branch.