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

[Rob Austein <[e-mail]>]

Another review of draft-ietf-sidr-bgpsec-protocol-05.

>  1.  Introduction
...
>     BGPSEC relies on the Resource Public Key Infrastructure (RPKI)
>     certificates that attest to the allocation of AS number and IP
>     address resources.  (For more information on the RPKI, see [6] and
>     the documents referenced therein.)  Any BGPSEC speaker who wishes to
>     send BGP update messages to external peers (eBGP) containing the
>     BGPSEC_Path_Signatures must have an RPKI end-entity certificate (as
>     well as the associated private signing key) corresponding to the
>     BGPSEC speaker's AS number.  Note, however, that a BGPSEC speaker
>     does not require such a certificate in order to validate update
>     messages containing the BGPSEC_Path_Signatures attribute.

The BGPSEC speaker doesn't need the certificate, only the private key.
The certificate needs to exist in the RPKI, but unless I'm missing
something the BGPSEC speaker need never see its own certificate.

>  2.  BGPSEC Negotiation
...
>                               Capability Value:
>  
>                      0        1        2  3       4 5 6 7
>                   +---------------------------------------+
>                   | Send | Receive | Reserved  |  Version |
>                   +---------------------------------------+
>                   |               AFI                     |
>                   +---------------------------------------+
>                   |                                       |
>                   +---------------------------------------+
>                   |             Reserved                  |
>                   +---------------------------------------+
>                   |               SAFI                    |
>                   +---------------------------------------+
...
>     The fifth octet in the capability contains the 8-bit Subsequent
>     Address Family Identifier (SAFI).  This value is encoded as in the
>     BGP multiprotocol extensions [2].

Is zero SAFI the same thing as null SAFI?  It's not in RFC 3779: an
OCTET STRING of length two is not the same as an OCTET STRING of
length three with the third octet zeroed.

I suspect that presence of SAFI may require a flag bit, unless we are
certain that zero SAFI always means the same thing as null SAFI.

>  3.  The BGPSEC_Path_Signatures Attribute
...
>  
>          High-Level Diagram of the BGPSEC_Path_Signatures Attribute
>                            BGPSEC_Path_Signatures

What's the doubled attribute name supposed to mean?

>          +---------------------------------------------------------+
>          |     +-----------------+                                 |
>          |     |   Secure Path   |        +-----------------+      |
>          |     +-----------------+        | Additional Info |      |
>          |     |    AS X         |        +-----------------+      |
>          |     |    pCount X     |        |  Info Type      |      |
>          |     |    Flags X      |        |  Info Length    |      |
>          |     |    AS Y         |        |  Info Value     |      |
>          |     |    pCount Y     |        +-----------------+      |
>          |     |    Flags Y      |                                 |
>          |     |      ...        |                                 |
>          |     +-----------------+                                 |
>          |                                                         |
>          |     +-----------------+       +-----------------+       |
>          |     | 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    |       |
>          |     |      ...        |       |      ....       |       |
>          |     +-----------------+       +-----------------+       |
>          |                                                         |
>          +---------------------------------------------------------+

I did not find this picture very clear and I thought I knew this
protocol.  Generally, in diagrams of this type, one does not assume
row major ordering of internal boxes.

>     The following is a more detailed explanation of the format of the
>     BGPSEC_Path_Signatures attribute.
>
>                       BGPSEC_Path_Signatures Attribute
>  
>           +-------------------------------------------------------+
>           | Secure_Path                             (variable)    |
>           +-------------------------------------------------------+
>           | Additional_Info                         (variable)    |
>           +-------------------------------------------------------+
>           | Sequence of one or two Signature_Blocks (variable)    |
>           +-------------------------------------------------------+

It's not more detailed, but it is clearer, because at least it's
obvious how the sections match up with the following text.

>  3.2.  Additional_Info
>  
>     Here we provide a detailed description of the Additional_Info in the
>     BGPSEC_Path_Signatures attribute.
>  
>                                Additional_Info
>  
>                +---------------------------------------------+
>                | Info Type                      (1 octet)    |
>                +---------------------------------------------+
>                | Info Length                    (1 octet)    |
>                +---------------------------------------------+
>                | Info Value                     (variable)   |
>                +---------------------------------------------+
>  
>  
>     The Info Type field is a one-octet value that identifies the type of
>     additional information included in the Info Value field.  This
>     specification defines a single (null) type of Additional_Info.  The
>     Info Type for this null type is zero.
>  
>     The Info Length field contains the length in octets of the Info Value
>     field.  For the (null) Info Type zero specified in this document, the
>     Info Length MUST be zero.
>  
>     The syntax and semantics contained in the Info Value field depends on
>     the type contained in the Info Type field.  For the (null) Info Type
>     zero specified in this document, the Info Value field is empty (since
>     the Info Length field must be zero).
>  
>     Implementations compliant with this specification MUST set the Info
>     Type to zero in BGPSEC update messages for route advertisements that
>     they originate (see Section 4.1 for more details).  When an
>     implementation compliant with this specification receives a BGPSEC
>     update message with an Info Type field that it does not understand
>     (i.e., an Info Type other than zero), the implementation MUST use the
>     Additional_Info when it verifies digital signatures (as per Section
>     5.2).  However, other than signature verification, the implementation
>     MUST ignore the Info Value field when it does not understand the Info
>     Type.

So this is an expansion mechanism, fine, got that.  How many of these
appear in a message?  Looks like exactly one.  Not very flexible.  We
need to include this instead of just going to a new attribute type
because?

Overall, this extension format looks like it wants to be IPv4 header
options when it grows up, but the rest of the formats doom this to
being a singleton.  Kind of strange.

I don't really object, it's just more than a little odd as a
placeholder for we-have-no-real-idea-what.

>  4.1.  Originating a New BGPSEC Update
...
>     In particular, this AS number MUST match the AS number in
>     the AS number resource extension field of the Resource PKI end-entity
>     certificate(s) that will be used to verify the digital signature(s)
>     constructed by this BGPSEC speaker.

"The" or "an"?   Is it legal for the EE certificate to cover more RFC
3779 resources than just a single ASN?

>  4.2.  Propagating a Route Advertisement
...
>     The BGPSEC speaker next copies the Additional_Info portion of the
>     BGPSEC_Path_Signatures directly from the received update message to
>     the new update message (that it is constructing).  Note that the
>     BGPSEC speaker MUST NOT change the Additional_Info as any change to
>     Additional_Info will cause the new BGPSEC update message to fail
>     validation (see Section 5).

So Additional_Info is generated only by the originator.  It would have
been kind to mention this earlier in the document.

>     If the received BGPSEC update message contains two Signature_ Blocks
>     and the BGPSEC speaker supports both of the corresponding algorithms
>     suites, then the new update message generated by the BGPSEC speaker
>     SHOULD include both of the Signature_Blocks.  If the received BGPSEC
>     update message contains two Signature_Blocks and the BGPSEC speaker
>     only supports one of the two corresponding algorithm suites, then the
>     BGPSEC speaker MUST remove the Signature_Block corresponding to the
>     algorithm suite that it does not understand.  If the BGPSEC speaker
>     does not support the algorithm suites in any of the Signature_Blocks
>     contained in the received update message, then the BGPSEC speaker
>     MUST NOT propagate the route advertisement with the
>     BGPSEC_Path_Signatures attribute (i.e., propagate it as an unsigned
>     BGP update message).

Parenthetical phrase says opposite of what I suspect it was intended
to mean.  I suspect you meant something like:

"(i.e., if it chooses to propagate this route advertisement at all, it
MUST do so as an unsigned BGP update message)".

>  4.3.  Processing Instructions for Confederation Members
...
>     When a confederation member receives a BGPSEC update message from a
>     peer within the confederation and propagates it to a peer outside the
>     confederation, it must remove all of the Secure_Path Segments added
>     by confederation members as well as the corresponding Signature
>     Segments.  To do this, the confederation member propagating the route
>     outside the confederation does the following:
>  
>     o  First, starting with the least recently added Secure_Path
>        segments, remove all of the consecutive Secure_Path segments that
>        have the Confed_Segment flag set to one.  Stop this process once a
>        Scure_Path segment is reached which has its Confed_Segment flag
>        set to zero.  Keep a count of the number of segments removed in
>        this fashion.
>  
>     o  Second, starting with the most recently added Signature Segment,
>        remove a number of Signature Segments equal to the number of
>        Secure_Path Segments removed in the previous step.  (That is,
>        remove the K most recently added signature segments, where K is
>        the number of Secure_Path Segments removed in the previous step.)

Is "least recently added" in first bullet item a mistake?  Sure looks
like one.  If not, this seriously violates the Principal of Least
Astonishment, so the text needs a coherent explanation of why we
remove the least recent Secure_Path segments and the most recent
Signature Segments.

More generally, it would be nice if the document were clearer on the
circumstances under which there would ever be two or more blocks of
segments with the Confed_Segment flag turned on.  My guess is that
this should never happen, but, if so, the above description is an
awfully compled way of saying "strip out every Secure_Path segment
with the Confed_segment flag turned on, along with the corresponding
signature_segments."

>  5.  Processing a Received BGPSEC Update
...
>     Implementations that support such deferment of validation MUST
>     perform validation of these messages as soon as possible (i.e., as
>     soon as resources are available to perform validation) and MUST re-
>     run best path selection once the validation status of such update
>     messages is known.

Is it OK to do this incrementally if the implementation so desires, or
must the implementation only do this after catching up completely?  If
the latter, what should the implementation do if the flood continues
and the implementation doubts it will ever catch up?

>  5.2.  Validation Algorithm
>  
>     This section specifies an algorithm for validation of BGPSEC update
>     messages.  A conformant implementation MUST include a BGPSEC update
>     validation algorithm that is functionally equivalent to the external
>     behavior of this algorithm.

s/external/externally visible/

>     If there are two Signature_Blocks within the BGPSEC_Path_Signatures
>     attribute and one of them is poorly formed (or contains the wrong
>     number of Signature segments) , then the recipient should log that an
>     error occurred, strip off that particular Signature_Block and process
>     the update message as though it arrived with a single
>     Signature_Block.  If the BGPSEC_Path_Signatures attribute contains an
>     error that is not local to one of two Signature_Blocks, then the
>     recipient should log that an error occurred and drop the update
>     message containing the error.  (In particular, if any of checks 3-5
>     above fail, the recipient should log that an error occurred and drop
>     the update message containing the error.)

Do routers "log that an error occurred"?  Does incrementing a MIB
variable count?  What's the expected load from this logging?  What bad
thing (if any) happens if this logging is dropped or ignored?

>        signature is invalid, then mark the entire Signature-List Block as
>        'Not Good' and proceed to the next Signature_Block.  If the
>        signature validation algorithm determines that the signature is
>        valid, then continue processing Signature-Segments (within the
>        current Signature-List Block).

What's a "Signature-List Block"?  Never defined.

>     If all Signature-Segments within a Signature-List Block pass
>     validation (i.e., all segments are processed and the Signature-List
>     Block has not yet been marked 'Not Good'), then the Signature_Block
>     is marked as 'Good'.

What's a "Signature-Segment"?  Never defined.

>     If at least one Signature_Block is marked as 'Good', then the
>     validation algorithm terminates and the BGPSEC update message is
>     deemed to be 'Good'.  (That is, if a BGPSEC update message contains
>     two Signature_Blocks then the update message is deemed 'Good' if the
>     first Signature_Block is marked 'Good' OR the second Signature_Block
>     is marked 'Good'.)

I think this is saying that the validation algorithm requires that
there exist a complete valid chain for some single algorithm, ie, that
one cannot construct a valid chain by hopping between algorithms in
the middle of the chain.  This makes sense, since otherwise the
signature chaining won't work.  Might want to say so.

>  6.1.  Algorithm Suite Considerations
...
>     To this end, a mandatory algorithm suites document will be created
>     which specifies a mandatory-to-use 'current' algorithm suite for use
>     by all BGPSEC speakers [12].  Additionally, the document specifies an
>     additional 'new' algorithm suite that is recommended to implement.

Badly phrased, unless the real intent here is to say that we're going
to pick both the current and next algorithms right off the bat, which
seems unlikely to me.   I think it would be more correct to say that
we will specify an initial mandatory algorithm suite, and, once we
have some idea of what the next algorithm should be, we will publish
a series of updated documents phasing in the new one and (eventually,
years later) phasing out the old one.

>     It is anticipated

By whom?
                        
>     Once the transition has successfully been completed in this manner,
>     BGPSEC speakers SHOULD include only a single Signature_Block
>     (corresponding to the 'new' algorithm).

So we're going to start out with only one, right?

>  6.2.  Extensibility Considerations
...
>     At this point a transition would begin which is analogous to the
>     algorithm transition discussed in Section 6.2.

Specifying section numbers manually instead of using <xref/>, are we?

>  7.  Security Considerations
...
>     (It should be noted that BGPSEC does not offer a precise
>     guarantee that the data packets would propagate along the
>     indicated path; it only guarantees that the BGP update conveying
>     the path indeed propagated along the indicated path.)

"Precise guarantee", hell.  It provides no guarantee of any kind on
this point.  AFAIK nobody knows how to do that (yet).