Subject: [OAUTH-WG] AD Review: draft-ietf-oauth-discovery-06

From: Eric Rescorla <[email-masked]>
Date: Sun, 3 Sep 2017 15:26:28 -0700

Hi folks,

Note: the original of this review is on Phabricator at:

  https://mozphab-ietf.devsvcdev.mozaws.net/D7

If you want to see comments in context, you can go there. Also,
you can create an account and respond inline if you like.
If you elect to, let me know if you run into problems.

-Ekr

I have marked a number of places where it seems like you either need
defaults or need to indicate what the semantics are if missing

   This metadata can either be communicated in a self-asserted fashion
   or as a set of signed metadata values represented as claims in a JSON
I assume "self-asserted" in this case means "asserted by the server origin
via HTTPS"

Line 222
      authentication methods.  Servers SHOULD support "RS256".  The
      value "none" MUST NOT be used.
What's the default if omitted?

Line 235
      represented as a JSON array of BCP47 [RFC5646] language tag
      values.
What's the default if omitted?

Line 267
      "OAuth Token Endpoint Authentication Methods" registry
      [IANA.OAuth.Parameters].
What's the default if omitted?

Line 275
      "client_secret_jwt" authentication methods.  The value "none" MUST
      NOT be used.
What's the default if omitted?

Line 288
      Access Token Types" registry [IANA.OAuth.Parameters].  (These
      values are and will remain distinct, due to Section 7.2.)
What's the default if omitted?

Line 296
      "client_secret_jwt" authentication methods.  The value "none" MUST
      NOT be used.
What's the default if omitted?

Line 304
      challenge method values are those registered in the IANA "PKCE
      Code Challenge Methods" registry [IANA.OAuth.Parameters].
What's the default if omitted?

Line 343
   MUST be registered in the IANA "Well-Known URIs" registry
   [IANA.well-known].
IMPORTANT: Shouldn't this be required to be HTTPS

Line 500
   client MUST perform a TLS/SSL server certificate check, per RFC 6125
   [RFC6125].  Implementation security considerations can be found in
   Recommendations for Secure Use of TLS and DTLS [BCP195].
Hmm.... I'm unsure about whether this should be a citation to 2818. Is the
general feeling that 6125 superceded 2818?

Line 564
   The following registration procedure is used for the registry
   established by this specification.
This section seems like it needs RFC2119 language

Line 568
   Values are registered on a Specification Required [RFC5226] basis
   after a two-week review period on the [email-masked]
   mailing list, on the advice of one or more Designated Experts.
What happens if you don't do anything within two weeks.

Line 756
   o  Change Controller: IESG
   o  Specification Document(s): Section 2 of [[ this specification ]]
Extra whitespace.

---

From: Mike Jones <[email-masked]>
Date: Tue, 5 Sep 2017 23:11:54 +0000

Thanks for your useful review, Eric.  Proposed resolutions to all comments are inline prefixed by "Mike>".

From: OAuth [mailto:[email-masked]] On Behalf Of Eric Rescorla
Sent: Sunday, September 3, 2017 3:26 PM
To: [email-masked]
Subject: [OAUTH-WG] AD Review: draft-ietf-oauth-discovery-06

Hi folks,

-Ekr

   This metadata can either be communicated in a self-asserted fashion
   or as a set of signed metadata values represented as claims in a JSON
I assume "self-asserted" in this case means "asserted by the server origin via HTTPS"

Mike> Thanks - I will use this language.

Line 222

Mike> I will add "If omitted, these authentication methods cannot be used."

Line 235

Mike> There is no default.  I will add "If omitted, the set of supported languages and scripts is unspecified."

Line 267

Mike> I will add client_secret_basic as the default - just like it already was for token_endpoint_auth_methods_supported.

Line 275

Mike> I will add "If omitted, these authentication methods cannot be used."

Line 288

Mike> There is no obvious default.  Therefore, I will add "If omitted, the set of supported authentication methods MUST be determined by other means."

Line 296

Mike> I will add "If omitted, these authentication methods cannot be used."

Line 304

Mike> I will add "If omitted, the authorization server does not support PKCE."

Line 343

Mike> I will add "This path MUST use the "https" scheme."

Line 500
   client MUST perform a TLS/SSL server certificate check, per RFC 6125
   [RFC6125].  Implementation security considerations can be found in
   Recommendations for Secure Use of TLS and DTLS [BCP195].
Hmm.... I'm unsure about whether this should be a citation to 2818. Is the general feeling that 6125 superceded 2818?

Mike> OAuth 2.0 [RFC 6749] also requires an RFC 6125 certificate validation, so this is in line with other uses of the OAuth protocol family.

Line 564

Mike> This registry language closely follows that in OAuth 2.0 [RFC 6749] and subsequent OAuth specifications.  I'd rather keep them parallel unless something isn't clear.

Line 568
   Values are registered on a Specification Required [RFC5226] basis
   after a two-week review period on the mailto:[email-masked]
   mailing list, on the advice of one or more Designated Experts.
What happens if you don't do anything within two weeks.

As it says later in this section "Registration requests that are undetermined for a period longer than 21 days can be brought to the IESG's attention (using the [email-masked] mailing list) for resolution."  

Line 756

Are you talking about there being two spaces between the bullet character "o" and the items such as "Change Controller: IESG"?  That's what <list style='symbols'> does.  Or are you wanting more whitespace somewhere?  Please give more context because I'm not looking at this on Phabricator.  (I created an account "mbj" but it wanted me to install a second factor phone app, which seemed like a bit much...)

				Thanks,
				-- Mike

---

From: Mike Jones <[email-masked]>
Date: Thu, 7 Sep 2017 21:25:16 +0000

Draft -07 https://tools.ietf.org/html/draft-ietf-oauth-discovery-07 contains these proposed resolutions.  The only change is that in places where I'd previously proposed to say "If omitted, these authentication methods cannot be used" I now say "This metadata entry MUST be present if either of these authentication methods are specified in the "{token,revocation,introspection}_endpoint_auth_methods_supported" entry.  No default algorithms are implied if this entry is omitted."

I made the change because saying that they cannot be used isn't actually correct.  This information could always have been communicated out-of-band to the metadata.

				-- Mike

-----Original Message-----
From: Mike Jones 
Sent: Tuesday, September 5, 2017 4:12 PM
To: 'Eric Rescorla' <[email-masked]>; [email-masked]
Subject: RE: [OAUTH-WG] AD Review: draft-ietf-oauth-discovery-06

Hi folks,

If you want to see comments in context, you can go there. Also, you can create an account and respond inline if you like.
If you elect to, let me know if you run into problems.

-Ekr

I have marked a number of places where it seems like you either need defaults or need to indicate what the semantics are if missing

   This metadata can either be communicated in a self-asserted fashion
   or as a set of signed metadata values represented as claims in a JSON I assume "self-asserted" in this case means "asserted by the server origin via HTTPS"

Line 222

Line 235

Line 267

Line 275

Line 288
      Access Token Types" registry [IANA.OAuth.Parameters].  (These
      values are and will remain distinct, due to Section 7.2.) What's the default if omitted?

Line 296

Line 304

Line 343

Line 500

Line 564

Line 568

Line 756
   o  Change Controller: IESG
   o  Specification Document(s): Section 2 of [[ this specification ]] Extra whitespace.

				Thanks,
				-- Mike

---

From: Mike Jones <[email-masked]>
Date: Mon, 11 Sep 2017 19:35:46 +0000

Eric, I believe that https://tools.ietf.org/html/draft-ietf-oauth-discovery-07 addresses all your comments.  Thanks for them again.

If you agree, can you please progress the document to the next step?

				Thanks,
				-- Mike

-----Original Message-----
From: OAuth [mailto:[email-masked]] On Behalf Of Mike Jones
Sent: Thursday, September 7, 2017 2:25 PM
To: Eric Rescorla <[email-masked]>; [email-masked]
Subject: Re: [OAUTH-WG] AD Review: draft-ietf-oauth-discovery-06

				-- Mike

-----Original Message-----
From: Mike Jones 
Sent: Tuesday, September 5, 2017 4:12 PM
To: 'Eric Rescorla' <[email-masked]>; [email-masked]
Subject: RE: [OAUTH-WG] AD Review: draft-ietf-oauth-discovery-06

Hi folks,

-Ekr

Line 222

Line 235

Line 267

Line 275

Line 288

Line 296

Line 304

Line 343

Line 500

Line 564

Line 568

Line 756

				Thanks,
				-- Mike
_______________________________________________
OAuth mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/oauth

---

From: Eric Rescorla <[email-masked]>
Date: Sat, 23 Sep 2017 12:58:32 -0700

Hi Mike,

These changes look good. I have pushed the last call button.

-Ekr

On Mon, Sep 11, 2017 at 12:35 PM, Mike Jones <[email-masked]>
wrote:

> Eric, I believe that https://tools.ietf.org/html/
> draft-ietf-oauth-discovery-07 addresses all your comments.  Thanks for
> them again.
>
> Draft -07 https://tools.ietf.org/html/draft-ietf-oauth-discovery-07
> contains these proposed resolutions.  The only change is that in places
> where I'd previously proposed to say "If omitted, these authentication
> methods cannot be used" I now say "This metadata entry MUST be present if
> either of these authentication methods are specified in the
> "{token,revocation,introspection}_endpoint_auth_methods_supported"
> entry.  No default algorithms are implied if this entry is omitted."
>
> I made the change because saying that they cannot be used isn't actually
> correct.  This information could always have been communicated out-of-band
> to the metadata.
>
>                                 -- Mike
>
> If you want to see comments in context, you can go there. Also, you can
> create an account and respond inline if you like.
>    or as a set of signed metadata values represented as claims in a JSON I
> assume "self-asserted" in this case means "asserted by the server origin
> via HTTPS"
>
> Mike> I will add client_secret_basic as the default - just like it already
> was for token_endpoint_auth_methods_supported.
>
> Line 275
> Mike> There is no obvious default.  Therefore, I will add "If omitted, the
> set of supported authentication methods MUST be determined by other means."
>
> Line 296
> Mike> OAuth 2.0 [RFC 6749] also requires an RFC 6125 certificate
> validation, so this is in line with other uses of the OAuth protocol family.
>
> Line 564
> Mike> This registry language closely follows that in OAuth 2.0 [RFC 6749]
> and subsequent OAuth specifications.  I'd rather keep them parallel unless
> something isn't clear.
>
> Line 568
> As it says later in this section "Registration requests that are
> undetermined for a period longer than 21 days can be brought to the IESG's
> attention (using the [email-masked] mailing list) for resolution."
>
> Line 756
> Are you talking about there being two spaces between the bullet character
> "o" and the items such as "Change Controller: IESG"?  That's what <list
> style='symbols'> does.  Or are you wanting more whitespace somewhere?
> Please give more context because I'm not looking at this on Phabricator.
> (I created an account "mbj" but it wanted me to install a second factor
> phone app, which seemed like a bit much...)
>
>                                 Thanks,
>                                 -- Mike