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 <worley@xxxxxxxxxxx> wrote:
>
> 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.
Alvaro caught these too in his AD review so I’ve got them in the editor's copy I’m working with.
> 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.”
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.
> 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.
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.
> 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.
I think this was hold over from the CRMF request description; “and PublicKey” can be dropped.
> 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.
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
> 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/
fair enough ;)
> 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?
Yes
> 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].
It’s a tangled web the SIDR drafts have weaved :) The latter.
> 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.
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.
> (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}
You got it.
> 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.
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.
> 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.
Yep it’s a note for the para before the bullets so I fixed the ident.
> 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.
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.
> 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?)
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.
> 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.
The validation constraints are found in s3.3 we could put a forward pointer in here, but I think that might be overkill.
> 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/.
sigh - yep
> 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.
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
> 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”.
Alvaro had a similar comment on the IANA considerations and he suggested the first option.
> 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) }
This is great catch! And, yes the pkix(7) is missing.