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

[Justin Richer <[e-mail]>]

Hi Roman, I’ve made a pull request that hopefully addresses the majority of your comments:

https://github.com/ietf-wg-gnap/gnap-core-protocol/pull/513

I’ve snipped out a handful of places where no changes were made or where I had additional questions about the approach. Thanks again for the thorough review!

-- 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?

This is most notably the autonomous case, where the end-user (if there is one) doesn’t talk to the AS, as well as cases where no additional interaction is necessary (the client instance can provide attestations or other claims directly to the AS, for example a VC proof from a wallet), or the asynchronous case where the end-user and resource owner are different and the RO can approve the end-user’s request out of band from the GNAP protocol flow. The “when supported” is meant to echo text about interaction methods, as GNAP does not define a mandatory set of interactions.

We didn’t change the text here, but the upshot is that there isn’t the same kind of relationship between the end user and the AS and so the promises don’t really apply. Should this be called out?


** 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?

I believe so - especially because it’s going to be very API-specific what a “lesser level” is going to mean for any AS.


This was expanded to explicitly call out the fact that the AS could ignore a class_id it doesn’t understand and treat the client software as if it had never heard of it.



** Section 2.5.2.  nonce.  Is there a minimal or recommended nonce size?

I don’t have one but I’m open to suggestion on what good guidance would be here, especially if we can point to another document to establish that.


I don’t have good advice for what random values like nonce, instance_id, interaction_ref, or the like would want to have in them.


What is envisioned with “equivalent”?  This phrase is used a few other times.

This is a pattern I’d pulled from other specs to allow for protections other than TLS, like a VPN tunnel or a localhost connection -- something where you could use a plain HTTP URL that you know the connection wouldn’t be sniffed otherwise.

This was called out explicitly in a number of places where similar language was used.


** Section 3.3.4.  Please use one of the example domains instead of  “srv.ex”

I was trying to show a shortened URL as an example of what you’d expect here. Do we have an example domain that’s fairly short? All of “example.com<http://example.com/>”, “example<http://example.com/example>.net”, and “foo.example” feel too long for this. If these are the only real options, we can use one.

I ended up with “s.example” but would love to be able to have something even shorter if anyone knows of a good candidate.



** 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.

Since this is a user-facing value that we expect people to type in by hand, it really should be contextual to whatever the target deployment is. For a lot of folks that’s printable US-ASCII, but that’s not a universal assumption, I think. From a protocol perspective, it’s a JSON string, so as long as the string can be displayed and entered unambiguously, it should be fine. It’s the AS that generates and receives the code, so the AS gets to make the decision of what goes in it. The considerations here are somewhat subjective because you need to account for people.

I think in practice, the US-ASCII recommendation is what we’ll likely see.

We’ve expanded the discussion here to say that it’s really about consideration of what the user needs to read and type, hopefully this is more clear now.


** Section 4.1.3.  Editorial.  Much of this guidance seems to be verbatim repetition of section 4.1.2.  Is that needed?

That seems right - these were originally one section with options, now it’s two sections. Unless we wanted to extract a common section again, I don’t think it’s good to cut down the text. Especially if an implementor is going to read just the one piece that they’re building, we’d want them to have all the guidance right there.


This was left as it stood for the reasons stated.



** 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?

Sometimes it’s not possible to proactively revoke a token already in flight, depending on the nature of the deployment of the overall system. GNAP doesn’t presume stateful or stateless access tokens as the result of the delegation process, so it’s revoke-if-possible, which to me feels like a SHOULD. Even in that case, it’s possible for the AS want to treat the grant request and its results separately — so revoking the grant request could still leave the tokens around as artifacts. You can’t alter the grant or use it to make new tokens, but the old tokens could time out and die of natural causes.

The client never can know for sure if all the tokens have been deleted, it can only make a best effort request to delete them, and then stop using those tokens itself. The same kind of pattern is found in OAuth’s token revocation (RFC7009) which this piece (6.2) is based on.

This was left as a SHOULD if possible.



** 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”?

This is meant to be guidance for implementors of the nonce check. We don’t expect people to store all nonces for all time, but instead to have a bunch around for a reasonable time period to prevent replay within that window. Can you suggest a better way to accomplish that?


Would appreciate some input on this, along with the nonce and other random value length requirements.



** 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?

Because ultimately they’re custom to the API. The common fields are provided as guidance for common usage. If an API designer wants to use “actions” to represent something else entirely, that’s not going to affect interoperability with other APIs, it’ll just be confusing for developers to make sense of.


We removed the normative SHOULD here, and like RAR are silent on the use of these in API design.



** 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?

I don’t know the story of why that was required in OAuth or whether it would also be required here. I would appreciate input from those that know better how to manage this.

I haven’t received any feedback on this, so I’m inclined to believe the same circumstances do not apply.


** 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.


Thanks, we’ll review and see if we can trim this down. We were trying to make it explicitly clear that “all” means “all”. I do think it’s worthwhile to point out (c) as a developer might not consider the browser-to-service connections as part of the protocol proper, but they need to account for this.

I didn’t see a good way to remove the repetition, apart from a little bit of editorial cleanup. The (a) is the requirement, the (b) and the (c) are more specifics as to why (a) is the requirement.




 — Justin