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

[Justin Richer <[e-mail]>]

Hi Russ, Responses below with the items that seem clear to me pulled out.

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.

The "subject" is not equivalent to the "end user" or the "client software/instance". Instead it’s the party — whoever or whatever they are — that the "subject information" returned is about. This isn’t conflating these items at all, and how the release decision is made is out of scope of GNAP — which is not in conflict with the rest of the spec.


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.

Again, the statefulness here is about the grant request. The AS and client instance both need to have some notion of the statefulness of this grant request as it goes through the system. The state machine is not for the AS, it’s for the grant request itself.


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.

Ah, that sentence was pointed out in a separate review — the "one or more" is a holdover bug from a previous syntax. This array — and indeed this entire structure — is about a single access token, as described in the section’s introduction. The subsequent section is how you use that within an array for multiple tokens.


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.

The client should be able to update its self-hosted resources without updating the registration at the AS.
So yes, letting the admin of the web server ( where the URL is hosted ) decide what to return is exactly the use case here. If you want to have a fixed value, use a data url, as suggested in the text.



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.

The section is about starting user interaction. I think it’s sensible to include requirements for extensions in the section that describes the extendable portion — which we’ve done elsewhere in the document.


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.

The client instance compares the referrer parameter returned to the URL of the RS.


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.

That’s not an interoperability problem, that’s the protocol failing to function when an attacker changes one of the values. This is the intent of having and checking the hash. I see no issue here.


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.

Your suggestion is incorrect. If you see the example above, the AS does not return an empty set when the client requested "Bar" — it returns with the subset that it understands and supports (Foo and Baz), which never includes a method that the AS doesn’t support (Bar). That’s the intent of the language.



Thanks again for your comments,
 — Justin