Re: [GNAP] Secdir last call review of draft-ietf-gnap-core-protocol-16

[Justin Richer <[e-mail]>]

Hi Russ, thanks for the review. Some responses inline below. I’m copying the GNAP WG and the AD, but leaving off the other distribution lists — please let me know if that’s not the desired audience.

> On Nov 4, 2023, at 12:50 PM, Russ Housley via Datatracker <[e-mail]> wrote:
> 
> Reviewer: Russ Housley
> Review result: Has Issues
> 
> I reviewed this document as part of the Security Directorate's ongoing
> effort to review all IETF documents being processed by the IESG.  These
> comments were written primarily for the benefit of the Security Area
> Directors.  Document authors, document editors, and WG chairs should
> treat these comments just like any other IETF Last Call comments.
> 
> Document: draft-ietf-gnap-core-protocol-16
> Reviewer: Russ Housley
> Review Date: 2023-11-04
> IETF LC End Date: 2023-11-21
> IESG Telechat date: Unknown
> 
> Summary: Has Issues
> 
> 
> I did not review the Appendices.
> 
> 
> Major Concerns:
> 
> Section 1.2: I am having trouble getting my head around the idea that
> the AS role grants subject information to client instances.  This feels
> like a purposeful confusion between authentication and authorization.
> 

In spite of its name, GNAP is neither an authorization nor authentication protocol — as it says in the introduction, it’s a delegation protocol. What is being delegated to the client application broadly falls in two categories: 

 - access to APIs and services (this is the access tokens, the authorization side)
 - the knowledge of who the user is (this is the subject identifiers and assertions, the authentication side)

This has been a grounding use case for GNAP from the very beginning of the working group, and it’s long-established in OAuth and OpenID Connect.

> Section 1.3: Elsewhere in the document, there is a careful distinction
> between software that acts on behalf of a subject and the natural person
> that uses the software.  This distinction is missing in the definition
> of Subject.  It is made worse by including organizations.  Please say
> how people and organizations "decide whether and under which conditions
> its attributes can be disclosed to other parties."  I believe this is
> done by interacting with the software or initial configuration of the
> software.

It’s outside the scope of GNAP how these decisions are made, and not for this section of concise definitions to go into any detail, even by example.

> 
> Section 1.5: Is the AS the only stateful role?  If not, Section 1.2
> should say which ones are stateful and which ones are not.  If so,
> please clarify the 2nd para of Section 1.5.

This section isn’t about statefulness of the AS, client, or any other components — it’s about the statefulness of the grant request itself. How any of the components handle that statefulness is a development decision. I believe the current text is clear that this is speaking about the grant process.

> 
> Section 2: I worry about the phrase "unless otherwise specified by the
> signature mechanism" inside a MUST statement.  Which part of the MUST
> statement can the signnature mechanism change?  Can it change the use of
> a JSON object?  Can it change the use of HTTP POST?  Can it change the
> use of application/json as the content type?
> 

The signature mechanism can change the use of the JSON object and associated media types, as demonstrated in §7.3.4 (https://www.ietf.org/archive/id/draft-ietf-gnap-core-protocol-16.html#name-attached-jws). This cutout can be made more specific.

> Section 2.1.1: This subsection is about a request for a single access
> token, yet the description continues to talk about "one or more access
> tokens".  Please be consistent.

Good catch, this language was a holdover from a previous syntax, we’ll update.

> 
> Section 2.1.2: I expected a parallel structure to Section 2.1.1.  I
> think it would really help the reader if the sections had the same
> structure.

I don’t understand the request here - section 2.1.2 is activated by sending an array of the objects defined in section 2.1.1, with some additional constraints (label is required, namely). So to me, they are already deeply interrelated. How would you suggest parallelizing these more?

> 
> Section 2.3.2: I am not worried about logo_uri if only data: URIs are
> allowed, but that does not seem to be the case.  Since the logo might
> be fetched, there need to be an integrity protection mechanism to 
> ensure that the web server does not provide a different image than
> was intended.  RFC 3709 has this same concern and it uses a hash of
> the image to ensure that the intended image was provided.

One of the goals of having a remote URL for this field is to allow the hosted URL to change its contents without updating any stored or registered values with the AS. Therefore, an integrity hash would negate the usefulness of this feature.

Related to this: impersonation and related issues are listed in §13.13 (https://www.ietf.org/archive/id/draft-ietf-gnap-core-protocol-16.html#name-client-instance-impersonati), SSRF in §13.33 (https://www.ietf.org/archive/id/draft-ietf-gnap-core-protocol-16.html#name-server-side-request-forgery).

> 
> Section 2.5.1: I do not understand what an implementer is supposed to
> do based on this MUST statement:
> 
>   "All interaction start method definitions MUST provide enough
>   information to uniquely identify the grant request during the
>   interaction."
> 

The "implementor" here is someone defining a new interaction start method. Whatever information passed to that method would need to uniquely identify the associated grant request that the interaction is associated with. For example, the "redirect" mode uses a unique URI, the "user_code" modes use the user code, etc.

> Section 2.5.2.1: How does the use of an application-specific URI scheme
> provide for the same security as HTTPS?  It seems like an way to avoid
> them.  This text appears several places in the document.

Application-specific URIs are generally loaded on the same device as the browser, and therefore the request does not transit untrusted networks.

> 
> Section 3.1: It says:
> 
>      " ... and omission of the value MUST NOT be
>      interpreted as zero (i.e., no delay between requests)."
> 
> It would be much better to provide a default value.  That is,
> omission of the value MUST be treated a delay request of 5 seconds.
> 

This requirement was based on experience with implementing RFC 8628, which I now see has exactly this set as the default, so we’ll add that in.

> Section 7: Please consider a reference to RFC 4107.  I'm not sure where
> in this section is the best place to add a cite.

Thank you, we’ll consider where best to incorporate a reference.

> 
> Section 9.1: I do not understand what an implementer is supposed to
> do based on this MUST statement:
> 
>   "client instance MUST check its value to protect itself"

The client instance checks whether the value of the `referrer` parameter is the URI of the resource server that it made the request to. The rationale for this is discussed a little more in §13.36 (https://www.ietf.org/archive/id/draft-ietf-gnap-core-protocol-16.html#security-compromised-rs).

> 
> Section 13.1: This section ought explain what is menant by "mutual TLS"
> as used in the body of the document.

Thanks, we meant it as used by RFC8705 and so can import the definition used there (https://datatracker.ietf.org/doc/html/rfc8705#section-1.2)

>  Please consider moving Section 13.17
> and Section 13.18 to follow Section 13.1.

Thanks, we’ll consider re-organizing.

> 
> Section 13.2: Please consider the work of the SUIT WG.

I’m not sure which work in SUIT is directly applicable here. We’re not talking about attestation of the software itself, but about binding the actual request messages to a key held by the client instance. Could you be more specific about what you’re suggesting we incorporate here?

> 
> Section 13.24: This section does not seem like a Security Consideration.
> If the calculation of the interaction hash is not done the same, then
> there will be interoperability issues.

I’m not sure what the concern is with this comment - the section serves to expand the security discussion around the need for properly calculating the interaction hash as defined in §4.2.3 (https://www.ietf.org/archive/id/draft-ietf-gnap-core-protocol-16.html#name-calculating-the-interaction), and it illustrates how an attacker could leverage a bad implementation that skipped this required step.

I don’t understand the comment about interoperability problems - the AS and the client instance always calculate the hash the same way. The problem described in this section only occurs if the client instance is lazy and does not calculate the hash at all. 

> 
> 
> Minor Concerns:
> 
> Section 1.4, 1st para: Where does the quoted text appear?  Please add a
> cite.

It isn’t a quote from external sources, it is a definition.

> 
> Section 1.4, "end user/client" bullet: This seems like the wrong place
> to be an attacker's client software.  Rather, a forward pointer to
> authentication considerations seems more appropriate here.

This section describes the relationship between different components and parties.

This is appropriate since getting the end-user to use malicious or compromised client software is one major vector of attack. The end user has a trust relationship with the client software, even if that trust relationship should not actually happen because it’s an attacker’s software.

Authentication doesn’t help - an attacker’s client software could be properly authenticated, without the end-user knowing. See the section on client impersonation for some possible issues here known in the wild.

> 
> Section 1.4, "client/AS" bullet: Again, this seems like the wrong place
> to discuss the difference between honest AS and otherwise.  As a result,
> the actual trust relationship between the client and the AS is unclear.

Same comment as above, this trust relationship exists regardless of the honesty of the AS. 

> 
> Section 1.4, last para: Why is this here?  If it is relevant, this feels
> like the wrong place for this statement.

This serves as an anchor for the trust model here — while it is not fully normally specified, the language here uses terms and structures from the referenced document.

> 
> Section 3.3: This seems like a throw away statement:
> 
>   "The AS MUST NOT respond with any interaction mode that the AS does
>   not support."
> 
> It would me much more helpful to say what ought to happen is the client
> asks for an unsupported interaction mode.

It’s not throwaway - a lazy implementation could try to always respond to everything, or throw an error if something is asked for that is not supported. Neither is the correct behavior: instead, the AS returns only interaction modes that it supports. It’s essentially a set intersect operation:

Let’s say a client software supports [ Foo, Bar, Baz ] methods and the AS supports [ Foo, Baz, Qux ] methods.

	Client -> AS : I can do interaction modes [ Foo, Bar, Baz ] for this request

AS sees this and doesn’t support Bar at all, so it won’t respond using that. It does support Qux, but since the client didn’t ask for that it won’t return it. It will probably decide that Foo and Baz are both OK:

	AS -> Client : I can support [ Foo, Baz ] for this request

If the client asks for things the AS doesn’t support (or doesn’t want to honor for any other reason for this request), then the AS responds with an empty set.

> 
> Section 3.3.3: It is unclear whether you want the code to avoid
> characters that are easily confused, like 1 and l.  Of course, the
> more characters that are to supported reduces the effort to guess the
> code.  This text appears several places in the document.

Yes, it is the goal to point implementors (of the AS) in a direction to avoid easily misconstrued characters. This is going to vary depending on system, expected inputs, character sets, and cultural norms.

> 
> Section 7.3: Please provide a reference for the "RS256" algorithm.
> 

Thanks, we’ll point to the JWA entry for this.

> 
> Nits:
> 
> Global: Please create a reference for the hash-alg IANA registry and use
> it instead of the URL to the registry: 
> https://www.iana.org/assignments/named-information/named-information.xhtml#hash-alg
> 

Thanks, we didn’t realize this wasn’t a reference already. We’ll fix that.

> Section 1.2: Suggested wording improvement:
> 
>   OLD:
>   ... For
>   example, a client instance could have components that are installed
>   on the end user's device as well as a back-end system that it
>   communicates with. 
> 
>   NEW:
>   ... For
>   example, a client instance could have components that are installed
>   on the end user's device as well as on a communicating back-end
>   system.
> 

Thanks, we’ll take a look at that wording to make sure it’s clear.

> Section 1.5: Why are the state bullest offered with underlines? Please
> use the same format as Section 1.3 uses for the elements.

The italics were intended to set off the state values as an enumerated set. I believe the current typography is correct.

> 
> Section 1.6.3: Stretch the "End User" box in the figure or shift things
> to avoid "Completed+".

This seems to only be an issue in the ASCII version (the SVG rendered version is clear). We’ll see if that can be tweaked.

> 
> Section 2.2: s/subject that information is being requested for./
>              /subject for which information is being requested./
> 
> Section 2.2: s/All identifiers in the sub_ids array MUST/
>              /If present, all identifiers in the sub_ids array MUST/
> 
> Section 2.5.1.1: s/techniques such as this/techniques/
> 
> Section 3.3.1: s/is a string containing the URI to direct the end user to./
>                /is a string containing the URI for the end user to visit./
> 
> Section 3.4: s/that the subject information is received from/
>              /AS from which the subject information was received/
> 
> Section 4.2.3: s/single newline (\n)/single newline (0x0a)/
> 
> Section 5: s/requests to RS's/requests to an RS/
> 
> Section 6.2: s/ AS should/ AS SHOULD/
> 

Thanks, we’ll look at all of these in an editorial pass.



 — Justin