Feature/marine dissect all packet fields by ScarletMetal · Pull Request #59 · tomlegkov/marine-python

ScarletMetal · 2021-12-20T20:41:09Z

Should add parse_all_fields to simple_marine by tomorrow.
Please review the rest, as it would just be a small wrapper around Marine.parse_all_fields.

yehonatanz

Great work here, and in marine-core too.

yehonatanz · 2022-01-04T19:22:05Z

marine/marine.py

+        self,
+        packet: bytes,
+        encapsulation_type: Optional[int] = encap_consts.ENCAP_ETHERNET,
+    ) -> dict:


Dict[str, WhateverTheValueCanBe]

yehonatanz · 2022-01-04T19:23:30Z

marine/marine.py

+            return bool(value.bool_value)
+
+        if value_type == MarinePacketFieldValueType.STR:
+            return value.str_value.decode()


Specify encoding explicitly please.
Is the encoding identical for fields?

yehonatanz · 2022-01-04T19:25:09Z

tests/marine/test_marine.py

    }
+
+
+def test_parse_all_fields_int_value(tcp_packet_fields):


Test all types please

Forgot float :)

From what I've seen, nearly no built-in dissector utilizes float values.
I would remove this value type, and make it fall back to string.

yehonatanz · 2022-01-04T20:10:56Z

Rebase this one too, make each commit atomic, meaningful and blacked

ScarletMetal · 2022-01-05T14:46:24Z

Rebase this one too, make each commit atomic, meaningful and blacked

Done

tomlegkov

Very nice feature!

The main thing that worries me here is reducing the amount of consts we duplicate from the C to the Python, so that writing clients in new languages won't be too annoying :)

tomlegkov · 2022-01-06T20:16:15Z

tests/marine/test_marine.py

+def test_parse_all_fields_list_value(tcp_packet_fields):
+    ip_addr = tcp_packet_fields["ip"]["ip.addr"]
+    assert isinstance(ip_addr, list)
+    assert "10.0.0.255" in ip_addr


If you know exactly what the list looks like, consider comparing it directly to the list

assert ["10.0.0.255", "21.53.78.255"] == ip_addr

I cannot know ahead of time the order of the list produced by the dissector, So I though checking it that way would be safer.

Isn't it always src, dst?

tomlegkov · 2022-01-06T20:16:45Z

tests/marine/test_marine.py

+def test_parse_all_fields_bool_value(tcp_packet_fields):
+    tcp_ack_flag = tcp_packet_fields["tcp"]["tcp.flags_tree"]["tcp.flags.ack"]
+    assert isinstance(tcp_ack_flag, bool)
+    assert tcp_ack_flag


For safety, maybe add another assert here for a flag that is 0?

As in - the bit extracted from the dissector is 0?
Doesn't isn't the assert on line 1052 enough? it would fail if the flag is False.

I meant checking that a different tcp flag is 0

tomlegkov · 2022-01-06T20:18:11Z

tests/marine/test_marine.py

    }
+
+
+def test_parse_all_fields_int_value(tcp_packet_fields):


Forgot float :)

tomlegkov · 2022-01-06T20:21:16Z

marine/marine.py

+    def _load_child_fields(cls, children):
+        child_fields = {}
+        for child in children.data[: children.len]:
+            name = child.name.decode("ascii")


Is it really always ascii? Probably yes, but let's make sure :)

Maybe going for utf-8 is safer?

Changed to utf-8.

If this is slow in benchmarks, consider adding a cache :)

tomlegkov · 2022-01-06T20:21:52Z

marine/marine.py


+    @classmethod
+    def _load_marine_packet(cls, field):
+        children = field.children


Why do you need this variable?

tomlegkov · 2022-01-06T20:32:20Z

marine/marine.py

+]
+
+
+class MarinePacketFieldValueType:


Can you use the enum that is defined in marine.h for this? Similar to how we use MARINE_ALREADY_INITIALIZED_ERROR_CODE for example.

Sadly, this doesn't seem to be very trivial :(
The value you referenced, MARINE_ALREADY_INITIALIZED_ERROR_CODE is an int const, while marine_value_type is an enum, and I would prefer it to remain that way (separate type, automatic index generation etc).
I didn't find a simple way to expose all values of this enum from the shared library (I even found some resources that outright suggest duplicating the enum), So doing this would most likely be the lesser evil in this situation.

tomlegkov · 2022-01-10T19:37:07Z

marine/marine.py

+
+        if value_type == MarinePacketFieldValueType.LIST:
+            return [cls._load_field_value(v) for v in value.list_value[:value_len]]
+


Do we want an else here to throw an exception or something?

tomlegkov · 2022-01-10T19:39:49Z

tests/marine/test_marine.py

+def test_parse_all_fields_bool_value(tcp_packet_fields):
+    tcp_ack_flag = tcp_packet_fields["tcp"]["tcp.flags_tree"]["tcp.flags.ack"]
+    assert isinstance(tcp_ack_flag, bool)
+    assert tcp_ack_flag


I meant checking that a different tcp flag is 0

yehonatanz

Looks good, I had some more notes but nothing serious.

I'm worried about the CI, not running, so when you're done fixing stuff can you please attach a screenshot of the linter and tests pass?

yehonatanz · 2022-01-13T07:59:15Z

tests/marine/test_marine.py

+def test_parse_all_fields_list_value(tcp_packet_fields):
+    ip_addr = tcp_packet_fields["ip"]["ip.addr"]
+    assert isinstance(ip_addr, list)
+    assert "10.0.0.255" in ip_addr


Isn't it always src, dst?

yehonatanz · 2022-01-13T08:00:53Z

tests/fixtures/marine/marine_fixtures.py

        + ip.IP(src_s="10.0.0.255", dst_s="21.53.78.255")
-        + tcp.TCP(sport=16424, dport=41799)
+        + tcp.TCP(
+            sport=16424, dport=41799, flags=0b00010000, body_bytes=tcp_payload


There's some tcp.ACK const somewhere in pypacker, use it and remove the comment

yehonatanz · 2022-01-13T08:02:00Z

marine/marine_pool.py


+    def parse_all_fields(
+        self, packets: List[bytes], encapsulation_type: Optional[int] = None
+    ) -> List[Dict[str, Any]]:


Can't you use ParsedPacket here too?

yehonatanz · 2022-01-13T08:02:13Z

marine/marine_pool.py

+    @classmethod
+    def _parse_all_fields(
+        cls, packet: bytes, encapsulation_type: Optional[int] = None
+    ) -> Dict[str, Any]:


ParsedPacket?

yehonatanz · 2022-01-13T08:02:23Z

marine/simple_marine.py


+def parse_all_packet_fields(
+    packet: bytes, encapsulation_type: Optional[int] = None
+) -> Dict[str, Any]:


ParsedPacket?

ScarletMetal · 2022-01-14T10:09:31Z

@yehonatanz regarding your qusetion about src, dst.
This is technically an implementation detail of Wireshark's IP dissector, So I think it is better to depend on it as little as possible, So I would rather not guess the order of it's output.

ScarletMetal force-pushed the feature/marine-dissect-all-packet-fields branch from 4cb6a4c to 20c0291 Compare December 25, 2021 12:59

danielg55 approved these changes Jan 4, 2022

View reviewed changes

yehonatanz requested changes Jan 4, 2022

View reviewed changes

ScarletMetal force-pushed the feature/marine-dissect-all-packet-fields branch from 20c0291 to 4c0415d Compare January 5, 2022 14:46

ScarletMetal force-pushed the feature/marine-dissect-all-packet-fields branch 2 times, most recently from c4a078e to 9a71adc Compare January 5, 2022 14:59

tomlegkov requested changes Jan 6, 2022

View reviewed changes

ScarletMetal force-pushed the feature/marine-dissect-all-packet-fields branch from 9a71adc to 1aebd7a Compare January 8, 2022 14:01

tomlegkov requested changes Jan 10, 2022

View reviewed changes

ScarletMetal force-pushed the feature/marine-dissect-all-packet-fields branch from d1e7919 to b854ead Compare January 10, 2022 20:23

yehonatanz reviewed Jan 13, 2022

View reviewed changes

ScarletMetal force-pushed the feature/marine-dissect-all-packet-fields branch from b854ead to 30f7da3 Compare January 14, 2022 08:33

ScarletMetal added 3 commits January 14, 2022 11:26

Added Marine.parse_all_fields with basic tests.

fe93ff0

Extended Tests for Marine.parse_all_fields.

54ca13a

Increased marine version.

5bdb79e

ScarletMetal force-pushed the feature/marine-dissect-all-packet-fields branch from 30f7da3 to 5bdb79e Compare January 14, 2022 09:37

tomlegkov approved these changes Jan 15, 2022

View reviewed changes

tomlegkov merged commit b847143 into tomlegkov:master Jan 15, 2022


		if value_type == MarinePacketFieldValueType.LIST:
		return [cls._load_field_value(v) for v in value.list_value[:value_len]]

		]


		class MarinePacketFieldValueType:

Conversation

ScarletMetal commented Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yehonatanz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yehonatanz commented Jan 4, 2022

Uh oh!

ScarletMetal commented Jan 5, 2022

Uh oh!

tomlegkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomlegkov Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ScarletMetal Jan 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yehonatanz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ScarletMetal commented Dec 20, 2021 •

edited

Loading

tomlegkov Jan 6, 2022 •

edited

Loading

ScarletMetal Jan 8, 2022 •

edited

Loading

ScarletMetal commented Jan 14, 2022 •

edited

Loading