Subject: [Gen-art] Review of draft-ietf-sidr-bgpsec-pki-profiles-18

From: Dale Worley <[email-masked]>
Date: Tue, 13 Dec 2016 13:45:09 -0800

Reviewer: Dale Worley
Review result: Ready with Nits

I am the assigned Gen-ART reviewer for this draft.  The General Area
Review Team (Gen-ART) reviews all IETF documents being processed by
the IESG for the IETF Chair.  Please treat these comments just like
any other last call comments.

Document: draft-ietf-sidr-bgpsec-pki-profiles-18
Reviewer: Dale R. Worley
Review Date: 12 Dec 2016
IETF LC End Date: 19 Dec 2016
IESG Telechat date: unknown

Summary:

This draft is basically ready for publication, but has nits that
should be fixed before publication.

Some of these items appear to be technical, but I suspect that the
intended meanings are clear to people well-versed in PKI and are known
to work.  However, they are unclear to a naive reader.

2.  Describing Resources in Certificates

   The RIR, in turn, issues a CA certificate to an Internet Service
   Providers (ISP). 

s/Providers/Provider/
		    
   The CA also
   generate.  The CA also generates Certificate Revocation Lists
(CRLs).

The first "The CA also generate." is extraneous.

3.1  BGPsec Router Certificate Fields

   A BGPsec Router Certificate is a valid X.509 public key
certificate,
   consistent with the PKIX profile [RFC5280], containing the fields
   listed in this section.  This profile is also based on [RFC6487]
and
   only the differences between this profile and the profile in
   [RFC6487] are specified below.

I suspect this paragraph is going to cause implementers some trouble.
What, exactly, are the constraints on the BGPsec Router Certificate?

It looks like the certificate must conform to:

- X.509

- RFC 5280

- RFC 6487 as modified by "below"

and I see that RFC 6487 requires that certificates conform to RFC
5280.  So it seems that the first two items are directly required by
this document and transitively required by RFC 6487.

I suggest changing the first two items to be required only by
transitivity, only mentioning X.509 and RFC 5280 as explanatory:

   A BGPsec Router Certificate is consistent with the profile in
   [RFC6487] as modified by the specifications in this section.  As
   such, it must be a valid X.509 public key certificate and
   consistent with the PKIX profile [RFC5280].

Also, "below" is vague.  I suspect you mean "The differences between
this profile and the profile in [RFC6487] are specified in this
section."

3.1.1.1.  Subject 

   However, each
   certificate issued by an individual CA MUST contain a Subject name
   that is unique within that context.

What is "that context"?  Do you mean:

   However, the certificates issued by an individual CA MUST contain
   unique Subject names.

However that has difficulties when it comes time for the CA to issue
new certificates with later validity times.

Why there is a constraint based on "issued by an individual CA" is not
clear, given that there is no constraint regarding which CA issues
certificates to which routers.  Merely aggregating the work of
multiple CAs into one CA could require changing the subject names in
the next revision of issued certificates, whereas splitting the
work of one CA into multiple CAs would loosen this requirement.  In
addition, the definition of "an individual CA" is not clear.  Is there
really an operational requirement for this uniqueness constraint?

More to the point, is the combination of common name (i.e. AS number)
and serial number (router ID) required to be globally unique or not?
That seems to be the only question that can be operationally
significant.

I suspect that someone well-versed in PKI knows these answers, but for
the naive, what is required and why seems confusing.

3.2.  BGPsec Router Certificate Request Profile

    o The SubjectPublicKeyInfo and PublicKey fields are specified in
      [ID.sidr-bgpsec-algs].

There is no "PublicKey field" discussed in ID.sidr-bgpsec-algs.  Is
"subjectPublicKey" intended?  If so, "subjectPublicKey" seems to be a
sub-field of SubjectPublicKeyInfo (per ID.sidr-bgpsec-algs section
3.1), which is also listed here, so it is not clear why it is
mentioned individually here.

3.3.  BGPsec Router Certificate Validation 

   The validation procedure used for BGPsec Router Certificates is
   identical to the validation procedure described in Section 7 of
   [RFC6487] (and any RFC that updates this procedure), but using the
   constraints applied come from this specification.

I assume you mean "and any RFC that updates the procedure of
[RFC6487]".  In that case, I think that "that procedure" would be
required, but "the procedure of [RFC6487]" would eliminate any
ambiguity.

"but using the constraints applied come from this specification" is
unclear.

   step 3: "the certificate contains all the field that must be
present"

This doesn't match the text in RFC 6487, despite claiming to be
quoted:
s/the certificate/The certificate/
s/all the field/all fields/

    o BGPsec Router Certificate MUST include the "Subject Public Key
      Info" described in [ID.sidr-bgpsec-algs] as it updates [ID.sidr-
      rfc6485bis].

There is no "Subject Public Key Info" in ID.sidr-bgpsec-algs.  Is
"subjectPublicKeyInfo" intended?

The construction "[ID.sidr-bgpsec-algs] as it updates
[ID.sidr-rfc6485bis]" is awkward.  Is "[ID.sidr-rfc6485bis] as updated
by [ID.sidr-bgpsec-algs]" intended?  If the latter construction is
used, it is well-defined, though it means that the actual place to
look for the description of "subjectPublicKeyInfo" is in
[ID.sidr-bgpsec-algs].

Better, though, is to ask, What will this look like when the RFCs are
published?  Will [ID.sidr-bgpsec-algs] and [ID.sidr-rfc6485bis] be
separate RFCs?  If so, the desired format of "subjectPublicKeyInfo"
will be described in one or the other of the RFCs, but (it seems) you
could name just one of them.

(It seems to me that if you have one draft modifying another draft,
you should combine them into one draft, or move the modifying text
into the draft it modifies.)

   NOTE: The cryptographic algorithms used by BGPsec routers are found
   in [ID.sidr-bgpsec-algs].  Currently, the algorithms specified in
   [ID.sidr-bgpsec-algs] and [ID.sidr-rfc6485bis] are different. 
BGPsec
   RPs will need to support algorithms that are used to validate
BGPsec
   signatures as well as the algorithms that are needed to validate
   signatures on BGPsec certificates, RPKI CA certificates, and RPKI
   CRLs.

I assume that there are two dichotomies:

   [ID.sidr-bgpsec-algs] vs. [ID.sidr-rfc6485bis]

   {the algorithms that are used to validate BGPsec signatures} vs.
   {the algorithms that are needed to validate signatures on BGPsec
   certificates, RPKI CA certificates, and RPKI CRLs}

It would be easier on the reader if it was clear how these paired.
E.g.,

   NOTE: BGPsec RPs will need to support the algorithms in
   [ID.sidr-bgpsec-algs], which are used to validate BGPsec
   signatures, as well as the algorithms in [ID.sidr-rfc6485bis],
   which are needed to validate signatures on BGPsec certificates,
   RPKI CA certificates, and RPKI CRLs.

or vice-versa.

4.  Design Notes 

      Note that this behavior is similar to the CA including the AS
      Resource Identifier Delegation extension in issued BGPsec Router
      Certificates despite the fact it is not present in the request.

This text is indented as if it is a continuation of the immediately
preceding bullet point.  I can't tell for sure, but it seems to me
that the text is actually a continuation of the paragraph containing
the bulleted list, and so should be out-dented by two spaces from
where it is now.

6.  Security Considerations 

   A BGPsec Router Certificate will fail RPKI validation, as defined
in
   [RFC6487], because the algorithm suite is different.

Different from what?  Also, "algorithm suite" doesn't appear to be a
defined datum in certificates; at least, it's not mentioned anywhere
else in this document.

   Consequently, a
   RP needs to identify the EKU to determine the appropriate
Validation
   constraint.

I *think* this means that a RP needs to examine the EKU value to
determine the algorithms that are used for [something].  (What does it
mean to "identify" the EKU?)

Also, this paragraph discusses "the certificate will fail validation
as defined in 6487", and then talks about "the appropriate validation
constraint", but it doesn't state that "the appropriate validation
constraint" modifies the process in 6487.  I suspect that the EKU
value determines an algorithm suite (based on some unstated
correspondence), which is then used to replace the algorithm suite
demanded by some part of the 6487 process, and then after that, the
certificate will pass the modified process.  But the text doesn't
assert that the certificate has to pass the modified process.

I suspect that the intended meaning of this paragraph is obvious to
anyone well-versed in PKI, but I don't think the words actually say
that meaning.

   Hash functions [ID.sidr-bgpsec-algs] are used when generating the
two
   key identifiers extension included in BGPsec certificates.

I suspect s/key identifiers extension/key identifier extensions/.

   However
   as noted in [RFC6818], collision resistance is not a required
   property of one-way hash functions when used to generate key
   identifiers.  Regardless, hash collisions are possible and if
   detected an operator should be alerted.

The fact that "an operator should be alerted" suggests that if a hash
collision happens it will cause an operational problem of some sort.
What that problem is should be described and some bound stated for the
amount of ensuing trouble.  Conversely, if no operational problem can
arise, then there is no reason to alert an operator.

7.  IANA Considerations

   No IANA allocations are request of IANA, ...

This should be "No IANA allocations are requested of IANA", or
probably better "No allocations are requested of IANA".

9.2.  Informative References 

Appendix A.  ASN.1 Module

     id-kp  OBJECT IDENTIFIER  ::= {
       iso(1) identified-organization(3) dod(6) internet(1)
       security(5) mechanisms(5) kp(3) }

Is this correct?  I believe this should be

     id-kp  OBJECT IDENTIFIER  ::= {
       iso(1) identified-organization(3) dod(6) internet(1)
       security(5) mechanisms(5) pkix(7) kp(3) }

RFC 3029 has

   id-kp-dvcs  OBJECT IDENTIFIER ::= {iso(1)
identified-organization(3)
        dod(6) internet(1) security(5) mechanisms(5) pkix(7) kp(3) 10}

RFC 6494 has

      send-kp OBJECT IDENTIFIER ::=
        { iso(1) identified-organization(3) dod(6) internet(1)
          security(5) mechanisms(5) pkix(7) kp(3) }

The IANA registry "smi-numbers" shows 1.3.6.1.5.5.7.3 as "Extended key
purpose identifiers" with ...10 as "id-kp-dvcs".  The IANA registry
shows 1.3.6.1.5.5.3 as "PEM-Based IDUP Mechanism".

Dale

---

From: Sean Turner <[email-masked]>
Date: Thu, 22 Dec 2016 16:31:33 -0500

Dale,

Thanks for the review.  Responses inline.  And, assuming Steve agrees I’ll submit a version that incorporates these and other changes before the IESG does its eval.

spt

> On Dec 13, 2016, at 16:45, Dale Worley <[email-masked]> wrote:
> 
> Reviewer: Dale Worley

Alvaro caught these too in his AD review so I’ve got them in the editor's copy I’m working with.

> section.”

It’s basically profile of a profile of a profile plus some stuff.  I’m happy to adopt your suggestion modulo s/must be/is.

> the naive, what is required and why seems confusing.

I think this is just a case of a missing “CA” in front of the word “context” so tweaking it to: “…. that is unique to that CA context”.  The certs only need to be unique on a per CA basis the subject name does not need to be unique across the whole of the RPKI.  The combination of issuer+subject+serial # plus all the parent certs provides the uniqueness.

> mentioned individually here.

I think this was hold over from the CRMF request description; “and PublicKey” can be dropped.

> "but using the constraints applied come from this specification" is

Sure - how about:

     The validation procedure used for BGPsec Router Certificates is
     identical to the validation procedure described in Section 7 of
     [RFC6487] (and any RFC that updates that procedure), but using the
     constraints applied come from this specification

> s/all the field/all fields/

fair enough ;)

> "subjectPublicKeyInfo" intended?

Yes

> look for the description of "subjectPublicKeyInfo" is in

It’s a tangled web the SIDR drafts have weaved :) The latter.

> could name just one of them.

They are separate as ID.sidr-rfc6485bis is now RFC 7935.  I think I agree we should just refer to the bgpsec-pki-algs draft - it’s clearer.

>   certificates, RPKI CA certificates, and RPKI CRLs}

You got it.

>   RPKI CA certificates, and RPKI CRLs.

It’s funny we say the RPKI and BGPsec algs are different at least twice.  I’m happy to change the wording here to make this paragraph easier to digest.

> the bulleted list, and so should be out-dented by two spaces from

Yep it’s a note for the para before the bullets so I fixed the ident.

> defined datum in certificates; at least, it's not mentioned anywhere

An algorithm suite refers to the signature and hashing algorithm used.  We could make it more explicit and change it to: … because the cryptographic algorithms used to are different.

> mean to "identify" the EKU?)

The RP needs to find it in the cert and then know that the value means do this validation routine not the other one.  I think identify works here because I’d say all of the previous sentence is associating, which is a meaning of the word identify.

> anyone well-versed in PKI, but I don't think the words actually say

The validation constraints are found in s3.3 we could put a forward pointer in here, but I think that might be overkill.

> I suspect s/key identifiers extension/key identifier extensions/.

sigh - yep

> arise, then there is no reason to alert an operator.

Key identifiers are used to build certification paths which is then used to validate the signature. i.e., build up validate down.  As an attacker, I can insert a certificate with a collision that might get picked up and used when validating the BGPsec signatures; it’ll obviously fail because the math won’t work so this is really a DoS.  How about something like the following:

  Regardless, hash collisions are unlikely, but they are possible
  and if detected an operator should be alerted.  A subject key
  identifier collision might cause the incorrect certificate to be
  selected from the cache, resulting in a failed signature validation

> probably better "No allocations are requested of IANA”.

Alvaro had a similar comment on the IANA considerations and he suggested the first option.

>       security(5) mechanisms(5) pkix(7) kp(3) }

This is great catch! And, yes the pkix(7) is missing.