Re: [Acme] Benjamin Kaduk's Discuss on draft-ietf-acme-authority-token-tnauthlist-08: (with DISCUSS and COMMENT)

[Chris Wendt <[e-mail]>]

Hi Ben,

Thanks for the review, and apologize to you and everyone for the delay in getting this done.  I have submitted draft -09 and have responded to your discuss points inline:

> On Nov 27, 2021, at 5:41 PM, Benjamin Kaduk via Datatracker <[e-mail]> wrote:
> 
> Benjamin Kaduk has entered the following ballot position for
> draft-ietf-acme-authority-token-tnauthlist-08: Discuss
> 
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
> 
> 
> Please refer to https://www.ietf.org/blog/handling-iesg-ballot-positions/
> for more information about how to handle DISCUSS and COMMENT positions.
> 
> 
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-acme-authority-token-tnauthlist/
> 
> 
> 
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
> 
> (0) As written, the validation procedures for the authority token
> contain a gaping security hole.  In particular, §6 has us use the public
> key of the certificate referenced by the token's "x5u" parameter,
> without checking that that "x5u" value (or the certificate it
> references) is a trusted issuer of authority tokens.  This in essence
> boils down to "go fetch a certificate from an attacker provided location
> and verify that the signature over the attacker-provided token was made
> by that attacker-provided certificate".  This is trivial for an attacker
> to achieve and provides no security value.  We need to know that the
> token issuer is trusted and authorized to issue this class of token.
> The companion document draft-ietf-acme-authority-token does describe the
> need for this mutual trust relationship, but it is negligent for us to
> provide a step-by-step procedure here that omits this step.

Your point about validation that the x5u is from a trusted issuer of authority tokens is key and i added an explicit step in the process to clarify this.

> 
> (1) Related to my discuss on draft-ietf-acme-authority-token, we should
> be clear on which document is the authoritative specification for
> "token-authority" usage; at present the description seems to be split
> across the two documents.

Yes, i added more explicit description in the introduction of this as a profile document of the Authority Token specification specific to TNAuthList

> 
> (2) Section 3
> 
>   The format of the string that represents the TNAuthList MUST be
>   constructed as a base64 [RFC4648] encoding of the TN Authorization
>   List certificate extension ASN.1 object.  The TN Authorization List
>   certificate extension ASN.1 syntax is defined in [RFC8226] section 9.
> 
> Does it need to be the (base64 encoding of the) DER encoding of the
> ASN.1 object?  Or do we allow less stringent ASN.1 encoding rules?
> (Similarly in §5.4.)

Yes, i added DER encoding as the the required format

> 
> (3) I think my discuss point on draft-ietf-acme-authority-token about
> how the issuer is identified will also apply (with slight modification)
> to this document -- in §5.1 we have text that indicates either "iss" or
> "x5u" identifies the issuer, which I do not believe to be accurate.

Similar to the resolution of this issue in the Authority Token draft, i removed the constraint in the text that the Header claim had to be an “x5u” and could be other ways of referencing the certificate associated with the authorized issuer.

> 
> (4) This document claims to define the "atc" claim, but
> draft-ietf-acme-authority-token also claims to do so.  I note that the
> IANA registration is currently in the other document, but this one has a
> more accurate/fleshed-out description of the contents, including the
> various keys that are present in the JSON object.  (The other document
> says it's an "array", not an object!)

Fixed and added reference to authority token document where it is defined.

> 
> (5) The end of §5.5 has some guidance on HTTP response codes in various
> failure cases.  The proposed behavior provides a trivial side channel to
> an attacker as to whether a given account ID exists (404 vs 403), and I
> think we should avoid providing such a side channel, returning 403 for
> most failures.

Agreed, good suggestion, fixed.

> 
> (6) The validation procedure in §6 just says to check that the
> "fingerprint" claim is "valid".  I think we should be more specific and
> say that it must match the account key of the client making the request.

Agreed, fixed.

> 
> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> Please consider making a pass through the examples to harmonize the
> nonce usage and signature values presented, considering the combined
> corpus encompassing this document and RFC 8555.  That is, the "nonce"
> values in the JWS POST body protected headers should be distinct, as
> should the Replay-Nonce header fields in the responses, other than
> optionally having the value from a Replay-Nonce appear in a subsequent
> POST body; similarly, the "signature" payloads should not repeat.  We
> might also go so far as to have unique "jti" values for example tokens
> between this document and draft-ietf-acme-authority-token.

I will try to complete these things before publication, I will need to go through each of the examples in detail to make sure the flow and values is correct, although i have addressed some of the examples in some of your other comments.

> 
> Section 3
> 
>   The format of the string that represents the TNAuthList MUST be
>   constructed as a base64 [RFC4648] encoding of the TN Authorization
> 
> Given the potential for confusion between regular base64 and base64url,
> it's common for RFCs to reference the specific section of RFC 4648 that
> defines the desired encoding strategy.

Fixed

> 
> It's also common to specify whether padding characters ('=') are
> included -- RFC 8555 is careful to specify that padding characters are
> NOT included, and we might wish to do so here as well.

Fixed

> 
>   A full new-order request would look as follows,
>   [...]
>   {
>     "protected": base64url({
>       "alg": "ES256",
>       "kid": "https://example.com/acme/acct/1",
>       "nonce": "5XJ1L3lEkMG7tR6pA00clA",
> 
> In addition to my above comment about the nonce, I'd also prefer if the
> account URL was not so guessable (the '1' path component) -- the example
> in RFC 8555 that uses this nonce uses a kid value of
> "https://example.com/acme/acct/evOfKhNU60wg".

Fixed 

> 
>   On receiving a valid new-order request, the CA creates an
>   authorization object, [RFC8555] Section 7.1.4, containing the
>   challenge that the ACME client must satisfy to demonstrate authority
>   for the identifiers specified by the new order (in this case, the
>   TNAuthList identifier).  The CA adds the authorization object URL to
> 
> I think we need to talk about the "(ACME) server" as doing these things,
> not the CA.

Fixed

> 
>     "status": "pending",
>     "expires": "2015-03-01T14:09:00Z",
> 
> A bit odd to have the expiration in 2015 when the "notBefore"/"notAfter"
> are in 2021...

Fixed

> 
> Section 4
> 
>     "challenges": [
>       {
>         "type": "tkauth-01",
>         "tkauth-type": "atc",
>         "token-authority": "https://authority.example.org/authz",
>         "url": "https://boulder.example.com/authz/asdf/0"
> 
> The "url" listed there is surprising to see as an example of a challenge
> URL (i.e., the "authz" path component vs. the "chall" that RFC 8555
> uses; 8555 uses "authz" in authorization URLs).  (Note that if this
> example changes, the subsequent response example would need to change
> accordingly.)

Fixed

> 
>   POST /acme/authz/asdf/0 HTTP/1.1
>   Host: boulder.example.com
>   Content-Type: application/jose+json
> 
>   {
>     "protected": base64url({
>     "alg": "ES256",
>     "kid": "https://example.com/acme/acct/1",
>     "nonce": "Q_s3MWoqT05TrdkM2MTDcw",
>     "url": "https://boulder.example.com/acme/authz/asdf/0"
>     }),
>     "payload": base64url({
>     "atc": "DGyRejmCefe7v4N...vb29HhjjLPSggwiE"
> 
> It's surprising to me that we use "atc" for both the JWT claim that
> contains authorization information and for the POST body field that
> contains the JWT that contains that authorization information.  It seems
> like we could reduce confusion by changing the challenge response object
> to use a different map key, maybe "tkauth" or something like that.
> 
> I also strongly suggest explicitly documenting the new field (of
> whatever name it has) as "a new field in the challenge object for the
> tkauth-01 challenge type".

Fair enough, i did change it to “tkauth” and defined it as above

> 
> Section 5.4
> 
>   o  a "fingerprint" key with a fingerprint value equal to the
>      fingerprint, as defined in [RFC4949], of the ACME account
>      [...]
> 
> I think we can just refer to RFC 8555 and its "thumbprint" construction
> and skip most of the details here.

Fixed

> 
>   An example of the TNAuthList Authority Token is as follows,
> 
>   {
>     [...]
>     "payload": base64url({
>       "iss":"https://authority.example.org/authz",
>       "exp":1300819380,
> 
> That expiration time is in 2011...

Fixed

> 
> Section 5.6
> 
>   When the Token Authority creates the TNAuthList Authority Token, it
>   is the responsibility of the Token Authority to validate that the
>   information contained in the ASN.1 TNAuthList accurately represents
>   the SPC or telephone number resources the ACME client is authorized
>   to represent.
> 
> I think that the Token Authority is actually evaluating the
> authorization of the entity that authenticated to the Token Authority.
> Since this (as written) involves different credentials than the ACME
> account key, I don't think we should say that this is the ACME client --
> the only way the ACME client comes into play seems to be the key
> thumbprint in the request (and we don't even require a proof of
> possesion of that key).  In effect, we rely on self-identification of
> the ACME client in the transaction that authorizes use of the SPC or
> telephone number resources.

I did clarify this, agree on ACME client not the right description, the use of the fingerprint was not meant to be verified by the token authority, but signed by the token authority so that when the party using the ACME client uses the token for the challenge, that party (that controls the ACME account with the ACME server) is indeed the same party that requested the token from the token authority.

> 
> Section 8
> 
> (The considerations I mentioned in draft-ietf-acme-authority-token
> relating to only using SHA256 for thumbprintts would also apply here.)

Fixed

> 
> Also as for draft-ietf-acme-authority-token, a reference to the JWT BCP
> (RFC 8725) would seem to be in order.

Fixed

> 
>   The creation, transport, and any storage of this token MUST follow
>   the strictest of security best practices beyond the recommendations
>   of the use of encrypted transport protocols in this document to
> 
> "strictest of security best practices" doesn't really seem like
> actionable guidance.

Fixed

> 
> Section 11.1
> 
> We only reference RFC 8224 in one location, which is just an example of
> protocols that can use PASSporTs.  While 8224 may indeed be a normative
> reference in terms of the technology involved, we don't currently
> refererence it in such a manner.  So, I'd suggest either adding
> additional (normative) references in the text or reclassifying it as
> informative.  Similarly for RFC 8225.

Fixed

> 
> NITS
> 
> I think these are all useful changes, but there's no need to respond if
> you disagree -- just go ahead and ignore anything you don't like.
> 
> Section 3
> 
>   The format of the string that represents the TNAuthList MUST be
>   constructed as a base64 [RFC4648] encoding of the TN Authorization
>   List certificate extension ASN.1 object.  The TN Authorization List
>   certificate extension ASN.1 syntax is defined in [RFC8226] section 9.
> 
> I might elide the spaces for "TNAuthorizationList", since that's the
> actual ASN.1 syntax element we want (right?).
> 
>   An example of an ACME order object "identifiers" field containing a
>   TNAuthList certificate would look as follows,
> 
>    "identifiers": [{"type":"TNAuthList","value":"F83n2a...avn27DN3=="}]
> 
> If I'm reading this right, the value (F83n2a...) should be the base64
> encoding of the (DER?) ASN.1 encoded TNAuthorizationList sequence, using
> explicit tags.  But the base64 decoding (to hex) of this string starts
> out as 17 cd ..., and I'm having a hard time getting that to line up
> with the universal tag for SEQUENCE OF as 0x10.  So, I guess I'm asking
> if this string was extracted from an actual example or is just
> "arbitrary base64".

In general many of the base64url(ASN.1) examples are not actual values, i will try to generate example values before publication.

> 
> Section 5
> 
>   The Telephone Number Authority List Authority Token (TNAuthList
>   Authority Token) is an extension of the ACME Authority Token defined
>   in [I-D.ietf-acme-authority-token].
> 
> Is it an "extension of" or an "instance of" the authority token?

I used “profile instance” 

> 
> Section 5.1
> 
>   The "iss" claim is an optional claim.  It can be used as a URL
>   identifying the Token Authority that issued the TNAuthList Authority
>   Token beyond the "x5u" Header claim that identifies the location of
>   the certificate of the Token Authority used to validate the
>   TNAuthList Authority Token.
> 
> Per RFC 7515, "x5u" can point to a certificate or a certificate chain.

Clarified that.

> 
> Section 5.4
> 
>   The "atc" claim MUST be included and is the only claim specifically
>   defined in this document.  It contains a JSON object of three
>   elements.
> 
> [We go on to list more than three elements.]

fixed

> 
>   o  a "tkvalue" key with a string value equal to the TNAuthList
>      identifier "value" string which contains the base64 encoding of
> 
> The phrasing with "TNAuthList identifier "value" string" comes off oddly
> in the absence of a specific reference to TNAuthList as the (ACME)
> identifier type.  So maybe "the 'value' string of the TNAuthList
> identifier type"?

Fixed, removed the repetitive words

> 
> Section 5.5
> 
>   The body of the POST request MUST contain the "atc" JSON object that
>   should be embedded in the token that is requested, for example the
>   body should contain a JSON object as shown:
> 
> I'd suggest s/should be embedded in the token that is requested/is
> requested as the content of the claim of that name in the issued token/
> to avoid the dangling "token that is requested" at the end.
> 
> Also, that looks like a comma splice.

I had changed this based on some comments above, but did use your suggestion

> 
> Section 5.7
> 
>   Because this specification specifically involves the TNAuthList
>   defined in [RFC8226] which involves SPC, TNBlock, and individual TNs,
>   the client may also request an Authority Token with some subset of
>   its own authority the TNAuthList provided in the "tkvalue" element in
>   the "atc" JSON object.  Generally, the scope of authority
> 
> I think we want an "in" or "as" before "the TNAuthList provided in".

Fixed

> 
>   representing a communications service provider is represented by a
>   particular SPC (e.g. in North America, an OCN or SPID) which is
>   associated with a particular set of different TN Blocks and/or TNs,
>   although more often the former typically through a set of regulated
>   authoritative registries or databases.  TNAuthList can be constructed
> 
> This sentence is getting pretty long and convoluted; I recommend
> splitting it.

Fixed

> 
>   to define a limited scope of the TNBlocks or TNs either associated
>   with an SPC or with the scope of TN Blocks or TNs the client has
>   authority over.
> 
> In the companion document we provided some motivation for why a client
> might want to claim a smaller range than it is entitled to; it could be
> worth repeating that reasoning here.

Added

> 
> Section 6
> 
>   o  Verify that the token contained in the Payload "atc" field is a
>      valid TNAuthList Authority Token.
> 
> Do we mean to verify that the value of the "atc" claim is a well-formed
> TNAuthorizationList structure?

I re-did some of the steps to clarify what is meant by verifying values in the JSON more specifically

> 
> 
>