Having read the spec cover-to-cover, it surprised me how different this is from OAuth, despite the statements that it is based on OAuth. My highest-priority request is to make all the extensions to OAuth defined in this document optional, so that profiles could use actual OAuth, which currently doesn't seem to be possible. Much of this draft written in a way that huge OAuth extensions are obliquely assumed when describing other features, without any clear definitions or justifications for them when they are introduced in passing. These things need to be teased apart, with references to sections actually defining and justifying each extension, rather than assuming that many of them exist in passing in other sections.
Detailed substantive comments on specific document text follow. Editorial nits are communicated in the accompanying pull request https://github.com/LudwigSeitz/ace-oauth/pull/112. Title The current title "Authentication and Authorization for Constrained Environments (ACE)" doesn't match the scope of what's in the spec, as the spec is only about resource authorization but not authentication. Please change the title to "Resource Authorization Framework for Constrained Environments" or something similar. (Authentication would require defining the equivalent of an OpenID Connect ID Token to convey authentication information, but this is entirely absent from the specification.) 1. Introduction The introduction defines what it means by "authorization" by doesn't say what it is referring to by "authentication". This provides more evidence that "authentication" does not belong in the title or abstract. Please remove the passing references to the underspecified concept "authentication" from the introduction and the abstract. (Note that while "OAuth client authentication" is a limited form of authentication, its use as a mechanism in the spec does not justify saying that the spec is defining authentication for its use cases in any general sense of the term.) 2. Terminology The "/" syntax for endpoint names, such as "/token" is misleading, as it seems to imply that these endpoints use particular pathnames. Please replace all uses of "/endpoint-name" with just "endpoint-name". 2. Terminology References are needed for the first uses of many of the terms and abbreviations used. For instance, you're using abbreviations like "AS", "RS", and "IoT" without saying what they refer to. 2. Terminology Please change all uses of the word "introspect" to "introspection" when referring to the introspection endpoint. 2. Terminology You introduce an "authz-info" endpoint in the terminology section, when this isn't commonly understood. You need a section reference following the first use of this concept. 3.1 OAuth 2.0 - token and introspect endpoints This section is the first of many places in which the text is written in a way that assumes that introspection will be implemented and used. This needs to be corrected throughout the document, appropriately qualifying descriptions of introspection saying that some profiles may use it and others will not. For instance, the draft says "The token introspection endpoint, /introspect, is used by the RS when requesting additional information regarding a received access token." Please change this so something more like "In some profiles, a token introspection endpoint may be present and used by the RS to request additional information about a received access token." You should also clearly state that introspection is unnecessary and adds overhead that can be avoided when the resource server understands the access token format, such as when it is a CWT. 3.1 OAuth 2.0 - Proof of Possession Tokens The draft uses the term "proof of possession token" to denote an access token supporting proof of possession. Note however, that there are many other kinds of proof of possession tokens other than just access tokens. Please change all uses of the term "proof of possession token" to "proof of possession access token" or "access token supporting proof of possession". Similarly, change all uses of the term "PoP token" to "PoP access token". 3.1 OAuth 2.0 - Scopes and Permissions This section contains another example of an extension to OAuth being assumed, when for many profiles it won't be necessary. The draft says "In turn, the AS may use the scope response parameter to inform the client of the scope of the access token issued." This assumes that a "scope" response will be added to all participating OAuth implementations, without justifying this addition to the standard. At most, this section should refer to another section which defines an optional "scope" response parameter and describes the circumstances in which profiles would and would not need it. 3.1 OAuth 2.0 - Scopes and Permissions Likewise, the draft says "As the client could be a constrained device as well, this specification uses CBOR encoded messages for CoAP". This is one of many places in the draft that it's assumed that CBOR and COAP will be used rather than JSON and HTTP. The framework draft should explicitly leave these choices up to profiles. 4. Protocol Interactions Please do not in any way endorse using the Client Credentials grant - which defeats the purpose of even having OAuth. The client should never have the user's credentials! At most, you should say that while the Client Credentials grant may be used for some scenarios, it has known security and privacy flaws and its use is deprecated. 4. Protocol Interactions The sentence "In such a case the resource owner or another person on his or her behalf have arranged with the authorization server out-of-band, which is often accomplished using a commissioning tool." needs clarification. What has been arranged? Figure 1: Basic Protocol Flow This is yet another place where a major OAuth extension is assumed, without clear justification. The diagram adds "+ RS Information" to the authorization server response to the client, when this isn't a normal part of OAuth and not defined. At least, please indicate that this is optional and refer to a section that defines and justifies this optional extension, rather than assuming in passing that it's been added. Figure 1: Basic Protocol Flow Please rework this diagram to illustrate that introspection is optional (and wastes resources by performing unnecessary communication). 4. Protocol Interactions - Access Token Response This also assumes the existence of "RS Information". At least, say that "Some profiles may choose to return information about the resource servers" and reference a section describing this optional feature, rather than write the text in a way that assumes its existence. 4. Protocol Interactions - Access Token Response This also assumes the existence of profile information in the response. This would normally be implicit. Again, please do not write the draft in a way that assumes the presence of extensions that are often not needed. 4. Protocol Interactions - Resource Request The two-part request described in the third paragraph is not OAuth, nor is there any justification for adding additional steps. Likewise, the language in paragraph 4 about "comparing the claims in the access token with the resource request" is a highly specific extension that is not normal OAuth. Please refactor this description to clearly delineate what's OAuth and what are optional extensions - referencing specific sections that define these extensions. 4. Protocol Interactions - Token Introspection Request Please rework this section to describe that introspection is optional (and wastes resources by performing unnecessary communication). Describe how things are simpler and more efficient when the resource directly understands the contents of the access token. 4. Protocol Interactions - Token Introspection Response This section adds yet another OAuth extension on the fly - the "client token" - again with no justification or clear definition. Please remove the oblique "client token" reference here. 5. Framework - Proof-of-Possession After "The binding is provided by the "cnf" claim" add references to [RFC 7800] and [draft-ietf-ace-cwt-proof-of-possession]. 5. Framework - Proof-of-Possession The text currently includes "update a token". Shouldn't this actually be "get a new token", since an existing access token can't be updated? 5.1 Authorization Grants This section includes the phrase "client credentials grant is recommended". Because of the security and privacy problems with the client credentials grant, please rework this section to ensure that implementers and profile writers understand that the use of client credentials grant is never recommended, including describing why that is the case. 5.1 Authorization Grants Reference [RFC 6749] after you say "see the OAuth 2.0 framework". 5.2 Client Credentials After you say that "the OAuth framework defines one client credential type", you should also describe that RFC 7522 and RFC 7523 define additional kinds of client credentials that may be used. 5.4 The Authorize Endpoint Change all occurrences of "authorize endpoint" to "authorization endpoint". 5.5 The Token Endpoint Rather than saying that "this framework extends", say that the framework defines optional extensions and reference their definitions and justifications. 5.5 The Token Endpoint Change "this framework defines encodings using CoAP and CBOR, as a substitute for HTTP and JSON" in a way that allows profiles to make either choice. 5.5.1 Client-to-AS Request This section is another place in which this specification is making assumptions about choices that rightfully belong to profiles. It starts with "The client sends a CoAP POST request". Profiles may choose to use CoAP or they may choose to use HTTP. Please reword this so that it says something more like "The client sends a CoAP or HTTP POST request, depending upon the profile". Please likewise generalize the description of the framework so that it's clear that the choice between CoAP or HTTP is up to the profile. Heck, Bluetooth Smart is another transport that's possible, which this spec shouldn't preclude! 5.5.1 Client-to-AS Request This is again written in such a way that it assumes that the additional parameters will be implemented and used, which won't be necessary for some profiles. It starts "In addition to these parameters, this framework defines the following parameters". Qualify this by saying that some profiles may choose to implement and use the following OAuth extension parameters. 5.5.1 Client-to-AS Request - aud The OAuth Token Exchange spec uses the "audience" parameter for this functionality. See https://tools.ietf.org/html/draft-ietf-oauth-token-exchange-09#section-2.1. Please reference this spec and use the same parameter name. 5.5.1 Client-to-AS Request - aud The spec says that "If a client submits a request for an access token without specifying an "aud" parameter, and the AS does not have a default "aud" value for this client, then the AS MUST respond with an error message". This violates the OAuth principle that implementations must ignore parameters that they do not understand. https://tools.ietf.org/html/rfc6749#section-3.1 says "The authorization server MUST ignore unrecognized request parameters". Rework this to say that it's perfectly legitimate for participants to ignore the "aud" parameter when they don't implement or understand it. 5.5.2 AS-to-Client Response Once again, extensions to OAuth are implicitly required - in this case the "profile" and "cnf" parameters. Reword to make these choices available to profiles. 5.5.2 AS-to-Client Response - profile This certainly shouldn't be required, as the profile knowledge will be implicit in most OAuth deployments. Very rare indeed will the cases in which participants will support multiple profiles. 5.5.2 AS-to-Client Response The spec says that "Figure 5 summarizes the parameters that may be part of the RS Information". However the term "RS information" is undefined. Instead, change this sentence to say "Figure 5 summarizes the parameters that may be part of the token response". 5.5.2 AS-to-Client Response - Figure 6 I suggest removing "profile" from the example, as this is unnecessary protocol bloat. 5.5.3 Error Response The spec says "The error codes MAY be abbreviated using the codes specified in table Figure 7". Again, whether to do this is a profile choice. Making it optional only makes all implementations bigger because they have to support both choices. Reword to say that profiles will specify whether error codes are abbreviated in this manner or not. 5.5.3 Error Response - Figure 7 These values should be in a registry - possibly called something like "OAuth error code CBOR value mappings". This registry should reference the values registered in the OAuth Extensions Error Registry at https://www.iana.org/assignments/oauth-parameters/oauth-parameters.xhtml#extensions-error. 5.5.4.1 Audience Reference the audience parameter in https://tools.ietf.org/html/draft-ietf-oauth-token-exchange-09#section-2.1 and indicate that whether it is used a profile-specific choice. 5.5.4.1 Audience The spec says that "It should be encoded as CBOR text string". Again, reword this to say that it's a profile choice whether to use CBOR or standard OAuth. 5.5.4.2 Grant Type Rather than saying "MAY", say that the profile specifies the representation that it used. 5.5.4.2 Figure 8 These values should be in a registry - possibly called something like "OAuth grant type CBOR value mappings". This registry should reference the sections defining these values in RFC 6749 and the grant type values defined by RFC 7522 and RFC 7523. 5.5.4.3 Token Type Reference draft-ietf-oauth-pop-key-distribution for the definition of the "pop" token_type. 5.5.4.3 Token Type When you say that "The values in the 'token_type' parameter MUST be CBOR text strings", you're again implicitly making choices that belong to profiles. Please generalize this description. 5.5.4.4 Profile Whether "profile" is even needed is up to the profile. Typically it will be implicit, since implementations will use a single profile. Please revise accordingly. 5.5.4.5 Confirmation Please replace the definition of "cnf" for CBOR here with references to RFC 7800 and draft-ietf-ace-cwt-proof-of-possession. 5.5.5 Mapping parameters to CBOR It looks to me like these values are intended to align with those registered in the CBOR Web Token (CWT) Claims registry initially populated by https://tools.ietf.org/html/draft-ietf-ace-cbor-web-token-08#section-9.1.2. If so, the spec should make this relationship explicit. However, it would be inappropriate to use the rare single-byte CBOR integer values for application-specific claim keys. I would suggest that the claim identifiers for client_id through refresh_token and profile start at 256 (a two-byte CBOR value) and go up from there. In that case, I suspect they could be successfully registered in the CWT Claims registry - which I think you want. ("cnf" will already be registered by draft-ietf-ace-cwt-proof-of-possession.) 5.6 The Introspect Endpoint First, change introspect to introspection. And like other places, state that it is up to the profile whether to use introspection at all, and if so, whether to use standard introspection syntax or the CoAP/CBOR version. 5.6.2 AS-to-RS Response - Figure 15 Remove profile and client_token from this example, since both are pretty esoteric and not likely to be used in the common case. 5.6.4 Client Token I agree with Hannes' review of this feature: "The "Client Token" is somewhat experimental and not on par with the rest of the document in terms of maturity and alignment with OAuth. I would prefer this functionality to be covered in a separate document, if someone still cares about it. While OAuth has seen a lot of formal analysis this feature obviously hasn't." Please remove this from the specification and if you still believe in it, place it in a separate document for independent consideration by the working group. 5.6.4 Client Token The spec says "The client is pre-configured with a generic, long-term access token when it is commissioned." This hardly seems secure - especially since secrets can often be extracted from deployed software. 5.6.4 Client Token The spec says "The RS then performs token introspection to learn what access this token grants." Again, this is unnecessary if the resource understands the claims in the access token. 5.6.5 Mapping Introspection Parameters to CBOR As in my related comment on 5.5.5, it looks to me like these values are intended to align with those registered in the CBOR Web Token (CWT) Claims registry initially populated by https://tools.ietf.org/html/draft-ietf-ace-cbor-web-token-08#section-9.1.2. If so, the spec should make this relationship explicit. However, it would be inappropriate to use the rare single-byte CBOR integer values for application-specific claim keys. I would suggest that the claim identifiers for client_id through refresh_token and profile start at 256 (a two-byte CBOR value) and go up from there. In that case, I suspect they could be successfully registered in the CWT Claims registry - which I think you want. ("cnf" will already be registered by draft-ietf-ace-cwt-proof-of-possession.) 5.7 The Access Token Reference [RFC 7800] and [draft-ietf-ace-cwt-proof-of-possession] when describing the use of "cnf". 5.7.1 The Authorization Information Endpoint Please describe the relationship between this endpoint and the resource endpoint used by RFC 6750. 5.7.1 The Authorization Information Endpoint Again, please clarify that support for this OAuth extension, the use of CoAP, and the use of introspection are all choices up to particular profiles. 5.7.1 The Authorization Information Endpoint The spec says "The RS MUST be prepared to store more than one token for each client, and MUST apply the combined permissions granted by all applicable, valid tokens to client requests." This seems really overly specific for a framework spec. Simple systems will likely only have one token per client, let alone not supporting complex permission combination logic! Please remove this statement. 5.7.2 Token Expiration Rather than saying "CWT/JWT" expand this to "CWT or JWT" for readability. 6. Security Considerations Add a reference upon first use of the term "AEAD". 7. Privacy Considerations The spec says "The latter may reveal the client's identity." What is meant by the client's identity? What kinds of information does this entail and what are the privacy risks associated with it? How does the client's identity relate to the identities of people who may be associated with the system in some way? 8.1 OAuth Introspection Response Parameter Registration Every place an IANA registration currently says "this document", please change it to "Section x.y.z of this document" (using the appropriate <ref target="x.y.z"/> tag for the section that defines the value. 8.1 OAuth Introspection Response Parameter Registration "aud" is already registered at https://www.iana.org/assignments/oauth-parameters/oauth-parameters.xhtml#token-introspection-response. 8.4 Token Type Mappings The name "Token Type Mappings" registry is too generic. Please change it to "OAuth Token Type CBOR Mappings" or something similar. 8.4.1 Registration Template As was pointed out in comments on earlier versions of the CWT spec, the range 1 to 65536 makes no sense. Please consider using the same treatment of value ranges as CWT does (which themselves derived from the COSE usage of value ranges). Do this consistently every place that "1 to 65536" occurs in the spec. 8.5 CBOR Web Token Claims Consider using the "scp" claim defined at https://tools.ietf.org/html/draft-ietf-oauth-token-exchange-09#section-4.2 rather than "scope". If you don't, at least say why you are introducing a different claim to convey the same information. 8.5 CBOR Web Token Claims "cnf" is already being registered by draft-ietf-ace-cwt-proof-of-possession. 8.6 ACE Profile Registry The name "ACE Profile Registry" registry is too generic. Please change it to "ACE OAuth Profile Registry" or something similar. 8.7 OAuth Parameter Mappings Registry The name "OAuth Parameter Mappings" registry is too generic. Please change it to "OAuth CBOR Parameter Mappings" or something similar. 8.7.2 Initial Registry Contents Per my earlier comments, these values should actually reference the CWT Claims registry and application-specific values such as "client_id", etc. should not use the scarce single-byte value range. 8.8.1 Registration Template Registrations for the Introspection Endpoint CBOR Mappings registry should contain a field that lists the corresponding value registered in the OAuth Token Introspection Response registry at https://www.iana.org/assignments/oauth-parameters/oauth-parameters.xhtml#token-introspection-response. 8.10 CWT Confirmation Methods Registry Delete this section, as it has been moved to draft-ietf-ace-cwt-proof-of-possession. Appendix A. Design Justification - Communication Constraints Add that power saving may be another reason for communication constraints. Appendix B. Roles and Responsibilities - Authorization Server Add that enabling discovery of the AS's capabilities via metadata is likely in scope. Reference draft-ietf-oauth-discovery. Appendix B. Roles and Responsibilities - Client What do the parenthetical letters such as "(A)" refer to? Why are "(D)" and "(E)" missing? Appendix C. Requirements on Profiles You should be much more clear in this section that choices of JSON vs. CBOR, JWT vs. CWT, HTTP versus COaP, PoP versus Token Binding, Introspection or not, authz-info endpoint or not, etc. must be made by profiles. Appendix E. Deployment Examples - Figure 20 Use the "scp" claim defined at https://tools.ietf.org/html/draft-ietf-oauth-token-exchange-09#section-4.2 rather than "scope". Appendix E.2 Introspection Aided Token Validation It makes no sense to assume that the client is not able to access the AS at the time of the access request but can access the introspection endpoint, since the introspection endpoint is part of the AS. Please remove this section or revise accordingly. Appendix E.2 Introspection Aided Token Validation - Figure 23 Please revise the example to not use Client Credentials, because of the security and privacy problems associated with this grant type. Surprising Omission of Token Binding! The specification should describe the ability to use Token Bound access tokens, rather than PoP access tokens, as this will be substantially simpler for most implementations. Please reference draft-ietf-oauth-token-binding and describe how to apply it to this specification. Best wishes, -- Mike
_______________________________________________ Ace mailing list [email protected] https://www.ietf.org/mailman/listinfo/ace
