Re: [GNAP] Summarizing status from SECDIR IETF LC review of draft-ietf-gnap-core-protocol-16

[Russ Housley <[e-mail]>]

Justin:

>> 
>> During IETF LC, SECDIR (Russ Housley) performed a review on draft-ietf-gnap-core-protocol-16.  See https://mailarchive.ietf.org/arch/msg/txauth/i9NEvnr2l1p8hW7U_l382NZbLxg/.  Justin (Richer) responded and iterated on this review. Here are the relevant messages in the thread since it is not linked from the SECDIR review link in the Datatracker.
>> 
>> (Justin-2023-11-27) https://mailarchive.ietf.org/arch/msg/txauth/-Zl-mKGKC0WgujAlj8cd3sOgr2Q/
>> (Russ-2023-11-28) https://mailarchive.ietf.org/arch/msg/txauth/ZArrGJN06Tsyi1o9g48P2J4fzIo/
>> (Justin-2023-11-29) https://mailarchive.ietf.org/arch/msg/txauth/Ek0NCQeiUfuiJv7EdSFzPq2s-ao/
>> 
>> A -17 of the document has been released.  I very much appreciate the deep review and iteration.  I am trying to re-review the document and identify unresolved issues.  The following is my assessment.  The inline thread was broken at some point making it difficult to attribute text so I'm repeating the initial feedback from the SECDIR review here and annotating it with [Roman].  Please consult the above links for some of the details.  I'm looking for resolution of the issues below with further discussion or a document change; or a correction that that this issue been addressed in a way I didn't immediately recognize.
>> 
>> ** (Russ on -16) 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.
>> 
>> ** (Russ on -16) 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.
>> 
>> [Roman] Is it possible to be specific on where there is a mismatch?
> 
> GNAP is fundamentally a delegation protocol, which can delegate both authorization to call APIs (a la OAuth) and the identity of the user (a la OpenID Connect) in the same flows. I do not see a confusion between the authorization and authentication aspects here, especially as they are deliberately requested and returned separately from each other.

I got this from the discussion that we had following the SECDIR review.  However, I think that saying something about authentication in the role definition in Section 1.2 would be very helpful.

I am still confused about how a device can be an RO.  See my message from earlier today.

>> ** (Russ on -16)  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.
> 
> These two sections are not intended to have parallel structure. Section 2.1.1 says "these are the bits you need to use to get an access token" and 2.1.2 says "to get more than one access token use an array of the things in 2.1.1".  The "one or more" language was a bug and was fixed in -17; otherwise, all the fields are used the same way and I don’t see a value in repeating them in this section — I think that would actually confuse the reader more.

I just read these two section in version -17, and they seems fine now.

>> ** (Russ on -16) 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.
>> 
>> [Roman] Section 13.15 has several relevant considerations.  Building on Russ’s point of having confidence in fetched image and the intent to provide flexibility, isn’t there a residual security considerations to document where an AS provides a URI which 
> 
> Both the value and security consideration come from the client being able to swap out the image without updating the AS.
> 
> I’ve proposed an additional security consideration just for this particular aspect in this PR:
> 
> https://github.com/ietf-wg-gnap/gnap-core-protocol/pull/524

Great improvement!

I am not sure what you mean by a "sanitized and generic URI".  Do you have a reference or short description?

>> ** (Russ on -16)  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."
>> 
>> [Roman] These seems like guidance which should be included (moved to?) Section 11.8 which contains the evaluation criteria for the DE.  That sections already has a number of items to consider.
>> 
> 
> I’ve put together a PR to move this to the IANA section.
> 
> https://github.com/ietf-wg-gnap/gnap-core-protocol/pull/525

Great.  This one is now resolved.

>> ** (Russ on -16)  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.
>> 
>> [Roman] This doesn’t appear to have been resolved.  There was an indication that improved definitions would be added to Section 1.1
> 
> An application-specific URL is not loaded across an untrusted network connection and is therefore equivalent in protection to plain HTTP on localhost. I’ve added that text explicitly in all the places I could find:
> 
> https://github.com/ietf-wg-gnap/gnap-core-protocol/pull/526
> 
> I don’t believe it needs an additional definition.

Nit:  Would it change your intent to say "that already exists on the end user's device."

The point is that it is already there, so no network access is needed at all.

>> ** (Russ on -16) 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.
>> 
>> [Roman] -16 already contained a reference to RFC4107 in Section 13.7.  The discussion thread seems to indicate that additional pointers would be added in a version after -16.  I don't see it.
> 
> It seemed to make more sense to keep the reference in the security considerations section as opposed to making a reference to it (normative or otherwise) in the client-signing section (7). Yes, clients and AS’s need to manage and store their keys well. But the thrust of section 7 is how those are presented on the wire and used to bind request messages to keys, not how the keys themselves are managed. There’s already a forward reference to the security section that mentions RFC4107 in section 7, so we felt that was sufficient.

I really dislike the notion of a symmetric key being used for "signing".  In my view, digital signatures require asymmetric cryptography.  Is there a way to avoid this in the first paragraph of Section 13.7.

>> ** (Russ on -16) 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"
>> 
>> (Justin) 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).
>> 
>> (Russ) 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.
>> 
>> (Justin) The client instance compares the referrer parameter returned to the URL of the RS.
>> 
>> [Roman] Perhaps this clarifying guidance can be added directly into the text.
> 
> I’ve put together a PR to clarify this, as well as straighten out the language around the requirements of what’s returned from the RS and what the client is supposed to do with them.
> 
> https://github.com/ietf-wg-gnap/gnap-core-protocol/pull/527

This is now answering my concern.  However, the sentence is incomplete: "... plain string comparison method in ."  I assume you meant to insert a reference at the end of that sentence.

>> ** (Russ on -16) 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.
>> 
>> [Roman] Doesn’t seem like we have resolution on the issue.
> 
> It’s a security consideration because the client can choose to not calculate and check the value, which would leave it open to the described attack. The mitigation of the attack is to perform the function as specified in the document. This section explains why it’s important, which I believe is a valid use of a security considerations section.

I think you do not understand my comment.  You specify the way that the interaction hash is calculated.  Everybody needs to do it as specified.  Failure to do so is an interoperability issue.  This section talks about the protections that are offered by checking the interaction hash.  Failure to perform the check is a security issue.

Russ