-
Notifications
You must be signed in to change notification settings - Fork 62
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
Capture Information Block (CIB) addition #51
base: master
Are you sure you want to change the base?
Capture Information Block (CIB) addition #51
Conversation
51a854d
to
49853ab
Compare
49853ab
to
ac177d5
Compare
Adds CIB with location/orientation/velocity vectors, this is optionally attached to an IDB or referenced from EPBs.
c76c25d
to
ef54275
Compare
See [here](http://xml2rfc.tools.ietf.org/cgi-bin/xml2rfc.cgi?url=https://raw.githubusercontent.com/ryankurte/pcapng/proposed-wireless-fields/draft-tuexen-opsawg-pcapng.xml&modeAsFormat=html/ascii#rfc.section.4.7) for the RFC with proposed changes. |
It's not the formatting, it's the URL. Neither the URL If I go to the proposed-wireless-fields branch of your pcapng repository, click on draft-tuexen-opsawg-pcapng.xml, and then click the "Raw" button, I get the XML without the GitHub decoration, with the URL https://raw.githubusercontent.com/ryankurte/pcapng/proposed-wireless-fields/draft-tuexen-opsawg-pcapng.xml. So this url gives you the draft, with your changes, formatted as HTML. |
I've added some copy editor's notes as comments. |
Awesome, thanks! Plan to look at it again tomorrow and fix them up. |
draft-tuexen-opsawg-pcapng.xml
Outdated
|
||
<t hangText="cib_velocity"><vspace blankLines="0"/>The | ||
cib_velocity option specifies the location of the packet | ||
capture or interface; location is stored as three 32-bit floats |
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.
"velocity is stored as three 32-bit floats representing the rates of change of latitude and longitude in degrees per second, and the rate of change of altitude in metres/second."
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.
I think it makes more sense for velocity to be in meters per second against a global reference frame (orientation zero, Y facing north, -Z down). It's easier to compare and use in calculations, and works for non-global locations (see PR update introducing local locations too).
More copy-and-pasteos indicated in comments. |
Only if you assign them different numbers, otherwise some of the option numbers given for the CIB collide with already-assigned numbers for EPB options. Otherwise...
...I don't see any problem with allowing them in either type of block. |
Presumably the CIB is useful only for information that could change over time; otherwise, you'd just attach it to the IDB. Would the RSSI be better as an option for an EPB? Would the same apply to channel/frequency options? |
I'm thinking of a mobile multi-channel wireless sniffer (one IDB per channel) which could be used to survey channels and relative signal strength at given locations. Additionally, in my case one set of location details could apply to multiple interfaces and RSSI would be per actual EPB received. Speaking of conflicting option codes, the "global" comment option code (1) should still be permitted in a CIB so other options should start at 2 or other non-conflicting offset. |
I have no problem with overriding options in the EPB if that's helpful. But I agree with Guy that the option numbers must be unique in that case. It's too confusing if the same option has a different number for the same thing when present both in CIB and EPB - even if that means we skip some option numbers in the CIB options spec. Also, regarding CIB vs. MDB - I say let's call it CIB, because MDB is too generic and people might want to try and add tons of non-related-to-capture (but still "meta") information to that block. As an example I can think of storing data like pre-computed endpoint/conversation values found in the capture file, and we'll end up with a monster "block-of-all-trades" :-) |
"CIBs are associated with an IDB (via an optional interface id field) or with an EPB through the addition of a CIB ID option to the EPB." Also, what was the objection, if any, to allowing these options in the IDB itself and perhaps having the CIB as an overloaded/update IDB options block? (IOB) Too bad IDBs are sequential in order of appearance the pcapng doesn't allow updating an IDB and keeping the same interface number. Also, would an overloaded CIB option appearing in an EPB apply to just that packet or that and all subsequent packets until further update. I figure the latter makes sense. |
Alright, there appears to be some confusion arising as I hadn't updated the PR description with all the changes from the discussion here and in #48. As a quick overview (correct me if you disagree):
This is the only case in the current PR, sorry I had not updated the PR description.
Right, I'll re-assign the option numbers to be non-colliding?
It seems simpler to only allow the options in the EPB or CIB depending on their relevance to the interface or captured packet / removes the need for us to describe a heirachy and support overriding. I would prefer exclusive options personally.
Yep, except it's simpler to have one-true-way than define and handle levels of inheritance for options imo.
Yep
Definitely makes sense to re-index the options to be globally unique.
See above, that is the approach i initially proposed, but imo it's preferable to have one (simple) way of doing things and not impose parsing / encoding complexity.
The latter, check the diff or the rendered RFC for more information. |
The CIB block type code in Figure 15 is incorrect. It is showing the same number as the EPB. |
@jkcko thanks for catching that ^_^ I re-indexed the options to be unique within the CIB, do we need them to be globally unique given they can only occur in the CIB / having the CIB as the only mechanism for attaching that information simplifies encoding and parsing? |
Change CIB block ID to 0x0000000B to avoid newly included block
@guyharris merged and re-indexed to avoid the new journal block are there any outstanding things you need resolved to get this merged? |
See comments. |
Presumably if a measurement option is present, but the corresponding error option isn't present, the error is unknown? Presumably if a measurement option is absent, but the corresponding error option is present, whoever decided (directly, or indirectly by writing software to write a CIB) to provide an error estimate for a non-existent measurement is confused (i.e., ignore the error option)? |
thanks! resolved all of those i think, and added options examples. a couple more questions:
|
I would say "no", because 1) if the idea is that we'd want to be able to add that information to packet blocks as well, assigning separate option numbers for the EPB versions of the various information items isn't a huge problem in code reading pcapng files and 2) the second question you ask is a reason why it's a pain to pick globally unique option numbers - you have to pick a number that's larger than the maximum option number for all blocks to which the option would apply, and now you have holes in the available option number space for some of those block types. |
<t>Example: '41 24 cc cd 41 73 33 33 3f cc cc cd' decodes to | ||
x: 10.3m, y: 15.2m, z: 1.6m</t> | ||
|
||
<t hangText="cib_local_location_error"><vspace blankLines="0"/>The |
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.
Perhaps it should indicate that having a cib_local_location_error option without a cib_local_location makes no sense, and that having a cib_local_location option without a cib_local_location_error option means that the error is unknown.
If so, the same should be done for the other options as well.
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.
I added that an overarching comment at line 2125 rather than adding it per-option, though I can put it on each option if you would prefer?
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.
A global comment suffices.
What's the correct interpretation of a measurement without a corresponding error? "It's unknown how accurate this measurement is"?
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.
I think the absence of error information only indicates that there is no error information.
As to whether it is unknown, unknowable, elided because the capturer doesn't care, or for any other reason, would be outside the scope of the capture file?
Came across a blackhat presentation where they had implemented similar functionality, posting here so that people might consider various facets https://media.blackhat.com/bh-us-11/Cache/BH_US_11_Cache_PPI-Geolocation_WP.pdf |
hey hey, are there any other things i can do to help get this merged? |
Well, step 1 would be to change it to update the Markdown file rather than the XML file; @mcr converted the spec to extended Markdown - it's at draft-tuexen-opsawg-pcapng.md now. See cabo/kramdown-rfc2629 for the extensions. |
I'm converting the pcap file now, but I think you are referring to a merge
request against the pcapng. yes, so it needs to get re-edited, sorry.
|
@ryankurte can you rebase? I know this is years old, but sometimes it takes awhile for the IETF to get its ducks into the right geometric pattern. |
Adds a Capture Information Block (CIB) with global location/orientation/velocity vectors. CIBs are associated with an IDB using the interface number. It is intended that this will be extended with other useful fields in the future.
This began in #48, which has now been split into two components.
The proposal with changes is viewable here (thanks @guyharris!).
Please read the RFC or check out the diff for an up to date view of the changes