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

[Justin Richer <[e-mail]>]

Thank you, Roman! For the one remaining question from our side:


   locations (array of strings):  The location of the RS as an array of

      strings.  These strings are typically URIs identifying the

      location of the RS.





If these strings aren’t URIs, how else would the RS location be defined?  The text here seems to suggest that the locations could be something other than a URI, is that intended?  If it’s not a URI, what would it be?



It’s a logical identifier that the RS is known by, not necessarily the URL that’s being used to access it. Since an RS is a functional role, not just a web server, sometimes a URI isn’t the appropriate identifier for an RS. For example, if the same service is offered at multiple distinct servers, but the authorization is the same across all of them, then the “location” field would have the logical AS name and not the physical server URI.


These could take the form of a random string assigned by the AS, a tenancy identifier from a customer account, a developer-friendly label, etc. Each of these is something that the RS software care recognize itself when processing the access token. A URI is going to be a common way to label an RS,  but it’s far from the only reasonable way.


In the OAuth 2 version of this structure (RFC 9396, OAuth RAR), the “locations” field is defined as:


locations:
An array of strings representing the location of the resource or RS. These strings are typically URIs identifying the location of the RS. This field can allow a client to specify a particular RS, as discussed in Section 12<https://datatracker.ietf.org/doc/html/rfc9396#security_considerations>.


Section 12 is the security considerations section, which only has this to say about processing the field at the RS:


The common data field locations allows a client to specify where it intends to use a certain authorization, i.e., it is possible to unambiguously assign permissions to RSs. In situations with multiple RSs, this prevents unintended client authorizations (e.g., a read scope value potentially applicable for an email as well as a cloud service) through audience restriction.


The intent is to be compatible with this spec (really, RAR is a back port of this structure, but it was finished first), and to provide API developers with significant flexibility in how to address and manage their RS’s.

 — Justin


On Oct 29, 2023, at 6:32 PM, Roman Danyliw <[e-mail]> wrote:

Hi Justin and Fabien!

Thanks for all of the work you and the WG did to respond to my AD review.  The -16 is appreciated and addresses the overwhelming majority of my feedback.  I’m sending the document to IETF LC.  There is only the following residual feedback to sort out with LC.

==[ snip ]==


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



** Section 3.5.  Is there any minimum guidance on the length of an instance_id?



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



[Roman follow-up on all of these] Per your response, let me ask around on what might be good guidance.



** Section 8.  locations.



   locations (array of strings):  The location of the RS as an array of

      strings.  These strings are typically URIs identifying the

      location of the RS.





If these strings aren’t URIs, how else would the RS location be defined?  The text here seems to suggest that the locations could be something other than a URI, is that intended?  If it’s not a URI, what would it be?

==[ snip ]==

Thanks,
Roman


From: Justin Richer <[e-mail]>
Sent: Wednesday, October 18, 2023 12:36 PM
To: [e-mail]
Cc: [e-mail]; Fabien Imbault <[e-mail]>
Subject: Re: [GNAP] AD review of draft-ietf-gnap-core-protocol-15

Hi Roman,

The editors have merged the PR and intend to publish an update to the draft before the deadline on Monday. When you can, we’d appreciate answers to the outstanding questions below. We’d like to address as many as we can in the next draft.

Thank you,
 — Justin


On Oct 3, 2023, at 5:24 PM, Justin Richer <[e-mail]<mailto:[e-mail]>> wrote:

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
--
TXAuth mailing list
[e-mail]<mailto:[e-mail]>
https://www.ietf.org/mailman/listinfo/txauth