Subject: [dtn] AD review of draft-ietf-dtn-bpsec-default-sc-02

From: Zaheduzzaman Sarker <[email-masked]>
Date: Mon, 22 Mar 2021 20:42:20 +0000

Hi,

Thanks for the work done to specify the default security context for BPsec. As I understand this specification is important for interoperability of BPsec.

I found this document easy to read and carefully written. However, I have found number of cases those need clarifications and fix nits. Please see below –

* Abstract :
    The BPsec specification says -

  	"To ensure interoperability among various implementations, all BPSec
  	 implementations MUST support at least the current IETF standards-
   	track mandatory security context(s).  As of this writing, that BCP
   	mandatory security context is specified in
    	[I-D.ietf-dtn-bpsec-default-sc], but the mandatory security
    	context(s) might change over time in accordance with usual IETF
    	processes.  Such changes are likely to occur in the future if/when
   	flaws are discovered in the applicable cryptographic algorithms, for
    	example.".

   This makes the default SC is a MUST to implement for interoperability. However, the abstract writes

   	"These security contexts may be used for
   	both testing the interoperability of BPSec implementations and for
   	providing basic security operations when no other security contexts
   	are defined or otherwise required for a network. "

    I would suggest to replace "may be" with "intended to be".

* Terminology :
     Section 4.6 : please check in normative text are correctly following RFC8174.
     Section 6.1 : please check in normative text are correctly following RFC8174.

* It would be good for the user of this document to capture the reason behind selecting HMAC-SHA and AES-GCM in respective overview section (section 3.1 and section 4.1).

* Section 3.1 :
	These variants correspond
	to HMAC256-SHA256, HMAC384-SHA384, and HMAC512-SHA512 as defined in
	[RFC8152] Table 7: HMAC Algorithm Values.

      Names does not match table names in RFC8152. I understand it might be desirable to use the longer form that makes it clear that this is SHA256/384/512 version of HMAC. However, in that case I think an explicit mapping needs to occur in this document.

      Again the name does not match how it is written in section 3.1 and 3.3.1.

* Section 3.3.2 and Section 3.5 :
    I foresee some issues here about key management i.e., picking up the correct key. The assumption here is that the key management will able to figure out the correct key to be used. However, in many key management functions (BPsec does not specify any) one will derive specific keys, use them and replace them later. This means there are chances that different keys will be available over time in the nodes. How do you identify the correct key in such a scenario, especially when the key is derived from the material carried in the BIB?   It seems like some sort of key-id might be required to resolve this. The BPsec does not define any generic key-id and it is neither defined here. It might become an issue for interoperability.

* Section 3.3.3:
   Two aspects here -
    1. Why are so many bits used for flags? I tried to back track it to BPsec and BP documents but I am not sure I found what I was looking for.
    2. if the plan here is to assign in future (bits 8-15) then an procedure need to be in place to assign them (even in an abstract form). The future assignment will also impact 3.3.6 and 3.7 as the unknow bits will lead to failure as the flag value is part of the IPPT and receiver does not know what to add based on the bits. What is the thinking here?

* Section 3.3.4:
    I would suggest to add a reference to BPsec 3.1 here.
    Nit: ID 1 should have default value 6, or if the intention here is to use SHA256 then it should be 5 instead (as mentioned in the early SECART review).

* Section 3.6 :
     Is there any particular reason for defining the flag bits in the order  - primary block flag, target header flag and security header flag but for canonical form process the order is primary block flag, security header flag and target header flag?

    Nit: in the process description text "flag" is written in both uppercase and lower case.

* Section 3.7.1 :
	Any local flags used to generate the IPPT SHOULD be placed in the
     	 Integrity Scope flags security parameter for the BIB unless these
     	 flags are expected to be correctly configured at security
      	verifiers and acceptors in the network.

    I assume that security acceptor and verifiers will have to use the actual value provided in the Integrity scope flags unless they are correctly configured. This sounds like the local flags used to generate the IPPT MUST be placed in the Integrity Scope flags. Is there any reason why this is SHOULD not a MUST?

* Section 3.7.2 :
      This section should also say any error on verification should result in failure. Something similar to the text in the section 4.8.2

* Section 4 :
     Several of above mentioned issues are also applicable to subsections of section 4.

* Section 4.3.2 :

	When not provided, implementations SHOULD assume a value of 3
   	(indicating use of A256GCM), unless an alternate default is
   	established by security policy at the security source, verifier, or
   	acceptor of this integrity service.

     Does this "security policy" refers to local security policy?

* Section 5:
      As mentioned before registries for Scope flags will be necessary.

* Section 6.3

     To me this security considerations for fragmentation belongs to BPsec as the main technical issues on fragmentation are described there.

I would also like to draw attention to the SECART review (Thanks to the SECART team and Christian Huitema) proposing test vectors to mitigate interoperability issue pointed out by the reviewer. This seems like a good proposal to include in this specification.

Many thanks!!

Zahed

---

From: "Birrane, Edward J." <[email-masked]>
Date: Mon, 29 Mar 2021 03:42:52 +0000

Zahed,

  Thank you for this detailed review!

  I have included my responses in-line below and have enumerated your comments using the convention C# for clarity.

  I have also posted a -03 which should address comments below.

>     I would suggest to replace "may be" with "intended to be".

C1: Agreed. This change was made in the -03 version of the document.

>      Section 6.1 : please check in normative text are correctly following

C2: Agreed. I have looked through these sections and made adjustments to the wording in the -03 version.

> * It would be good for the user of this document to capture the reason
> behind selecting HMAC-SHA and AES-GCM in respective overview section
> (section 3.1 and section 4.1).

C3: Agreed. I have added reasons for selection in 3.1 and 4.1 in the -03 version.

> * Section 3.1 :
>       Names does not match table names in RFC8152. I understand it might be
> desirable to use the longer form that makes it clear that this is
> SHA256/384/512 version of HMAC. However, in that case I think an explicit
> mapping needs to occur in this document.
> 

C4: Agreed. I have changed the names in the -03 version of the document to correspond to the names used in RFC8152.

> It seems like some sort of key-id might be required to resolve
> this. The BPsec does not define any generic key-id and it is neither defined
> here. It might become an issue for interoperability.

C5: Agreed this might become an issue for interoperability.  For example, if the policies for requiring security operations differ on different nodes there may be interop problems. Similarly, if the policies for key generation/communication differ on different nodes, there may be interop problems. The DTNWG consensus was this limitation is appropriate for now for this default security context and the DTN WG is looking to determine delay-tolerant key management strategies in the future. 

> * Section 3.3.3:
>    Two aspects here -
>     1. Why are so many bits used for flags? I tried to back track it to BPsec and
> BP documents but I am not sure I found what I was looking for.

C6: Agreed. We do not need this many options. It has been reduced in the -03 version of the document.

>     2. if the plan here is to assign in future (bits 8-15) then an procedure need
> to be in place to assign them (even in an abstract form). The future
> assignment will also impact 3.3.6 and 3.7 as the unknow bits will lead to
> failure as the flag value is part of the IPPT and receiver does not know what
> to add based on the bits. What is the thinking here?

C7: Agreed and do not mean to imply that the default security contexts should require assigned fields. This has been addressed in the -03 version of the document.

>     I would suggest to add a reference to BPsec 3.1 here.

C8: BPSec 3.1 describes the definition of the BIB and BCB block, but I think we can assume the reader understand these definitions.

>     Nit: ID 1 should have default value 6, or if the intention here is to use
> SHA256 then it should be 5 instead (as mentioned in the early SECART
> review).

C9: Agreed. I have adjusted this value in the -03 version of the document.

> * Section 3.6 :
>      Is there any particular reason for defining the flag bits in the order  -
> primary block flag, target header flag and security header flag but for
> canonical form process the order is primary block flag, security header flag
> and target header flag?

C10: No reason  and agree this is confusing.  I have addressed this in the -03 version of the document.

>     Nit: in the process description text "flag" is written in both uppercase and

C11: Agree that the capitalization is nonuniform for several items. I have made a pass through the -03 version of the document to try and normalize the use of capitalization.

>     I assume that security acceptor and verifiers will have to use the actual
> value provided in the Integrity scope flags unless they are correctly
> configured. This sounds like the local flags used to generate the IPPT MUST
> be placed in the Integrity Scope flags. Is there any reason why this is SHOULD
> not a MUST?

C12: The thinking is that if default flags are used, then there is no reason to include the flag regardless of the configuration of a verifier or an acceptor.

> * Section 3.7.2 :
>       This section should also say any error on verification should result in
> failure. Something similar to the text in the section 4.8.2

C13: I have updated the language in the -03 version to try and address this comment.

>      Several of above mentioned issues are also applicable to subsections of

C14: Agreed. Comments from section 3.* that also relate to section 4.* have been addressed in the -03 version of the document.

>      Does this "security policy" refers to local security policy?

C15: Yes.  Security policy at the source, verifier, or acceptor is meant to imply the local security policy at the node.

>       As mentioned before registries for Scope flags will be necessary.

C16: Similar to C7, there is no intent to define such a registry and the -03 version of the document corrects this.

> * Section 6.3
> 
>      To me this security considerations for fragmentation belongs to BPsec as
> the main technical issues on fragmentation are described there.

C17: I would support removing the fragmentation section from this document and including it in some other broader-scoped document on DTN security, though perhaps not the BPSec document itself because the issues with fragmentation can extend beyond the insertion and handling of security blocks. 

> I would also like to draw attention to the SECART review (Thanks to the
> SECART team and Christian Huitema) proposing test vectors to mitigate
> interoperability issue pointed out by the reviewer. This seems like a good
> proposal to include in this specification.

C18: Agreed and grateful for the review! As mentioned in my comments to the SECART review and similar to C17 above, I think that example encodings are a very useful concept, but would ultimately be useful at showing bundle structure and security block structure. Rather than repeating such encodings in all future security context documents, I propose the DTN WG consider the best way to produce this broader-scope documentation.

---

From: Zaheduzzaman Sarker <[email-masked]>
Date: Wed, 31 Mar 2021 07:06:51 +0000

Hi Edward,

Thanks for the update. The -03 version addresses most of the comments and concerns. 

See my further comments inline below with [ZS]. 

BR
Zahed

[ZS] Thanks, this is a good addition.

In section 4.1 :

The BCB-AES-GCM security context shall have the security context	
	   identifier specified in Section 5.1.

is this "shall" should be a "MUST" as described in Section 3.1?

[ZS] Ok,  is this consensus documented somewhere? Meeting notes or email thread or whatever?

[ZS] Ok, then clearly stating that helps. It was clearly mention in the other place(s).

[ZS] I don't think removing the fragmentation section from this document to some other document actually helps. This is a very important aspect of the security context and I think it is better to be decisive about it now. If moved to BPSec then other documents , specially security context documentation, can just refer to it. The question really is, where does it make more sense put there it is in line with the context of the content of the document? Right now I see that is BPsec. 

[ZS] I see the test vectors related to the applicability of the this document hence this need to be considered well. Failing to assure interoperability actually undermine the intention of this document, hence any tools to help achieving that goal is worth putting effort into. Also we need to make sure the test vector is actually usable by more than one implementation.  I would be happy if DTN WG comes up with a best way forward here.