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

[Justin Richer <[e-mail]>]

Hi Roman, thanks again for the thorough review. I’ve read through everything and tried to provide answers inline below. Pretty much everything here makes sense, and most of it is easily actionable so we can get started on a PR for those. Some questions remain in the responses below.

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.


The goal of this text (both the intro and the appendix) are to position how GNAP relates to OAuth 2. While it isn’t wire-compatible, it is in many ways structurally-compatible as a design goal. We can rephrase this to avoid the potentially-weighted term “legacy” here.

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

Good catch, we can add one to the list: A back-end service would be the best example here, something that you’d traditionally use an API key or the OAuth Client Credentials grant type for.


** 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 ]==

One of the problems with the definition in 6749 in practice is that it lends itself to a limited interpretation of what an RS is - ie, a data store kind of thing. “Provides operations on” was meant to imply that it’s really any kind of API. That said, I think we can rework the 6749 definition a bit to be more inclusive and also clearer here.


** Section 1.3.  At the level of detail provided here, what is the difference between an “attribute” and “subject information”?

Reading this now, I can’t recall what the goal for separating them was. A quick scan through the document shows that “attribute” is not used in this way outside of this section, so I think we can remove it from the list and adjust the other definitions.


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

I think in this case that’s true for honest client applications. The use of “promise” here is to align with the overall terminology of “promise theory” that defines a mathematical relationship between entities in this section. That said, I think that the trust relationship being talked about is more of a line between the client software and the end user, so we’ll adjust that to be less confusing.


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


Got it, thanks.

-- Isn’t the end user also equally exposed to incorrectly or poorly implemented clients

Good call, will add.


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


That usage is confusing, I think we just meant any type of direct interaction that the end-user might have with the AS, which is normally going to be through a browser of some type but need not be so. I think we can change that to just “interact with the AS through an AS-provided interface”.

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


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

The implication is that it’s doing so :for: the RO, so we can call that out explicitly.


** Section 1.5.  Editorial.  Consider providing a forward reference for “continuation access token”?

Good catch, thank you.


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

Correct - these are declarations from the client and we don’t expect interop on these values (their format, content, or anything) without additional profiling. Is there a better way to say that? This is discussed further in section 13.13 which is forward-linked.


-- Since SHOULD is used, when would the AS NOT exercise caution?

When there are additional profiles, such as client attestations, that can provide additional trust for such values. I think we can instead state this as something like:

> Absent additional attestations, profiles, or trust mechanisms, the AS MUST exercise caution in interpreting these values, treating them as hints and not absolute truth.


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

Got it, thanks!


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


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

I personally read “AS policy” in terms of policy engines and other codified things. I agree that the phrase does more generally encompass these other elements, but I think that it still makes sense to call them out. How about:

> based on attestations within the client request or other AS policy decisions.


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

When the client has a hardcoded key, or when it’s provisioned in a trusted framework. Some environments, especially the kinds of systems we’re starting to discuss in WIMSE, don’t have the same software-identity issues that we usually think of in the delegation protocol space here. In those environments, you’ve got other controls around key distribution and service identity that protect you from the mix-up attacks. Those aren’t mentioned in 13.30 at this time, so maybe we can expand that and other related sections like 13.10.


** Section 2.4.  Editorial.  Please provide an internal references to the format of “assertions”.

OK, will do - this will point to §3.4.


** Section 2.4.  Editorial.  Expand “IdP”.

Got it, thanks!


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


It’s simple enough to mention that - I personally think it’s a little overkill, since the client can only declare one method of callback.

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


As with the OAuth Device Code grant type, it doesn’t :have: to be a secondary device but it’s well suited to that. If it were on the same device, a redirect is more likely because it’s more probable for the client to be able to launch an arbitrary URL.

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

I think the original thinking here was that the AS can send back an empty interaction response to the client - but I think any recoverable case is already covered by the conditions around using this error. So It probably makes more sense for this to be a MUST, we’ll change 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.


** Section 2.5.2.  Editorial.  s/the the/the/

Got it, thanks!


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


There wasn’t supposed to be any difference here, this was just sloppy editing. We’ll align 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.


** Section 3.  Editorial. s/eg,/e.g.,/

Got it, thanks!


** Section 3.1.  Editorial. s/an an/an/

Got it, thanks!


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


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

It would need to be long enough to be unique per the AS, but that’s about it. Since it doesn’t contain key material, it doesn’t need to be completely unguessable.


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

The flexibility on the AS side was meant to allow for things like network timeout recovery and idempotent processing of methods. We run into similar issues in the OAuth world with the Authz code being one-time-use but in practice being kept around for a moment to allow for recovery if the result of the auth code (the token) doesn’t complete its delivery.

What if we made it MUST on both sides, but allowed for idempotent processing specifically? If you allow the client to use the same mechanism more than once you have to return the same results, and you should have some assurances that it’s the same actors (ie, not coming in on a different session/device).


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

That makes sense, this can be a MUST since it’s the AS making the decision, we’ll change that.


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


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

Same as above, it needs to be relative to the environment. Maybe we can give some examples, like “20 characters long” or the like?


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


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

Polling, as per section 5.2. The “finish” is a signal to the client to continue the grant request. In the absence of any such signal, the client can keep checking on its own.


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


Good question - I think since it’s a URL parameter the recommended alphabet is actually less. We can make this only query-parameter-safe characters, does that make sense?

** Section 5.  Typo. s/contination/continuation/

Thanks, got it!


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

Good catch, we’ll add an IANA section to register these both.


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


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

This was meant to allow different kinds of token presentation methods to be defined in the future. This should just point at 7/2 and 7.3 instead, we’ll change that.


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

I think MUST might actually be more appropriate here - as far as the client knows, the token’s unmodified. We’ll change that.


** Section 7.3.  Typo. s/defines defines/defines/

Got it, thanks!


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

Good point, I think this needs to  be a MUST. We’ve got similar requirements language for the signature methods in HTTP Sig that I think we can pull over, we’ll see if that fits.


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


** Section 8.  Should it be explicitly said that no interoperability across types?

We can do something like that  - I’d like to align with RAR (RFC9396) as much as we can, since that’s the OAuth2 version of this same structure.


** Section 8.  Typo. s/the the/the/

Got it, thanks!


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


** Section 8.  locations.  If these strings aren’t URIs, how else would the RS location be defined?

The location could be an internal identifier known between the AS and RS, for example.


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

It’s explaining the example, but the example represents the generally-expected interpretation of the items. We’re trying to show here how an API designer can fit the pieces together.


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

By “more powerful” we really mean “MAY have allow additional access”. Let’s say you call an RS for read access on a specific item, “object-1234", the RS figures out what you’re going for, and gives you back a reference that’s good not only to read that specific item, so an “action” of “READ” and an “identifier" of “object-1234”, but also to search across items and read other related items - let’s call that reference “SEARCH-ALL” for simplicity here. The “SEARCH-ALL” access right is more powerful, as defined by the API being protected, but importantly it includes the access needed by the client to make the same call again and succeed. The fact that the client can also do other things is API-specific, as are all access rights.


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

Because at the time I didn’t know that Specification Required included Expert Review. :) Happy to review and change these where it makes sense.


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


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

Yes, because we don’t want just the format, but how the format is encoded into a string. It’s an obvious encoding - put it in a JSON string and call it a day - but it needs to be specified and the target documents don’t do that. The sections in question do provide that context.


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

As with the HTTPS comment above, this cutout was meant to account for deployments that could account for network security without using TLS, such as a localhost connection or a virtual network inside of a container. What we really want is “if your connection is going out on a network connection you don’t fully control and trust, then encrypt it”. Is there a better way to say this? I think the same advice would apply to the HTTPS comment above.


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

That’s fair, the RS draft can (and will) define its own requirements here so we can drop that line.


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

** Section 13.3.  Editorial.  Expand “SPAs”.

Got it, thanks. Looks like we expand it elsewhere, so we’ll make sure it’s expanded on first use.


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

This is really meant to be about client certificates in MTLS being a major pain point.


** Section 13.23.  “… (in most cases over HTTP)”, should this be HTTPS?

Yes, modulo the discussion about HTTP/HTTPS/TLS/“or equivalent” above.


** Section 13.24.  Typo. s/defence/defense/

Got it, thanks!


** Section 13.24.  Typo. s/Prerequesits/Prerequisites/

Got it, thanks!


** IDNITs reports:

 == Outdated reference: draft-ietf-httpbis-client-cert-field has been
    published as RFC 9440


Thanks, we’ll fix that. We also made a conscious choice to not normatively reference RAR, but now that it’s an RFC would it make sense to at least informationally mention it in section 8, or even appendix B? We don’t expect wire compatibility but it’s part of the story of how OAuth 2 and GNAP can be bridged. Would appreciate some advice on that.


Again, thanks so much for the deep review, I think most of these are clearly actionable and only a few outstanding questions that I don’t see as particularly problematic.

 — Justin

--
TXAuth mailing list
[e-mail]
https://www.ietf.org/mailman/listinfo/txauth