quiclog / pcap2qlog

A tool to convert .pcap and .pcapng files into qlog files
MIT License
26 stars 7 forks source link

allow logging tshark's fields for frames unknown by pcap2qlog but known by tshark (off by default) #4

Closed francoismichel closed 4 years ago

francoismichel commented 4 years ago

Hello,

Some people are currently working on QUIC extensions involving new frames that are not part of the standard, such as multipath or FEC.

I have some FEC-specific frames that I added to the tshark dissector but the fields of those frames are ignored by pcap2qlog even if they are present in the tshark dissection. This pull requests adds the --logunknownframesfields CLI flag that, when set, makes pcap2qlog copy the fields known by tshark in the qlog description of the unknown frame.

Here is an example of what it practically does. I added to tshark the dissection of the RECOVERED frame for FEC. Without the pull-request, a packet containing this frame looks like this in the qlog output:

"frames": [
    {
        "frame_type": "unknown_frame_type",
        "raw_frame_type": "43",
    }
]

When using the code of this pull request and setting the --logunknownframesfields CLI flag, pcap2qlog adds the fields parsed by tshark under the raw_frame_content field of the frame :

"frames": [
    {
        "frame_type": "unknown_frame_type",
        "raw_frame_type": "43",
        "raw_frame_content": {
                "quic.frame_type": "43",
                "quic.recovered.n_symbols": "1",
                "quic.recovered.first_packet": "0x00000000000017e3",
                "quic.recovered.recovered_src_fpi": "6109"
        }
    }
]

When the --logunknownframesfields flag is not set, the frame is exactly the same as without the code of this pull request, which means that the raw_frame_content is not even present.

Do not hesitate to let me know if you need some modifications of the code before merging. Also, I have no problem at all if you prefer not adding this feature at all. :-)

François

rmarx commented 4 years ago

Hello François,

Thanks for the PR!

This ties into a few open issues for qlog on a whole (e.g., https://github.com/quiclog/internet-drafts/issues/104), where we're unsure how to deal with raw, potentially privacy-sensitive data exposure options. For this, I had actually previously removed the option to include raw data in the pcap2qlog output (though the high-level boolean seems to still be there...). Adding something similar back in and blanket copying over -all- wireshark entries as you do here does give some pause.

That's not to say I won't merge this PR :) I just wanted to give some context for later readers + say that I like the approach with the boolean flag here. I probably won't enable it by default on e.g., the public qvis server and don't think I will include the "raw_frame_content" field in qlog. However, I feel this is useful for people implementing new frames (and taking the effort to add wireshark dissection for them!), so will merge the PR for now.