Re: [Privacy-pass] Roman Danyliw's Discuss on draft-ietf-privacypass-protocol-14: (with DISCUSS and COMMENT)

[Christopher Wood <[e-mail]>]

Thanks for the thorough review, as always, Roman. We addressed your comments in the following PR:

   https://github.com/ietf-wg-privacypass/base-drafts/pull/494

Please see inline below for responses to specific feedback.

Best,
Chris

> On Sep 20, 2023, at 6:13 PM, Roman Danyliw via Datatracker <[e-mail]> wrote:
> 
> Roman Danyliw has entered the following ballot position for
> draft-ietf-privacypass-protocol-14: 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/about/groups/iesg/statements/handling-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-privacypass-protocol/
> 
> 
> 
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
> 
> Two things related to references ...
> 
> ** Section 7.  Given the deferment of security and privacy considerations to
> [ARCHITECTURE] and [CONSISTENCY], both seem to be a normative references.

Making the architecture a normative reference seems fine, but the referenced consistency document doesn’t rise to the same bar. That document provides guidance for how someone might implement a consistency mechanism, but it doesn’t actually specify a protocol. In this way, I consider it to be an informational reference.

> 
> ** The reference for the format of SPKI (for RSA key material) doesn’t seem
> right based on my read of the ASN.1 references.
> 
> -- Section 6.5
>   The key identifier for a keypair (skI, pkI), denoted token_key_id, is
>   computed as SHA256(encoded_key), where encoded_key is a DER-encoded
>   SubjectPublicKeyInfo (SPKI) object carrying pkI.  The SPKI object
>   MUST use the RSASSA-PSS OID [RFC5756], which specifies the hash
>   algorithm and salt size.
> 
> -- Section 8.2.2 says:
>   *  Token Key Encoding: Serialized as a DER-encoded
>      SubjectPublicKeyInfo (SPKI) object using the RSASSA-PSS OID
>      [RFC5756]
> 
> SubjectPublicKeyInfo (SPKI) seems to be defined in RFC5280.  Additionally, RFC
> 4055 provides the “algorithm” OBJECT IDENTIFIER as id-RSASSA-PSS, “parameters”
> are RSASSA-PSS-params, and the “subjectPublicKey” is RSAPublicKey.

Wow, indeed, the current references do not seem correct! I believe the PR correctly references the necessary definitions, and also makes an attempt to use the proper terms. I will ask for review from people more familiar with the X.509 details to ensure it’s correct and matches what is deployed today.

> 
> ==[ snip from RFC5280 ]==
>   AlgorithmIdentifier  ::=  SEQUENCE  {
>        algorithm               OBJECT IDENTIFIER,
>        parameters              ANY DEFINED BY algorithm OPTIONAL  }
> 
> SubjectPublicKeyInfo  ::=  SEQUENCE  {
>     algorithm            AlgorithmIdentifier,
>     subjectPublicKey     BIT STRING  }
> ==[ snip from RFC5280 ]==
> 
> ==[ snip from RFC4055 ]==
>      -- Used in SubjectPublicKeyInfo of X.509 Certificate.
>      RSAPublicKey  ::=  SEQUENCE  {
>         modulus           INTEGER,    -- n
>         publicExponent    INTEGER  }  -- e
> 
>      -- AlgorithmIdentifier parameters for id-RSASSA-PSS.
>      -- Note that the tags in this Sequence are explicit.
> 
>      RSASSA-PSS-params  ::=  SEQUENCE  {
>         hashAlgorithm     [0] HashAlgorithm DEFAULT
>                                  sha1Identifier,
>         maskGenAlgorithm  [1] MaskGenAlgorithm DEFAULT
>                                  mgf1SHA1Identifier,
>         saltLength        [2] INTEGER DEFAULT 20,
>         trailerField      [3] INTEGER DEFAULT 1  }
> 
>      HashAlgorithm  ::=  AlgorithmIdentifier
> 
>      MaskGenAlgorithm  ::=  AlgorithmIdentifier
> ==[ snip from RFC4055 ]==
> 
> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> Thank you to Alexey Melnikov for the SECDIR review.
> 
> ** Section 2.  The definition of Client and Issue as defined here seems
> slightly different than those in Section 2 of [ARCHITECTURE].  Is that
> intentional?  Why redefine them?

Good point. I replaced these definitions with references to the architecture document. 

> 
> ** Section 4.  token-key
> 
> -- Please explicitly say that the contents and formatting of this field are
> defined by the value of token-type.
> 
> -- Please provide a normative reference for base64URL encoding.
> 
> -- The openssl decode in in Section 6.5 was very helpful.  Consider providing a
> full example of a base64 encoded blob, which is then decoded with openssl.

Good idea — the PR listed above adds one such example.

> 
> ** Section 4.  Editorial. Why is the “not-before” field not included in Table 2
> which described all of the fields in the token-keys object?

It’s an optional field, rather than a mandatory field, so we opted to demote it from the table.

> 
> ** Section 5.1.  Editorial.
> 
>   nonce = random(32)
> 
> Please explain in the prose what this is doing.  Is that returning 32-bytes of
> random data?

Indeed — we use the same notational conveniences as OHTTP for this, and updated the prose to denote that the nonce value is 32 bytes. I think that provides sufficient clarity.

> 
> ** Section 5.1.  Editorial.
> 
>   POST /request HTTP/1.1
>   Host = issuer.example.net
>   Accept = application/private-token-response
>   Content-Type = application/private-token-request
>   Content-Length = <Length of TokenRequest>
> 
> Shouldn’t this read “Host: issuer.example.net” and not “Host =
> issuer.example.net”, or is this not trying to show an HTTP header?

Yep, thanks. We fixed that.

> 
> ** Section 6.5.  Per the openssl decode of the SPKI file, where is the public
> key in that example?

It’s the final BIT STRING in the SPKI blob.

> 
> ** Appendix B.2.  Test Vector #2.  I had trouble verifying the token of the
> second test vector in the example.  I didn’t try any other example.  I tried
> the following:

What are you trying to do here? The token is a Token structure, which openssl won’t be able to parse. Are you trying to verify the signature of the token using the public key in the test vector?

> 
> $ cat privacy-pass-token.hexbin  (no line breaks in the file)
> 0002a1aa8b371c37c9a8ddbd7342ab4f9dd5227d5b1600dca6517b60f63143cd43a3bb8a8cf1c59e7a251358ed76fe0ccff61044bc79dd261f16020324d22f2d434cca572f8982a9ca248a3056186322d93ca147266121ddeb5632c07f1f71cd27082899f4bc385b4147a650a3e7efc7d23a743cb944bb53e2ed8b88ee03a9c0b1cf1f73fd7f15c1bd1f850a5a96a34d4ee9f295543f30ac920825e9a0ce26e924f2c145583019dd14408c565b660e8b702fbea559908b1a8c8f9d21ef22f7cc6a4641c57c146e6c5497d2890ca230a6749f5f83a8fdd79eba0722f10dff9e81a2fb2d05fa4d989acc2e93f595ae69c98c3daa3b169efcdd6e59596b2f049f16ba49528761f661032da65a3ee0fe8a22409766e90daf8c60323c16522818c49273c795f26bbab306dc63cfc16fe1702af2464028cf819cc647d6f9b8a8f54d7d6585a268fdb8c75f76618c64aba266836a3b2db7cdd739815a021d7ff2b36ef91f23
> 
> $ xxd -r privacy-pass-token.hexbin | openssl asn1parse -dump -inform DER
> Error: offset out of range
> 
> $ xxd -r -p privacy-pass-token.hexbin | openssl asn1parse -dump -inform DER
>    0:d=0  hl=2 l=   2 prim: EOC
>      0000 - a1 aa