[websec] AD review of draft-ietf-websec-key-pinning-17

[Barry Leiba <[e-mail]>]

With the publication request for draft-ietf-websec-key-pinning, here's
my AD review.  I'm putting the document into "Revised I-D Needed"
substate.  That said, you should definitely push back at me on points
below that you disagree with.  And note the point about the registry
that you're not creating, which I specifically want to discuss with
you.

Yoav, thanks for a very good shepherd writeup.

Tiny thing in the Abstract: we have to squash this "this memo" stuff
forever, but it'll probably take forever to do it.  Please make it
"this document".

Similarly in the Introduction: Remember that the text has to make
sense after publication.  "We propose a new HTTP header to enable..."
doesn't do that (even though "Proposed Standard" will be the RFC's
status).  Go with "This document defines a new HTTP header that
enables...".

Tiny editorial: I suggest changing "that becomes invalid.  (See
Section 4.)" to "that becomes invalid (see Section 4)." -- merging the
citation into the sentence.  There are a couple of other citations in
the document that also read like that.

And in general, wherever you have a citation that looks like this: ([RFC6234])
...remove the parentheses so it looks like this: [RFC6234]

Is "we believe that" in the middle of the second paragraph something
that should stand as it is in the final RFC?

And I suggest:
OLD
   We intend for hosts to use PKP together with HSTS ([RFC6797]), but is
   possible to pin keys without requiring HSTS.
NEW
   PKP is meant to be used together with HTTP Strict Transport Security
   (HSTS) [RFC6797], but it is possible to pin keys without requiring HSTS.
END

You could make sure that "This draft is being discussed on the WebSec
Working Group mailing list, [e-mail]." is removed in AUTH48,
and the RFC Editor is likely to catch that anyway, but it can also be
removed now: the draft name already points readers to the working
group.

-- Section 2.1 --

   The "Public-Key-Pins" and "Public-Key-Pins-Report-Only" header
   fields, also referred to within this specification as the PKP and
   PKP-RO header fields, respectively, are are response headers used by
   server to indicate that a a UA should perform Pin Validation
   (Section 2.6) in regards to the host emitting the response message
   containing these header fields, and provide the necessary information
   for the UA to do so.

Apart from the length of that one sentence, there are some typos
there.  How about this?:

NEW
   The "Public-Key-Pins" and "Public-Key-Pins-Report-Only" header
   fields, also referred to within this specification as the PKP and
   PKP-RO header fields, respectively, are new response headers
   defined in this specification.  They are used by a server to
   indicate that a UA should perform Pin Validation (Section 2.6)
   for the host emitting the response message, and to provide the
   necessary information for the UA to do so.
END

   2.  All simple-directives MUST appear only once in a given header
       field.  Directives are either optional or required, as stipulated
       in their definitions.

I don't think that's correct.  It's certainly not required that each
header field contain every simple-directive.  I think you mean this:

NEW
   2.  A given simple-directive MUST NOT appear more than once in a
       given header field.  Directives are either optional or required,
       as stipulated in their definitions.
END

For list item 4, would it help to explain why?  Perhaps, "In
particular, UAs must not attempt to fix malformed header fields." ?

   Additional directives extending the semantic functionality of the
   header fields can be defined in other specifications, with a registry
   (having an IANA policy definition of IETF Review [RFC5226]) defined
   for them at such time.  Such future directives will be ignored by UAs
   implementing only this specification, as well as by generally non-
   conforming UAs.

I don't think it's clear enough that this document doesn't define that
registry, and that this is giving instruction for future registry
creation.  Please talk with me about why you feel the need *now* to
say that a future registry will have to use IETF Review as its policy.
Why would Specification Required or Expert Review not be OK?  Why
shouldn't the decision be left for the time the registry is created?

   In the pin-directive, the token is the name of a cryptographic hash
   algorithm, and MUST be "sha256".  (In the future, additional hash
   algorithms MAY be registered and used.)  The quoted-string is a
   sequence of base 64 digits: the base 64-encoded SPKI Fingerprint
   ([RFC4648]).  See Section 2.4.

I don't like the MUST there (nor the MAY).  As to "registered and
used", is this intended to be in the same registry as above?  So the
directive "pin-sha256" would be registered, along with a new
"pin-shaXYZ"?

Assuming that's right, I'm really starting to think that the registry
should be created now.  The reason to defer creation is if we think
that no new directives will ever really come along, so we won't need
the registry.  But if the hash algorithm names will be in the
registry, we can be pretty sure we'll need new ones.

Anyway, a fix for this paragraph (but this will change if we create
the registry now) could be like this:

NEW
   In the pin-directive, the token is the name of a cryptographic hash
   algorithm.  The only algorithm allowed at this time is "sha256";
   additional algorithms may be defined in the future. The quoted-string
   is a sequence of base 64 digits: the base 64-encoded SPKI Fingerprint
   [RFC4648].  See Section 2.4.
END

   The UA MUST ignore pin-directives with tokens naming hash algorithms
   it does not recognize.  If the set of remaining effective pin-
   directives is empty, and if the connection passed Pin Validation with
   the UA's existing noted pins for the Host (i.e. the Host is a Known
   Pinned Host), the UA MUST cease to consider the Host as a Known
   Pinned Host.  (I.e. the UA should fail open.)  The UA SHOULD indicate
   to users that the Host is no longer a Known Pinned Host.

The first sentence follows from (5) in the above list, so I suggest
you say so.  This also appears to be where you define "Known Pinned
Host", but you've buried the lede here.  I suggest this:

NEW
   When a connection passes Pin Validation using the UA's noted pins
   for the Host at the time, the Host becomes a Known Pinned Host.

   According to rule 5, above, the UA MUST ignore pin-directives with
   tokens naming hash algorithms it does not recognize.  If the set of
   remaining effective pin-directives is empty, and if the Host is a
   Known Pinned Host, the UA MUST cease to consider the Host as a Known
   Pinned Host (the UA should fail open).  The UA SHOULD indicate to
   users that the Host is no longer a Known Pinned Host.
END

On the last sentence, though, I'm really unsure: are we really giving
UI advice in a protocol document?  Is that a good idea?  Do w have any
sense that my mother will have the first idea what you're talking
about when you indicate this to her?

This also is a good place for me to point out that you use "host" and
"Host" inconsistently.  I think you use "pin" and "Pin" inconsistently
as well.  The RFC Editor will ask you to fix that, so you can save
time by doing it now.  Please go through the document and make sure
your capitalization in general is purposeful and consistent.

-- Section 2.1.1 --
In the first sentence, remove both commas.  The second sentence is
unnecessary, as it's repeated at the end of the section.

-- Section 2.1.2 --
Change "which" to "that" (or the RFC Editor will).

-- Section 2.1.3 --
What does it mean to use POST on a URI that isn't http or https?  What
do you do with a wss URI, for example?

   Note that the report-uri need not necessarily be in the same Internet
   domain or web origin as the Known Pinned Host.

This is confusing, particularly in context with the previous
paragraph.  I think you mean this:

NEW
   Note that the host in the report-uri need not necessarily be in the
   same Internet domain or web origin as the host being reported about.
END

-- Section 2.2.1 --

   Establishing a given host as a Known Pinned Host, in the context of a
   given UA, MAY be accomplished over the HTTP protocol, which is in
   turn running over secure transport, by correctly returning (per this
   specification) at least one valid PKP header field to the UA.  Other
   mechanisms, such as a client-side pre-loaded Known Pinned Host list
   MAY also be used.

There are two problems with this paragraph:

1. It has two sentences, and each sentence gives a MAY directive.
That means they're both optional, which means that it's possible to do
neither.  Really?  It seems to me that you mean to say that you do one
or the other.

2. The "which is in turn" part is non-restrictive -- it could be
removed without changing the meaning.  I don't think so.  I think you
mean to say that it may be accomplished by using HTTP over secure
transport -- the secure transport is a necessary part, yes?

Assuming so, try this:

NEW
   Establishing a given host as a Known Pinned Host, in the context of a
   given UA, is accomplished as follows:

   1. Over the HTTP protocol running over secure transport, by correctly
   returning (per this specification) at least one valid PKP header field
   to the UA.

   2. Through other mechanisms, such as a client-side pre-loaded Known
   Pinned Host list.
END

-- Section 2.3.1 --

"the UA MUST either" introduces a list of eight items.  Only the first
two are meant to be part of the "MUST either".  I suggest that you
pull the last six items out of the list, and just make them regular
paragraphs.

In the second list item, "different than that already maintained":
"different than" is incorrect.  Use either "different from" (US) or
"different to" (UK).

I think the third and fourth items can be in one paragraph.

"Otherwise:" is not necessary.

The sixth and seventh items can be merged into one paragraph (one sentence).

-- Section 2.3.2 --
This section should say what to do with the KPH cached information if
a PKP-RO header is received.  If the Pin Validation fails, it's clear
what to do.  But if Pin Validation succeeds, does the host become a
KPH?  If it's already a KPH, is the cached information updated?  If
there's both a PKP header and a PKP-RO header, Pin Validation succeeds
for both, and they have different directives, which wins with respect
to the KPH cached information?

-- Section 2.3.3 --

   The UA MUST ignore all
   expired Known Pinned Hosts from its cache if, at any time, an expired
   Known Pinned Host exists in the cache.

Eh?  I don't understand what this is trying to say.  Do you mean "MUST
*remove* them from its cache?  What's with the "at any time" bit?
Please try rephrasing this -- I don't understand it well enough to
try.

For the second paragraph, I would add something more explanatory to
real humans here, inserted as a first sentence: "Known Pinned Hosts
are only domain names, and never IP addresses."

And here we have another hugely long sentence.  Please keep in mind
that when sentences get too long, people have trouble understanding
them.

OLD
   Otherwise, if the substring does not congruently match a Known Pinned
   Host's domain name, per the matching procedure specified in
   Section 8.2 of [RFC6797], then the UA MUST note this host as a Known
   Pinned Host, caching the Pinned Host's domain name and noting along
   with it the Effective Expiration Date (or enough information to
   calculate it, i.e. the Effective Pin Date and the value of the max-
   age directive), whether or not the includeSubDomains directive is
   asserted, the value of the report-uri directive (if present).  If any
   other metadata from optional or future PKP header directives is
   present in the Valid Pinning Header, the UA MAY note them if it
   understands them, and need not note them if it does not understand
   them.
NEW
   Otherwise, if the substring does not congruently match an existing
   Known Pinned Host's domain name, per the matching procedure specified
   in Section 8.2 of [RFC6797], then the UA MUST add this host to the
   Known Pinned Host cache.  The UA caches:
   o the Pinned Host's domain name,
   o the Effective Expiration Date, or enough information to calculate
     (the Effective Pin Date and the value of the max-age directive),
   o whether or not the includeSubDomains directive is asserted, and
   o the value of the report-uri directive, if present.

   If any other metadata from optional or future PKP header directives
   are present in the Valid Pinning Header, and the UA understands them,
   the UA MAY note them as well.
END

-- Section 2.4 --

   (Future
   versions of this specification may add new algorithms and deprecate
   old ones.)

I would say "Future specifications"; algorithms might be added or
deprecated in extensions, not necessarily in future versions of *this*
specification.

   If the certificate's subjectPublicKeyInfo is incomplete when taken in
   isolation, such as when holding a DSA key without domain parameters,
   a public key pin cannot be formed.  Hence, pins using these keys
   cannot be pinned.

The second sentence is silly.  *Pins* aren't pinned.  Either remove
the sentence (it's redundant anyway) or change the subject.

-- Section 2.5 --

   Upon receipt of the PKP response header field, the UA notes the host
   as a Pinned Host, storing the Pins and their associated directives in
   non-volatile storage

"as a Known Pinned Host"... need to use terminology consistently.

The three-bullet list is wrong: the first phrase of each must be
factored out, because the "if and only if" applies to all three points
together.  So, like this:

OLD
   The UA MUST observe these conditions when noting a Host:

   o  The UA MUST note the Pins if and only if it received the PKP
      response [...]

   o  The UA MUST note the Pins if and only if the TLS connection [...]

   o  The UA MUST note the Pins if and only if the given set of Pins
      contains [...]
NEW
   The UA MUST note the Pins for a Host if and only if all three of
   the following conditions hold:

   o  It received the PKP response [...]

   o  The TLS connection [...]

   o  The given set of Pins contains [...]
END

-- Section 2.6 --

   A UA
   SHOULD perform Pin Validation whenever connecting to a Known Pinned
   Host, but MAY allow Pin Validation to be disabled for Hosts according
   to local policy.

Typical SHOULD/MAY trap.  MAY is not an explanation of when you can
violate SHOULD.  MAY means that something is entirely optional.

NEW
   A UA
   SHOULD perform Pin Validation whenever connecting to a Known Pinned
   Host.  It is acceptable to allow Pin Validation to be disabled for
   some Hosts according to local policy.
END

   Although the UA has previously received Pins at the HTTP layer, it
   can and MUST perform Pin Validation at the TLS layer, before
   beginning an HTTP conversation over the TLS channel.  The TLS layer
   thus evaluates TLS connections with pinning information the UA
   received previously, regardless of mechanism: statically preloaded,
   via HTTP header, or some other means (possibly in the TLS layer
   itself).

Can you re-word this?  It seems very strained, and I don't understand
it.  There's no background for telling me what the UA has previously
received.  I thought Pins *only* happened with TLS, so how can they
have been received without it?  And please avoid phrasing such as "can
and MUST"; it if MUST, it clearly can, or else the MUST is
meaningless.

For the last paragraph of the section, the combined negatives can be
confusing, but the structure is necessary in order for the SHOULD NOT
to work.  Perhaps an inserted first sentence that explains it
affirmatively will help: "UAs send validation failure reports only
when Pin Validation is actually in effect."

-- Section 2.7 --
In the first sentence, again, you're giving UI advice, and advice that
I find questionable.  This is an option that will *only* be applicable
to expert users, and that won't be understood, much less used, by
99.999% of UA users (certainly not by my mother).  I can't imagine how
such advice, well intentioned though it may be, merits a SHOULD.

-- Section 3 --

   include-subdomains indicates whether or not the UA has noted the
   includeSubDomains directive for the Known Pinned Host.  It is
   provided as one of the JSON identifiers true or false.

I would put "true" and "false" in quotes in the text.

-- Section 4.3 --

   Because having a backup key pair is so important to recovery, UAs
   MUST require that hosts set a Backup Pin. (See Section 2.5.)

Unless I misunderstand, this requirement means that if my server
doesn't send a backup pin, my server doesn't benefit from key pinning
(it's as though I never included the header in the first place).  If
that's not true, then Section 2.5 needs to be fixed to make it clear
that failure to include a backup pin constitutes a validation failure.

If that is true, then you're trading off recovery for security, and I
expect the Security ADs to question that.  To head that off, it might
be good to acknowledge that here, and perhaps to say a few more words
about it.

-- Section 7 --
I'll repeat the comment here about giving UI advice, especially with
SHOULDs and MUSTs.  The portion of users who will have any idea what
you're talking about is vanishingly small.

-- Appendix B --
I *strongly* advise you to pull the SHOULDs out of here, and talk
about this advice without using 2119 key words.  None of this has
anything to do with interoperability of the protocol -- it's all
quality of implementation/deployment stuff.

--
Barry, Applications AD