Re: [sidr] WGLC for draft-ietf-sidr-bgpsec-protocol-05

[Sean Turner <[e-mail]>]

Here's my review:

One biggie:

1) MUST have an "IANA Considerations" section even if it says "none". 
But, I think you've Flags and Info Types to register right?

Algs are getting done elsewhere ;)

One not so biggie but a required formatting thing:

1) s9: If all references are normative, put "Normative" in the section 
title.

A bunch of nits mixed in with requests for additional clarifications 
(i.e., no biggies):

1) s1: r/must have an/needs an

2) s1: r/does not require/does not need

3) s2: r/[4]that/[4] that

4) s2, bits 2 and 3 as well as the fourth octet: normally this should 
would be a must any reason it's not a must?:

  These
  reserved bits should be set to zero by the sender and ignored by the
  receiver.

5) s2: contains the following:

  If there does not exist at least one version of BGPSEC that is
  supported by both peers in a BGP session, then the use of BGPSEC has
  not been negotiated.  (That is, in such a case, messages containing
  the BGPSEC_Path_Signatures MUST NOT be sent.)

Should this paragraph say something about the BGP NOTIFICATION message 
with the Error Code 2 (OPEN Message Error) and the OPEN Message Error 
SubCode 4 (Unsupported Optional Parameter) or is it the SubCode 7 
(Unsupported Capability) MUST be returned if both don't support bgpsec?

6) s2, Figure: If the AFI is 2 octets can we do the following:

    +---------------------------------------+
    |               AFI                     |
    +                                       +
    |                                       |
    +---------------------------------------+

so people won't ask what's in the blank line ;)

7) s2: Is there a reference for AFI that we can point to?

8) s2: ? r/speaker must include two/speaker includes two

9) s2: r/it has also advertises/it has also advertised

10) s2: r/(see RFC 4893)./[RFC 4893] - and add a reference.

11) s2: X2 (aligns with earlier sentences) r:/its open message/its BGP 
OPEN message/

12) s3: r/new optional/new OPTIONAL

13) High-Level Diagram of the BGPSEC_Path_Signatures Attribute
     BGPSEC_Path_Signatures figure: I might be reading way to much in to 
this but I don't want people to get the impression that signature X and 
Y for alg 1 and 2 are the same.  You might draw that conclusion from this:

  |     +-----------------+       +-----------------+       |
  |     | Sig Block 1     |       |  Sig Block 2    |       |
  |     +-----------------+       +-----------------+       |
  |     | Alg Suite 1     |       |  Alg Suite 2    |       |
  |     | SKI X           |       |  SKI X          |       |
  |     | Sig Length X    |       |  Sig Length X   |       |
  |     | Signature X     |       |  Signature X    |       |
  |     | SKI Length Y    |       |  SKI Length Y   |       |
  |     | SKI Y           |       |  SKI Y          |       |
  |     | Sig Length Y    |       |  Sig Length Y   |       |
  |     | Signature Y     |       |  Signature Y    |       |
  |     |      ...        |       |      ....       |       |
  |     +-----------------+       +-----------------+       |

Maybe in Sig Block 1 r/X/X1 and r/Y/Y1 and in Sig block 2 r/X/X2 and r/Y/Y2?

14) s3: If it does include the AS_PATH attribute what happens:

  That is, update messages that contain the
  BGPSEC_Path_Signatures attribute MUST NOT contain the AS_PATH
  attribute.

15) s3.1: Should probably add some ascii art for the flags:

+-+-+-+-+-+-+-+-+
|C|   Reserved  |
+-+-+-+-+-+-+-+-+

16) s3.1: r/These bits MUST be set to zero by the sender.
            /These bits MUST be set to zero by the sender and ignored
             by the recipient.

17) s3.1/3.2: Are the editor's notes going to get removed at some point 
or are we planning on keeping it?

18) s3.3: Probably worth adding the the SKI is copied from the signers 
certificate and need not be generated.  Or, is that implementation specific?

19) s4: In the last paragraph should the "must" be "MUST" x2.  It's in a 
"Note" paragraph so I could see not use MUST, but in that case maybe 
r/must possess/needs and r/must/needs to. If you use the MUST strike 
"Note" -  I don't think requirements should appear in Note paragraphs 
(though they often do).

20) s4.1: I'd just strike "Note that" from the those two paragraphs (x3).

21) s4.2: If we're going to keep NOT RECOMMENDED then you need to add it 
to the conventions.  Put it after RECOMMENDED so the nit checker won't 
barf on it.

    The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
    "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY",
    and "OPTIONAL" in this document are to be interpreted as described
    in RFC 2119 [8].

Actually, the RFC editor is going to move the requirements language to 
the mainbody so might as well do it now.  Just slap it in as s1.1.

22) s4.3: MUST instead of must:

   Members of autonomous system confederations [3] must additionally
   follow the instructions in this section for processing BGPSEC update
   messages.

23) s4.3: r/it must remove all
            /it MUST remove all ?
           r/confederation members must make
            /confederation members MUST make ?

24) s5.2: I think there a link missing to how you determine whether the 
certificate is valid in the following:

   To do this, consult the valid
   RPKI end-entity certificate data and look up all valid (AS, SKI,
   Public Key) triples in which the AS matches the AS number in the
   corresponding Secure_Path segment.

To me that means do the RPKI certificate checks in RFC 6487 as augmented 
by s3.3 of draft-ietf-sidr-bgpsec-pki-profiles.  Probably just adding a 
pointer to [11] would wfm.

   To do this, consult the valid
   RPKI end-entity certificate data and look up all valid (AS, SKI,
   Public Key) triples, see Section 3.3 of [11], in which the AS matches
   the AS number in the corresponding Secure_Path segment.

or something like that.

25) s6.1, 1st para: r/must/needs to X2

26) s6.1, 2nd para, 2nd sentence: I think the bit about the 'new' 
algorithm being specified in [12] should be deleted. It doesn't say 
anything about 'new' algorithm right now and the 3rd paragraph says if 
and when the new alg is chosen that draft will be updated.

27) s6.1: r/the future the mandatory/the future mandatory

That's it for now.

spt


On 9/15/12 7:45 AM, Murphy, Sandra wrote:
> This starts a working group last call for draft-ietf-sidr-bgpsec-protocol-05.  The draft is available at http://tools.ietf.org/html/draft-ietf-sidr-bgpsec-protocol-05 and https://datatracker.ietf.org/doc/draft-ietf-sidr-bgpsec-protocol/
>
> Please review this draft to see if you think it is ready for publication.  Send end comments to the list.
>
> The WGLC will end on 29 September 2012.
>
> --Sandy, speaking as wg co-chair
> _______________________________________________
> sidr mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/sidr
>