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

[Russ Housley <[e-mail]>]

Justin:

My responses are in line.

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

This audience is fine with me.

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

Maybe there can be some text up front that provides this context to the fresh reader.

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

Since this distinction is made elsewhere in the document, I have a real problem with that response.

>> 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 5.1 says:

   _Processing_:  When a request for access (Section 2) is received by
      the AS, a new grant request is created and placed in the
      _processing_ state by the AS.

So, the AS is keeping state in some fashion.

The figure toward the beginning of the section is the state machine for the AS, right?  If not, please provide a paragraph after the figure that says more.

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

The existing text is confusing to me.

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

Thanks.

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

Here is the part that confused me in Section 2.1.1:

   access (array of objects/strings):  Describes the rights that the
      client instance is requesting for one or more access tokens to be
      used at the RS.  REQUIRED.  See Section 8.

Section 2.1.1 is about "Requesting a Single Access Token", and this allows the request of one or more access tokens.

Maybe the fields need to be explained in Section 2.1, and then the subsections explain their use for requesting a single access token or multiple.

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

My concern is not addressed in either Section 13.13 or Section 13.22.  I cannot see any benefit for allowing the web server to change the logo.  This enables The URL remains the same, but the admin of the web server can decide what logo is reurned at various points in time.

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

I am even more confused than before.  This section is about user interface.  If you want to impose requirements on people that will extend the protocol in the future, that seems to be a separate topic for a separate section.

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

Obviously, this was not clear to me.  I now see that Section 2.5.1.2 says that it is associated with "an application on the end user's device", however, I was thinking about the URLs used by WebEx and Teams and the like.  So, I think a definition is needed.  Maybe a definition in Section 1.1.

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

Thanks.

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

Thanks.

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

What checks MUST the client instance perform?  Section 13.36 does not address my concern.  Section 13.36 is about protections that are implemented at the resource server.  

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

I think a reference is needed here.

>> Please consider moving Section 13.17
>> and Section 13.18 to follow Section 13.1.
> 
> Thanks, we’ll consider re-organizing.

Thanks.

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

draft-ietf-suit-report <https://datatracker.ietf.orgdraft-ietf-suit-report/> provides an example of something that is being signed by the recipient of a firmware update.  If that is not useful, then disregard.

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

Consider step (6), where hash IH1 calculated from (CN1 + SN1 + IR1 + AS).  If the attacker manipulates one or more of these values, then IH1 will be different.  This can lead to a denial of service or an interoperability problem.

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

The quotes are very confusing.  Please reword.

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

This is another place where my very first comment is causing confusion.  Context is needed at the beginning of the document about GNAP being neither an authorization nor authentication protocol.

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

This is another place where my very first comment is causing confusion. 

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

Perhaps:

   A formal trust model is out of scope for this specification, but
   the terms and structures used in this document comes from
   [promise-theory].

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

Better:

   If the client requests an interaction mode that the AS does not
   support, then the AS MUST respond 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.

Okay. I'm not sure how to add clarity here, so I withdraw this comment.

>> Section 7.3: Please provide a reference for the "RS256" algorithm.
> 
> Thanks, we’ll point to the JWA entry for this.

Thanks.

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

Thanks.

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

Thanks.

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

I think the two sections should use the same style.  Pick one.

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

I did my review of the .txt file.

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

Thanks.

Russ