ace-wg / ace-oauth

This is the working area for draft-ietf-ace-oauth-authz
8 stars 8 forks source link

Éric Vyncke review comments #183

Closed hannestschofenig closed 3 years ago

hannestschofenig commented 3 years ago

Last very minor/cosmetic comment about this document as well to the oAuth terminology: using "refresh tokens" sounds weird to me, I would have preferred "permanent tokens" or "long-term tokens", but, I am afraid that the train has left the station for many years ;-) And the same applies for "introspection" that usually is done internally and does not require a third party as in oAuth (but this is another train, which has also left the station...).

This terminology decision was made in early days of OAuth = long time ago.

-- Section 3 -- Should references/expansions be added for "HTTP/2, MQTT, BLE and QUIC" ?

Done.

-- Section 3.1 -- Suggest to review the order of the definitions, notably popping up "introspection" as it is used by most of the other terms.

Done.

-- Section 4 -- Mostly cosmetic, any reason why figure 1 is so far away from its mention in §1 ?

Changed.

In "ensure that its content cannot be modified, and if needed, that the content is confidentiality protected", I wonder why the confidentiality is only optional ? As far as I understand it, the possession of an access token grants access to a ressource, so, it should be protected against sniffing. What did I miss ?

I added a note that this confidentiality protection of the access token happens on top of the protection offered by the underlying communication security protocol.

In "If the AS successfully processes the request from the client" may look ambiguous because processing correctly (per protocol) an invalid credential is also "successfully processed". Suggest to mention something about "positive authentication" ;)

Text added.

-- Section 5 -- As a non-English native speaker, I cannot see the verb in the second proposition in "For IoT, it cannot be assumed that the client and RS are part of a common key infrastructure, so the AS provisions credentials or associated information to allow mutual authentication.". While I obviously understand the meaning, could it be rephrased ?

Text changed.

-- Section 5.1.1 -- Could the word "unprotected" be better defined in "received on an unprotected channel" ? E.g., is it only about TLS ? Else, I like the implicit lack of trust.

Text changed to "unsecured channel"

-- Section 5.1.2 -- I must admit that I have failed to understand the semantic of "audience"... Can you either explain its meaning or provide a reference ?

I assume you mean "Section 5.3". Text added.

-- Section 5.5 -- In "Since it requires the use of a user agent (i.e., browser)" is it "i.e." or "e.g." ?

I cannot find this text in the document.

-- Section 5.6 -- s/the semantics described below MUST be/the semantics described in this section MUST be/ ?

OK

In "The default name of this endpoint in an url-path is '/token'" should "SHOULD" normative language be used ?

OK.

-- Section 5.6.4.1 -- In figure 11, would you mind adding the section ID in addition to RFC 6749 ? I failed to spot them in RFC 6749.

Done.

-- Section 5.7.2 -- It is a little unclear to me which profile must be used as 'profile' is optionnial? Should a default or any profile be used ?

I cannot find this text.

-- Section 5.8.1 -- Suggest to use the BCP14 "SHOULD" in the text "The default name of this endpoint in an url-path is '/authz-info'"

Changed.

-- Section 10.2 -- Is RFC 7049 really an informative reference as CBOR appears as the default encoding ?

Changed to a normative reference and updated reference.

All nits below have been addressed.

== NITS ==

s/application layer protocol/application-layer protocol/ ?

Should multi-words message names (e.g., AS Request Creation Hints) be enclosed by quotes ?

-- Section 2 -- Please introduce "authz-info" before first use.

-- Section 3.1 -- "PoP" is expanded twice in this section ;-)

"CBOR encoding (CWT) " the "CWT" acronym does not match the expansion :-)

-- Section 4 --

Sometimes "Client" is used and sometimes "client" is used...

s/reference to a specific credential/reference to a specific access credential/ ?

-- Section 5.1.2 -- Can you introduce to "kid" acronym ? It too me a while to understand that it is (probably) key-id... :-)

Unsure whether "nonce: h'e0a156bb3f'," is the usual IETF way to introduce an hexadecimal number.

typo in "5.8.4. Key Expriation" :-)

hannestschofenig commented 3 years ago

Comments addressed in this PR: https://github.com/ace-wg/ace-oauth/pull/184

LudwigSeitz commented 3 years ago

Merged