Re: [GNAP] AD review of draft-ietf-gnap-core-protocol-15

[Justin Richer <[e-mail]>]

Hi Roman, thanks for the thorough review, and I wanted to confirm that we saw it. Fabien and I going to go through all of these items. On a quick skim I don’t see anything shocking, but I know we need to do a more deep and thorough read through before responding.

Thanks again, it’s a big document and a lot to cover!

 — Justin

> On Sep 6, 2023, at 6:23 PM, Roman Danyliw <[e-mail]> wrote:
> 
> Hi!
> 
> I performed an AD review of draft-ietf-gnap-core-protocol-15.  Congrats on getting this core deliverable out of the WG.  This took a tremendous amount of work.  I appreciate everyone's patience as I reviewed this document.  Below is my feedback and it contains no show-stopping protocol feedback.  The majority of it points to places where additional clarity and process would be helpful.
> 
> 
> ** Section 1.  Editorial.
>   GNAP
>   and OAuth 2.0 will likely exist in parallel for many deployments, and
>   considerations have been taken to facilitate the mapping and
>   transition from legacy systems to GNAP. Some examples of these can
>   be found in Appendix C.5.
> 
> The example in Appendix C.5 appears be constructed around OAuth 2.0.  Is this text suggesting that OAuth is somehow a legacy protocol?  Please rephrase.
> 
> ** Section 1.2.  Editorial
>      The
>      client is operated by the end user or it runs autonomously on
>      behalf of a resource owner.
> 
>      Example: a client can be a mobile application, a web application,
>      etc.
> 
> These examples seem like ones where there is a end-user.  What is an example of client which runs autonomously on behalf of the resource owner?
> 
> ** Section 1.2.  Editorial.
> Resource Server (RS):  server that provides operations on protected
> Resources
> 
> “Provides operations” is difficult to parse.  How is this definition different than OAuth’s in RFC6749:
> 
> ==[ snip ]==
>      The server hosting the protected resources, capable of accepting
>      and responding to protected resource requests using access tokens.
> ==[ snip ]==
> 
> ** Section 1.3.  At the level of detail provided here, what is the difference between an “attribute” and “subject information”?  
> 
> ** Section 1.4.  Editorial.
> Client developers promise to implement
>      requirements and generally some recommendations or best practices,
>      so that the end users may confidently use their software.
> 
> Is that true?  Do developers of applications make such promises?
> 
> ** Section 1.4
>      However, end users might also be facing some attacker's client
>      software, without even realizing it.
> 
> -- Editorial. s/also be facing some attacker’s/also be facing an attacker’s/
> 
> -- Isn’t the end user also equally exposed to incorrectly or poorly implemented clients
> 
> ** Section 1.4
>   *  end user/AS: when the client supports it (see Section 3.3), the
>      end user gets to interact with front-channel URIs provided by the
>      AS. 
> 
> -- Please define the concept of a “front channel URI”?
> 
> -- How does GNAP work is the end user doesn’t engage the AS? I ask because the text is written … “when the client support its …”, which would suggest a client at time when it does not.  What happens then?
> 
> ** Section 1.4
>   *  RS/RO: the RS promises it protects its resources from unauthorized
>      access, and only accepts valid access tokens issued by a trusted
>      AS.  In case tokens are key bound, proper validation is expected
>      from the RS.
> 
> If this text is about the RO trusting the RS, none of the text here mentions the RO.
> 
> ** Section 1.5.  Editorial.  Consider providing a forward reference for “continuation access token”?
> 
> ** Section 2.3
>   Both the display and class_id are self-declarative and thus the AS
>   SHOULD exercise caution in their interpretation, taking them as a
>   hint but not as absolute truth.
> 
> -- Is “self-declarative” the same as “free-form”?  Interoperability is not expected and it’s use requires further profiling?
> 
> -- Since SHOULD is used, when would the AS NOT exercise caution?
> 
> ** Section 2.3.  Editorial.
> 
> The class_id field can be used in a
>   variety of ways to help the a variety of ways to help the AS make
>   sense of the particular context in which the client instance is
>   operating.
> 
> s/can be used in a variety of ways to help the a variety of ways/can be used in a variety of ways to help/.
> 
> ** Section 2.3
> it SHOULD return an
>   invalid_client error (Section 3.6) or choose to return lesser levels
>   of privileges.
> 
> Given the significant flexibility afforded to the “class_id”, is there confidence returning a lesser level of privilege is a normative SHOULD?
> 
> ** Section 2.3
>   If the AS does not know the client instance's public
>   key ahead of time, the AS MAY accept or reject the request based on
>   AS policy, attestations within the client request, and other
>   mechanisms.
> 
> Isn’t “AS policy” all encompassing and already covers “attestations within the client request and other mechanisms”?
> 
> ** Section 2.3
>   A client instance that is capable of talking to multiple AS's SHOULD
>   use a different key for each AS to prevent a class of mix-up attacks
>   as described in Section 13.30.
> 
> When would it be a prudent or necessary for the client to use the same key across ASs?  Put in another way, why this flexibility?
> 
> ** Section 2.4.  Editorial.  Please provide an internal references to the format of “assertions”.
> 
> ** Section 2.4.  Editorial.  Expand “IdP”.
> 
> ** Section 2.5.  Editorial
>   In this non-normative example, the client instance is indicating that
>   it can redirect (Section 2.5.1.1) the end user to an arbitrary URI
>   and can receive a redirect (Section 2.5.2.1) through a browser
>   request.
> 
>   "interact": {
>       "start": ["redirect"],
>       "finish": {
>           "method": "redirect",
>           "uri": "https://client.example.net/return/123455",
>           "nonce": "LKLTI25DK82FX4T4QFZC"
>       }
>   }
> 
> The example after this one explicitly mentions that this client cannot accept a redirect or push callback.  Isn’t push call back also not supported here?
> 
> ** Section 2.5
> 
>   In this non-normative example, the client instance is indicating that
>   it can display a user code (Section 2.5.1.3) and direct the end user
>   to an arbitrary URI (Section 2.5.1.1) on a secondary device, but it
>   cannot accept a redirect or push callback.
> 
>   "interact": {
>       "start": ["redirect", "user_code"]
>   }
> 
> Which part of this example suggests a secondary device?  The definition of “redirect” and “user_code” in Section 2.5.1.1 make no reference to a secondary device.
> 
> ** Section 2.5
> 
>   If the client instance does not provide a suitable interaction
>   mechanism, the AS cannot contact the RO asynchronously, and the AS
>   determines that interaction is required, then the AS SHOULD return an
>   invalid_interaction error (Section 3.6) since the client instance
>   will be unable to complete the request without authorization.
> 
> The sending of an error message seems to be optional.  When is a silent failure acceptable?
> 
> ** Section 2.5.2.  nonce.  Is there a minimal or recommended nonce size?
> 
> ** Section 2.5.2.  Editorial.  s/the the/the/
> 
> ** Section 2.5.2.1 and 2.5.2.2.
> 
> -- method=redirect says, “The client instance's URI MUST be protected by HTTPS, …”
> 
> -- method=push says, “The client instance's URI MUST be an HTTP URI and MUST be protected by HTTPS or equivalent”
> 
> What is the difference in intent?
> 
> What is envisioned with “equivalent”?  This phrase is used a few other times.
> 
> ** Section 3.  Editorial. s/eg,/e.g.,/
> 
> ** Section 3.1.  Editorial. s/an an/an/
> 
> ** Section 3.3.4.  Please use one of the example domains instead of  “srv.ex”
> 
> ** Section 3.5.  Is there any minimum guidance on the length of an instance_id?
> 
> ** Section 4.
>   The client instance MUST use each interaction method at most once.
>   The AS SHOULD handle any interact request as a one-time-use mechanism
>   and SHOULD apply suitable timeouts to any interaction start methods
>   provided, including user codes and redirection URIs.  The client
>   instance SHOULD apply suitable timeouts to any interaction finish
>   method.
> 
> There is a nuance of the client instance being constrained to using an interaction only once (MUST) but the AS seemingly having some flexibility.  Could this be clarified?
> 
> ** Section 4.1.
>   If the client instance does not start an interaction start mode
>   within an AS-determined amount of time, the AS SHOULD reject attempts
>   to use the interaction start modes.
> 
> If the AS sets a policy that the interaction mode needs to occur in a determined amount of time, why wouldn’t the AS reject the start attempt?
> 
> ** Section 4.1.2 and 3.3.3.  Could the text please be clearer on the permitted “alphabet” for the user code.
> 
> -- Section 3.3.3 says “To facilitate usability, this string MUST consist only of characters that can be easily typed by the end user (such as ASCII letters or numbers)”
> 
> -- Section 4.1.2 says “To facilitate this, it is RECOMMENDED that the AS use only ASCII letters and numbers as valid characters for the user code.”
> 
> The guidance in Section 3.3.3 seems subjective and doesn’t seem like it would lead to consistent implementations.
> 
> Same comments apply for Section 4.1.3.
> 
> ** Section 4.1.2.
>  As a
>   consequence, these URIs SHOULD be short.
> 
> I understand the spirit of this guidance but “short” is subjective.  Please be more specific or use alternative language.
> 
> ** Section 4.1.3.  Editorial.  Much of this guidance seems to be verbatim repetition of section 4.1.2.  Is that needed?
> 
> ** Section 4.2.
>   If a
>   finish method is not available, the AS SHOULD instruct the RO to
>   return to the client instance upon completion.
> 
> What is the alternative to this approach if a finish method is not available?
> 
> ** Section 4.2.2.  Does the interact_ref share the same properties as the nonce defined in Section 2.5.2.  For example, is it an ASCII string?  Does it use the whole ASCII alphabet?
> 
> ** Section 5.  Typo. s/contination/continuation/
> 
> ** Section 5 and 9.1
>   POST /continue/KSKUOMUKM HTTP/1.1
>   Authorization: GNAP 80UPRY5NM33OMUKMKSKU
>   Host: server.example.com
>   Content-Length: 0
>   Signature-Input: sig1=...
>   Signature: sig1=...
> 
> This example (and a number of them later) appear to be showing a new “GNAP HTTP Authentication Scheme” (i.e., “Authorization: GNAP 80UPRY5NM33OMUKMKSKU”). Section 9.1 also references “responding with the WWW-Authenticate header field and a GNAP challenge.” 
> 
> Where is the GNAP authentication scheme formally described and registered? https://www.iana.org/assignments/http-authschemes/http-authschemes.xhtml has no such registration and there no IANA action in this document to add it.
> 
> ** Section 5.4
>   The AS SHOULD revoke all associated
>   access tokens, if possible.  The AS SHOULD disable all token rotation
>   and other token management functions on such access tokens, if
>   possible.
> 
> Wouldn’t the use of SHOULD instead of MUST here present an issue.  If a client issues a “DELETE” for its access tokens with an AS how does it know that all of it’s tokens have been removed if this flexibility is provided?
> 
> ** Section 6.1
>   To rotate an access token, the client instance makes an HTTP POST to
>   the token management URI with no message body, sending the access
>   token in the appropriate header and signing the request with the
>   appropriate key.
> 
> What is the “appropriate header” here?
> 
> ** Section 6.1
>   If the AS is unable or unwilling to rotate the value of the access
>   token, the AS responds with an invalid_rotation error (Section 3.6).
>   Upon receiving such an error, the client instance SHOULD consider the
>   access token to not have changed its state.
> 
> Can the second sentence be clarified?  When would a rejection translate to state change?
> 
> ** Section 7.3.  Typo. s/defines defines/defines/
> 
> ** Section 7.3
>   Key binding method definitions SHOULD
>   enumerate how these requirements are fulfilled.
> 
> -- Is this guidance to the Designated Expert who is going to review future key binding methods?
> 
> -- If so, why would it be acceptable for a new key binding method NOT to explain how it fulfills its requirements?
> 
> ** Section 7.3.1
> 
>   The signer
>   SHOULD include the nonce parameter with a unique and unguessable
>   value.  When included, the verifier MUST determine that the nonce
>   value is unique within a reasonably short time period such as several
>   minutes.
> 
> How does the verifier determine that the freshness of the nonce to a “short time period”?
> 
> ** Section 8.  Should it be explicitly said that no interoperability across types?
> 
> ** Section 8.  Typo. s/the the/the/
> 
> ** Section 8.
>  Specific API
>   implementations SHOULD re-use these fields with the same semantics
>   and syntax.
> 
> Why wouldn’t deviations from the semantics defined here require custom labels from being defined?
> 
> ** Section 8.  locations.  If these strings aren’t URIs, how else would the RS location be defined?
> 
> ** Section 8.
> 
>   The access requested for each object in the array is the cross-
>   product of all fields of the object. 
> 
> Is this statement explaining the example or a general statement about interpreting each object in the array?
> 
> ** Section 9.1
>   The opaque access reference MUST be sufficient for at least
>   the action the client instance was attempting to take at the RS and
>   MAY be more powerful.
> 
> Could this guidance be clarified? How is this access reference have any “power” beyond being an identifier that needs to brought to the AS?
> 
> ** Section 11.  It appears that all of the registries in Section 11.* have a mandatory column to reference the specification document.  Given that requirement, why is the registration policy not “Specification Required” which also includes “Expert Review”?  
> 
> ** Section 11.  The OAuth registries (FWIW, all OAuth registries are specification required -- https://www.iana.org/assignments/oauth-parameters/oauth-parameters.xhtml.) were all very particular about having a change control column.  Do the circumstances that necessitated that apply to GNAP?
> 
> ** Section 11.4.2.  Is citing Section 2.2 and 3.4 for id_token and saml2 the right approach?  Those two sections don’t seem to provide anything specific for those mechanisms? Why isn’t the specification document [OIDC] And [SAML2]?  
> 
> ** Section 13.1
> 
> All requests in GNAP have to be made over TLS or equivalent as
>   outlined in [BCP195]
> 
> “or equivalent” doesn’t mean anything relative to BCP195 which is scoped to (D)TLS.  What is meant by “equivalent”? 
> 
> ** Section 13.1.
> 
> … and any
>   back-end communications such as from an RS to an AS as described in
>   [I-D.ietf-gnap-resource-servers]
> 
> It seems odd to provide normative requirements for another draft.
> 
> ** Section 13.1.  Editorial.  This section seems to repeat itself a lot.
> 
> (a)    All requests in GNAP have to be made over TLS or equivalent as
>   outlined in [BCP195] to protect the contents of the request and
>   response from manipulation and interception by an attacker.  This
>   includes all requests from a client instance to the AS, all requests
>   from the client instance to an RS, any requests back to a client
>   instance such as the push-based interaction finish method, and any
>   back-end communications such as from an RS to an AS as described in
>   [I-D.ietf-gnap-resource-servers].  Additionally, all requests between
>   a browser and other components, such as during redirect-based
>   interaction, need to be made over TLS or use equivalent protection.
> 
> (b)    The use of key-bound access tokens does not negate the requirement
>   for protecting calls to the RS with TLS.  
> 
> (c)    TLS or equivalent protection also needs to be used between the
>   browser and any other components.  
> 
> The text in (a) seems helpfully clear that all GNAP communication regardless of the parties needs to be over TLS.  (b) and (c) seem to repeat that.
> 
> ** Section 13.3.  Editorial.  Expand “SPAs”.
> 
> ** Section 13.18.
> 
>   PKI has historically been difficult to deploy, especially at
>   scale, but it remains an appropriate solution for systems where the
>   required overhead is not an impediment.
> 
> Recommend against this statement without citation or being specific about client-oriented certificates.  Server certificates in the WebPKI, especially since ACME seems to going quite well.
> 
> ** Section 13.23.  “… (in most cases over HTTP)”, should this be HTTPS?
> 
> ** Section 13.24.  Typo. s/defence/defense/
> 
> ** Section 13.24.  Typo. s/Prerequesits/Prerequisites/
> 
> ** IDNITs reports:
> 
>  == Outdated reference: draft-ietf-httpbis-client-cert-field has been
>     published as RFC 9440
> 
> -- 
> TXAuth mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/txauth