New BPv7 with working UTS and patched cbor modules to support it - #5075
New BPv7 with working UTS and patched cbor modules to support it#5075BrianSipos wants to merge 1 commit into
Conversation
|
There seems to be some inconsistency with existing cborfields interface between use of |
439ffe0 to
9ffc348
Compare
| # type: (CBOR_Packet) -> bytes | ||
| return b"".join(obj.build(pkt) for obj in self.seq) | ||
|
|
||
| class CBORF_ARRAY(CBORF_field[List[Any], List[Any]]): |
There was a problem hiding this comment.
The array could be refactored to be a subclass of CBORF_SEQUENCE with the encoded prefix CBOR head.
| if major_type != 4: | ||
| raise CBOR_Decoding_Error( | ||
| "Expected major type 4 (array), got %d" % major_type) | ||
| if count != len(self.seq): |
There was a problem hiding this comment.
This logic on seq does not account for optional or conditional fields not being part of the encoded array.
| cond, # type: Callable[[Packet], bool] | ||
| ): | ||
| fields.ConditionalField.__init__(self, fld, cond) | ||
| # Leave CBORF_field uninitialized |
There was a problem hiding this comment.
I don't know a good behavior here because ConditionalField.__getattr__ forwards all names to the sub-field.
|
|
||
| CBOR_root = CBORF_INDEFINITE_ARRAY( | ||
| CBORF_PACKET('primary', default=PrimaryBlock(), cls=PrimaryBlock), | ||
| CBORF_SEQUENCE_OF('blocks', default=[], |
There was a problem hiding this comment.
Likewise here, the payload of the entire bundle is the btsd field of the block having block_type==1 (and supposed to be the last block in this sequence). Some magic could be added to copy/clone the payload content as a next-layer packet.
|
This PR is cool, thanks a lot for doing it ! We're (mostly @polybassa) currently still performing some cleanups regarding the ASN1/CBOR, so sorry if we take a little bit of time to review this. |
I can write up a separate issue if it would help, but I really think it's most consistent to keep with the model used for other encoding types that:
Right now the internal form is CBOR_* instances with some complex logic to convert types where necessary and this really clutters up the human (h) view of things with text like I can propose some changes in this PR (with concrete example via the |
AI-Assisted: no
7b8c96e to
1aad3f4
Compare
|
Hi, Sorry for the delay. I'm somewhat busy this week, therefore I used some AI to help me with the review. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5075 +/- ##
===========================================
- Coverage 80.50% 47.95% -32.56%
===========================================
Files 390 373 -17
Lines 96785 96937 +152
===========================================
- Hits 77919 46484 -31435
- Misses 18866 50453 +31587
🚀 New features to boost your workflow:
|
polybassa
left a comment
polybassa
left a comment
There was a problem hiding this comment.
Thanks for the PR. I'll also provide a UTS file that triggers the findings so you can more easy verify if they are fixed.
References:
| class CBOR_Packet(Packet, metaclass=CBORPacket_metaclass): | ||
| CBOR_root = cast('CBORF_field[Any, Any]', None) | ||
|
|
||
| def setfieldval(self, attr, val): |
There was a problem hiding this comment.
1. [BLOCKING] Remove CBOR_Packet.setfieldval()
The PR adds a packet-level override that wraps a value before calling the normal Scapy implementation:
def setfieldval(self, attr, val):
fld = cast("CBORF_field", self.get_field(attr))
val = fld._wrap(val)
super().setfieldval(attr, val)This is unnecessary because Packet.setfieldval() already calls the field’s any2i() method, and CBORF_field.any2i() already calls _wrap(). The current implementation therefore performs conversion twice for ordinary CBOR fields. ([GitHub][2])
More importantly, the override changes the order of specialized conversions. For example, CBORF_UNSIGNED_FLAGS.any2i() first delegates symbolic strings to Scapy’s FlagsField and only then wraps the resulting integer. The packet override calls _wrap() first, so an assignment such as:
pkt = PrimaryBlock()
pkt.bundle_flags = "PAYLOAD_ADMIN"tries to convert "PAYLOAD_ADMIN" directly to an integer before FlagsField.any2i() can interpret it. By contrast, constructor assignment reaches any2i() directly:
pkt = PrimaryBlock(bundle_flags="PAYLOAD_ADMIN")The two assignment styles therefore have different semantics. The same issue affects enum fields and custom fields such as DtnTimeField. ([GitHub][3])
The override also prevents normal Scapy payload delegation. Base Packet.setfieldval() handles the special payload attribute and can forward an unknown field assignment into the payload. The CBOR override calls _wrap() on the result of get_field() before that logic runs; when the field belongs to the payload, there is no local CBOR field to wrap. ([GitHub][4])
Recommended change
Remove the override entirely:
class CBOR_Packet(Packet, metaclass=CBORPacket_metaclass):
CBOR_root = ...All Python-to-internal conversion should remain in each field’s any2i() implementation.
Required regression tests
pkt = PrimaryBlock()
pkt.bundle_flags = "PAYLOAD_ADMIN"
assert pkt.bundle_flags == PrimaryBlock.Flag.PAYLOAD_ADMIN
pkt = PrimaryBlock()
pkt.crc_type = "CRC32"
assert pkt.crc_type == PrimaryBlock.CrcType.CRC32Constructor assignment and post-construction assignment should produce identical internal values and identical bytes.
|
|
||
| def __getattr__(self, attr): | ||
| # type: (str) -> Any | ||
| return getattr(self.fld, attr) |
There was a problem hiding this comment.
2. [HIGH] Revert the ConditionalField.__getattr__() change in scapy/fields.py
The PR changes ConditionalField.__getattr__() to:
def __getattr__(self, attr):
try:
return getattr(self.fld, attr)
except AttributeError:
return super().__getattr__(attr)But ConditionalField inherits _FieldContainer, whose existing __getattr__() already performs:
return getattr(self.fld, attr)Consequently, the fallback just repeats the same lookup. ([GitHub][5])
This does not provide fallback access to CBORF_field. In the relevant multiple-inheritance hierarchy, CBORF_field appears before ConditionalField. A super() call made inside ConditionalField continues after ConditionalField in the method-resolution order; it cannot go backwards to CBORF_field.
The effective implementation is approximately:
try:
return getattr(self.fld, attr)
except AttributeError:
return getattr(self.fld, attr)It also potentially evaluates a wrapped property twice if that property raises AttributeError internally.
Recommended change
Revert the generic scapy/fields.py modification. The problem should be fixed in CBORF_CONDITIONAL, rather than changing Scapy’s established conditional field implementation.
| return self._field.i2repr(pkt, x) | ||
|
|
||
|
|
||
| class CBORF_CONDITIONAL(CBORF_field[Any, Any], fields.ConditionalField): |
There was a problem hiding this comment.
3. [BLOCKING] CBORF_CONDITIONAL does not apply its condition during serialization
CBORF_CONDITIONAL inherits in this order:
class CBORF_CONDITIONAL(CBORF_field, fields.ConditionalField):
...It explicitly checks the condition in m2i(), so the decoding path can omit a field. It does not override build(). Because CBORF_field is first in the MRO, CBORF_field.build() is used for serialization and never evaluates the condition. ([GitHub][3])
Traditional Scapy ConditionalField behavior is implemented around methods such as addfield(). The new CBOR field system bypasses that path and serializes fields through its own build() method. Multiple inheritance therefore does not automatically carry over conditional construction semantics.
The current primary-block test demonstrates the failure. It expects a leading byte of 0x8a, meaning a ten-element array, even though neither fragment flag nor CRC is enabled. BPv7 requires:
- 8 elements for a non-fragment without CRC;
- 9 elements for a non-fragment with CRC;
- 10 elements for a fragment without CRC;
- 11 elements for a fragment with CRC.
Fragment offset and total application data unit length are present only for fragmented bundles. The default test should therefore begin with 0x88, not 0x8a. ([GitHub][6])
The inheritance model also loses wrapped-field behavior. For example:
CBORF_field.any2i()wins over a wrapped enum or flags field’s specializedany2i().CBORF_field.build()wins over a wrapped array field’s specializedbuild().- Class attributes such as
islistorholds_packetscan shadow the wrapped field’s values before__getattr__()is invoked.
A conditional CBORF_ARRAY_OF is particularly problematic: inherited CBOR field serialization reaches the wrapped array field’s _encode(), which calls its packet-oriented build() with a Python list instead of a packet.
Recommended design
Use composition and explicit delegation. The wrapper must apply the condition on both construction and dissection:
class CBORF_CONDITIONAL(CBORF_field):
isconditional = True
def __init__(self, fld, cond):
self.fld = fld
self.cond = cond
def _evalcond(self, pkt):
return bool(self.cond(pkt))
def any2i(self, pkt, value):
return self.fld.any2i(pkt, value)
def i2h(self, pkt, value):
if pkt is not None and not self._evalcond(pkt):
return None
return self.fld.i2h(pkt, value)
def build(self, pkt):
if not self._evalcond(pkt):
return b""
return self.fld.build(pkt)
def dissect(self, pkt, data):
if not self._evalcond(pkt):
return data
return self.fld.dissect(pkt, data)The complete implementation should also delegate owner registration, representation, copying, islist, holds_packets, and any packet/list metadata expected by Scapy.
| # type: (CBOR_Packet) -> bytes | ||
| return b"".join(obj.build(pkt) for obj in self.seq) | ||
|
|
||
| class CBORF_ARRAY(CBORF_field[List[Any], List[Any]]): |
There was a problem hiding this comment.
4. [BLOCKING] Definite CBORF_ARRAY decoding ignores the encoded element count
CBORF_ARRAY.m2i() decodes the CBOR array head and obtains count, but for definite-length arrays it then iterates over every field in self.seq without using that count.
That creates two forms of parser desynchronization:
- If the wire array contains fewer elements than the schema, bytes belonging to a following structure may be consumed as fields of the current array.
- If the wire array contains more elements than the schema, valid members of the current array remain unconsumed and appear to belong to the outer structure.
Conditional fields make the problem more subtle because a schema field can consume zero wire elements. The parser needs to count actual elements consumed from the wire, not simply the number of field descriptors visited.
Recommended change
Maintain an explicit remaining-element counter for definite arrays. Each successful element decode decrements it. A conditionally absent field must not decrement it.
At the end:
remaining > 0should either decode a defined extension/tail field or raise a structural error.- Running out of fields while
remaining > 0should not silently leave bytes behind. - Running out of wire elements before required schema fields have been decoded should raise
CBOR_Decoding_Error.
| return CBOR_encode_head(int(CBOR_MajorTypes.ARRAY), len(parts)) + items | ||
|
|
||
|
|
||
| class CBORF_ARRAY_INDEFINITE(CBORF_ARRAY): |
There was a problem hiding this comment.
5. [HIGH] Indefinite CBORF_ARRAY handling can consume the break marker twice
During indefinite-array decoding, the loop calls _consume_break(..., required=False). When a break is found, that helper already consumes it. After the loop, the implementation calls _consume_break(..., required=True) again. ([GitHub][3])
An empty or short indefinite array therefore requires a second break marker. In a nested structure, the second call can consume the enclosing container’s break marker, desynchronizing the outer parser.
The normal BPv7 path may avoid this in some cases because CBORF_SEQUENCE_OF peeks at the break and leaves it for the outer array. The generic CBORF_ARRAY behavior remains incorrect.
Recommended change
Consume the terminating break exactly once. Track whether the loop exited because:
- all declared fields were processed;
- a break was found and consumed;
- the input ended;
- an element failed to decode.
Nested indefinite arrays and empty arrays need dedicated tests.
| return self.primary.check_crc() and all(blk.check_crc() for blk in self.blocks) | ||
|
|
||
|
|
||
| config.conf.debug_dissector = True |
There was a problem hiding this comment.
15. [HIGH] Importing bpv7.py changes global Scapy behavior and writes to stdout
The module sets:
config.conf.debug_dissector = Trueat import time, and one block-selection callback prints "new block" during dissection. ([GitHub][9])
Importing a contrib protocol must not globally enable dissector debugging for the whole Scapy process. Library dissection should also not write unsolicited text to stdout.
Recommended change
Remove both statements. Debugging should be enabled explicitly by the caller or locally through logging at an appropriate level.
|
|
||
| #@CanonicalBlock.register_type(11) | ||
| #@CanonicalBlock.register_type(12) | ||
| class AbstractSecurityBock(CBOR_Packet): |
There was a problem hiding this comment.
18. [MEDIUM] BPSec support is present but not actually integrated
BIB and BCB canonical block registrations are commented out, so canonical decoding does not select the new BPSec classes for their block types. The common class is also named AbstractSecurityBock, apparently missing the l in “Block.” ([GitHub][9])
The corresponding tests comment out substantive assertions and reserialization checks. As a result, the CBORF_ANY and homogeneous-container encoding failures are not exposed. ([GitHub][6])
Recommended change
Choose one clear scope for the PR:
- BPv7 core only: remove or keep the unfinished BPSec classes private until they are usable; or
- BPv7 plus BPSec: register BIB/BCB blocks and add real decode, construction, and reserialization tests.
Public class names should be corrected before merge because later renaming becomes an API compatibility issue.
|
|
||
|
|
||
| @enum.unique | ||
| class EidScheme(enum.IntEnum): |
There was a problem hiding this comment.
19. [MEDIUM] Two-element IPN endpoint IDs are not normalized according to RFC 9758
The implementation stores a two- or three-integer IPN service-specific part directly and renders those values as URI components. ([GitHub][9])
RFC 9758 defines the logical endpoint in terms of allocator, node, and service numbers. In the two-element CBOR form, allocator and node are packed into the fully qualified node number; the upper and lower 32-bit portions need to be separated. The three-element form carries them separately. ([RFC Editor][12])
Without normalization, a two-element representation with a nonzero allocator can render as a very large node number and will not compare equal to the equivalent three-element representation.
Recommended change
Normalize both encodings to one internal logical form:
allocator: int
node: int
service: intFor the two-element form:
allocator = fqnn >> 32
node = fqnn & 0xFFFFFFFFThen serialization can deliberately select either the packed or three-element wire representation.
Tests should prove equivalence between both encodings.
| from functools import lru_cache | ||
| from collections import defaultdict | ||
| import itertools | ||
| from typing import Set, List, Tuple, Any |
There was a problem hiding this comment.
20. [MEDIUM] scapy/libs/crc.py removes typing imports that are still referenced
The PR removes typing imports, but type comments and annotations in the file still reference names such as Any, Set, List, and Tuple. ([GitHub][13])
This may not affect normal runtime execution because several references are in type comments, but the names can no longer be resolved by static-analysis and type-checking tools.
| @@ -0,0 +1,283 @@ | |||
| % Bundle Protocol Version 7 test campaign | |||
There was a problem hiding this comment.
21. [MEDIUM] The tests do not exercise the critical construction paths
Several tests call .show(), print values, check only an outer marker, or verify parse–rebuild behavior. The BPSec assertions and byte round trips are commented out. One test writes to the fixed path /tmp/foo.pcap. ([GitHub][6])
This leaves the main representation mismatch untested:
- Parsed containers hold CBOR objects and often rebuild.
- Programmatically constructed containers hold Python values and serialize differently.
- Constructor assignment may work while later field assignment fails.
The fixed /tmp path is also nonportable and unsafe for parallel test execution.
Recommended change
Use Scapy’s temporary-file facilities or remove the PCAP write entirely. Tests should assert exact bytes, decoded field values, semantic validation, and expected failures.
|
Here is a uts file to verify the review findings. (I just needed to rename it to txt for github) |
|
I brian, I've started working on some fixes to support your progress. I let you know, once I think they are ready |
3 participants
Description
Add a BPv7 packet family (bundle, blocks, block-type-specific data) using the new CBOR packet and field modules, and eventually the CRC module.
Fixes #4874 eventually