[Unbearable] Review by Michael Jones of draft-ietf-tokbind-https-08

[Mike Jones <[e-mail]>]

I read all of draft-ietf-tokbind-https-08 today.  My resulting review comments follow.  A lot of these are things that the RFC Editor would catch, but you might as well correct them before IETF review.

IMPORTANT:

Section 5.2:  Change "the Token Binding ID in the identity token" to "the Token Binding ID (or a cryptographic hash of it) in the identity token".

Section 5.2:  Change "include in the token the Token Binding ID" to "include in the token the Token Binding ID (or a cryptographic hash of it)".

It's important to explicitly describe and sanction the usage of cryptographic hashes of the TBID in security tokens, since it's what the OpenID Connect and OAuth Token Binding specs actually do (for space reasons).

NITS:

Author list:  Change "Paypal" to "PayPal".

Abstract:  When editing other specs to become RFCs, IESG reviewers told me that references must not be present in the abstract, because it will be used in contexts such as references about the document, which the document itself is not present.  Therefore, delete [RFC5246] and [I.D.ietf-tokbind-protocol] from the abstract, while keeping the spec names that precede those references.

Abstract:  Delete the underscores around "first-party", "federated", and "different".  It's not clear why they are there and they are very distracting when reading the text.  Likewise, delete underscores used in this way other places in the draft, such as in the introduction.

Introduction: Add a comma after "e.g.".  This needs to be done in other places as well.

Introduction:  Add an informative to reference to [RFC6749] upon first use of the term "OAuth".

Section 2:  Change "pad characters" to "padding characters".

Section 2:  Change "Section 4.2.  "Server Processing Rules"" to Section 4.2 (Server Processing Rules)".  Make similar parallel changes where applicable.

Section 2.1:  Delete the comma in "various application protocols, and application contexts".

Section 2.1: "Add a hyphen to "general purpose".

Section 2.1, paragraph 2:  Change "(below)" to a section reference.  Also, this sentence is torturously complicated.  Break it into two or three concise sentences.

Section 2.1, paragraph 2:  Change "as described below" to "as described in <xref target="SECTION"/>".  If this section reference is to Section 8.1, then delete the last sentence, which says "See also Section 8.1, below.".

Section 4 title:  Change "First-party" to "First-Party".

Section 4:  Add a comma between "misuse" and "since".

Section 4:  Change "[I-D.ietf-tokbind-protocol] Section 5" to "Section 5 of [I-D.ietf-tokbind-protocol]".

Section 5.1:  Change "OpenID Connect identity token" to "OpenID Connect ID Token".

Section 5.1:  Add an informative reference to OpenID Connect upon first use of the term.  The correct reference text is supplied at the end of this review.

Section 5.1:  Add a comma after "In this section".

Section 5.1:  Add a comma after "Below" in "Below we specify".

Section 5.2:  Change "(where the identity token is called "ID Token")" to "(in which the identity token is called an "ID Token")".  Also change the "(where" to "(in which" in the following SAML clause.

Section 5.2:  Change the colon to a semicolon in "said client can use the identity token: The Relying Party will".

Section 5.2:  Change "reveals" to "uses".

Section 5.2:  Add a comma after "at the very least".

Section 5.2:  Change "redirect from Token Consumer to Token Provider" to "redirect from the Token Consumer to the Token Provider".

Section 5.3:  Change "a" to "an" in "it SHOULD include a Include-Referred-Token-Binding-ID".

Section 5.3:  Change "[RFC7231] Section 8.3" to "Section 8.3 of [RFC7231]".

Section 5.3:  Change "(see above)" to "as described in <xref target="SECTION"/>".

Section 5.3:  Put commas around "for some deployment-specific reason".

Section 5.5:  Remove the quotation remarks around "ID Token".

Legend:  Change "the "ID Token" in OIDC," to "the ID Token in OpenID Connect;".

Legend:  Change "Open ID Connect" to "OpenID Connect".

Section 6:  Change "See Section 8 "Privacy Considerations"" to "See Section 8 (Privacy Considerations)".

Section 7.3:  Change the colon after "moot" to a period or semicolon.

Section 7.3:  Change the colon after "field" to a period or semicolon.

Section 7.3:  Change "trick C to send" to "trick C into sending".

Section 7.3:  Add a comma after "as if C controls a certain key pair".

Section 7.3:  Change "Javascript" to "JavaScript".  (This correction needs to occur in other locations as well.)

Section 7.4:  Add a comma after "in a federated sign-in scenario".

Section 7.4:  Delete the semicolon in "Token Provider; or it can".

Section 7.4:  Change "as we do above" to "as is done above".

Section 7.4:  Delete the comma in "message sent by the client to the Token Consumer, and any message".

Section 7.4:  Delete the comma in "controls the redirect URL, and can tamper".

Section 7.4:  Change "trick the Token Provider to issue" to "trick the Token Provider into issuing".

Section 7.4:  Change "can not" to "cannot".

Section 8.1:  Change "[I-D.ietf-tokbind-protocol] section 4.1" to "Section 4.1 of [I-D.ietf-tokbind-protocol]".

Section 8.2 title:  Change "Life Time" to "Lifetime".

Section 8.2:  Change "across an extended period of time" to "for an extended period of time".

Section 8.2:  Change "web" to "Web".  Likewise, do this in other places that the proper name "Web" is not capitalized.

Section 8.2:  Change "life time" to "lifetime".

Section 8.3:  Delete the comma in "communicating endpoints, that receive".

Section 8.3:  Delete the comma in "other than their own, obtain".

Section 8.3:  Change ", see Section 6 "Implementation Considerations"" to ", per Section 6 (Implementation Considerations)".

Finally, I'll say that there are a lot of tortured uses of long parenthetical phrases in this spec.  At the very least, in cases in which the parentheses enclose a complete sentence or sentences, separate the parenthetical phrase from the containing sentence.  For instance, 8.3 contains this text:
   An application's various communicating endpoints, that receive Token
   Binding IDs for TLS connections other than their own, obtain
   information about the application's other TLS connections (in this
   context, "an application" is a combination of client-side and server-
   side components, communicating over HTTPS, where the client side may
   be either or both web browser-based or native application-based).
At the very least, free the parenthetical sentences, as follows:
   An application's various communicating endpoints, that receive Token
   Binding IDs for TLS connections other than their own, obtain
   information about the application's other TLS connections.  (In this
   context, "an application" is a combination of client-side and server-
   side components, communicating over HTTPS, where the client side may
   be either or both web browser-based or native application-based.)

The reference to use for OpenID Connect <xref target="OpenID.Core"/> is:

      <reference anchor="OpenID.Core" target="http://openid.net/specs/openid-connect-core-1_0.html">
        <front>
          <title>OpenID Connect Core 1.0</title>

          <author fullname="Nat Sakimura" initials="N." surname="Sakimura">
            <organization abbrev="NRI">Nomura Research Institute, Ltd.</organization>
          </author>

          <author fullname="John Bradley" initials="J." surname="Bradley">
            <organization abbrev="Ping Identity">Ping Identity</organization>
          </author>

          <author fullname="Michael B. Jones" initials="M.B." surname="Jones">
            <organization abbrev="Microsoft">Microsoft</organization>
          </author>

          <author fullname="Breno de Medeiros" initials="B." surname="de Medeiros">
            <organization abbrev="Google">Google</organization>
          </author>

          <author fullname="Chuck Mortimore" initials="C." surname="Mortimore">
            <organization abbrev="Salesforce">Salesforce</organization>
          </author>

          <date day="8" month="November" year="2014"/>
        </front>
      </reference>

                                                       Best wishes,
                                                       -- Mike