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

[Roman Danyliw <[e-mail]>]

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