dtn7 / dtn7-gold

Delay-tolerant networking software suite, Bundle Protocol Version 7 (deprecated)
https://dtn7.github.io/
Other
78 stars 14 forks source link

Number of elements in CBOR array for primary block can be 8 #34

Closed Marlinski closed 3 years ago

Marlinski commented 3 years ago

The current parser forbids primary block with 8 elements in the CBOR array:

https://github.com/dtn7/dtn7-go/blob/7f73e03069bf7a4194c15bedf5320441a94f44b9/pkg/bpv7/primary_block.go#L158

however the RFC authorize this in the case the primary block has no CRC and is not a fragment:

https://tools.ietf.org/html/draft-ietf-dtn-bpbis-31#page-23

oxzi commented 3 years ago

Thanks you for reporting this issue, @Marlinski!

You seem to be absolutely right about that. If I remember correctly, the CRC became mandatory at times and then again not. I seem to have overlooked the relevant passage in the diff.

I will take a look at the code and make some adjustments. Thanks!

oxzi commented 3 years ago
   CRC Type: CRC Type codes are discussed in Section 4.2.1. above.  The
   CRC Type code for the primary block MAY be zero if the bundle
   contains a BPsec [BPSEC] Block Integrity Block whose target is the
   primary block; otherwise the CRC Type code for the primary block
   MUST be non-zero.

   -- https://tools.ietf.org/html/draft-ietf-dtn-bpbis-31#page-24

Now I remembered why a CRC was still enforced for a primary block. There is still no complete BPsec implementation within dtn7-go.

The change was introduced in revision 15. Its predecessor, revision 14, started requiring a mandatory CRC. However, in revision 15 it was softened accordingly.

I just prepared a patch to address the issue, but now I am not quite sure how to proceed. On the one hand, it would be nice to allow primary blocks without a CRC - especially to be prepared if BPsec lands. On the other hand, this leads to a non-standard behavior. I think I am going to allow primary blocks without a CRC, but will set one for created bundles.

oxzi commented 3 years ago

@Marlinski: Please take a look at ae703f963d35666dd7fb186ad6835e8f66ddd687 which should fix this issue. Thanks again for reporting!

Marlinski commented 3 years ago

looks good to me!