Re: [GNAP] Murray Kucherawy's No Objection on draft-ietf-gnap-core-protocol-18: (with COMMENT)

[Justin Richer <[e-mail]>]

And now this time — with actual comments! (Accidentally bumped the "send" button, sorry!)

Fixed items are in this PR unless otherwise indicated: https://github.com/ietf-wg-gnap/gnap-core-protocol/pull/534

— Justin

On Mar 8, 2024, at 11:59 AM, Justin Richer <[e-mail]> wrote:

Thank you for the review. Comments inline below.

— Justin

On Mar 7, 2024, at 10:26 AM, Murray Kucherawy via Datatracker <[e-mail]> wrote:

Murray Kucherawy has entered the following ballot position for
draft-ietf-gnap-core-protocol-18: No Objection

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)


Please refer to https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/
for more information about how to handle DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-gnap-core-protocol/



----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

===

My own review isn't done yet, but forwarding some comments from Orie Steele,
incoming ART Area Director:

"""
The AS is uniquely defined by the grant endpoint URI, which the absolute URI
where grant requests are started by clients. """

Suggested change "which is the absolute URI" (missing is?).


Yes, that’s correct, fixed.

"""
Grant:

(verb): to permit an instance of client software to receive some attributes at
a specific time and valid for a specific duration and/or to exercise some set
of delegated rights to access a protected resource;

(noun): the act of granting permission to a client instance.

"""

I wonder if these definitions can be shortened to the following without loss of
generality?

"""
Grant:
(verb): to permit an instance of client software to receive attributes or to
access protected resources. (noun): an expression of attributes or permissions
given to an instance of client software.
"""

I don’t believe we can really tighten them any more than they are — there was considerable debate about the content of most of the definitions, and I believe that removing any specific content would lose the intent of the definition itself at this point. The text about the specificity of the grant is intentionally included to limit the usage.


...
"""
Some promises can be conditional of some previous interactions (e.g. repeated
requests). """

Suggested change: Some promises can be conditioned on previous interactions
(e.g. repeated requests).

Changed to "Some promises can be affected by previous interactions (e.g., repeated requests).


"""
In may cases, this happens through a front-channel interaction through the end
user's browser. """

Suggested change: In many cases,...

Thanks, fixed!


In section-2.1.1

"""
A unique name chosen by the client instance to refer to the resulting access
token. """

Is this meant to be a machine facing string, or a human facing one?
Are there unicode consideration that apply to this field, similar to
https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-sfbis-05#section-3.3.8
?

Put another way, is deceptive text a security consideration for this field?

This is a machine facing string, human should not normally see this. We’ve added text to make that more specific.


"""
Each object is a subject identifier as defined by
[[I-D.ietf-secevent-subject-identifiers](https://datatracker.ietf.org/doc/html/draft-ietf-secevent-subject-identifiers-18)].
"""

Suggest to update reference to: https://datatracker.ietf.org/doc/html/rfc9493

Updated, thank you.


"""
If the identified end user does not match the RO present at the AS during an
interaction step, and the AS is not explicitly allowing a cross-user
authorization, the AS SHOULD reject the request with an unknown_user error. """

Why not MUST?

It’s ultimately up to AS policy whether it wants to reject or escalate the request, especially during the interactive section. The AS could decide, for example, to allow the "correct" user to authenticate instead of rejecting outright.


Section 2.5.3.1 Indicate Desired Interaction Locales

It would have been good to have gotten an i18n review based on this section.

I recommend a reference to https://datatracker.ietf.org/doc/html/rfc5895
regarding international domain names, to ensure that language considerations in
URLs are also addressed.


I don’t see where that would apply in this section. The locales are the client telling the AS "this is what locale I think the user is using because they configured something with me, just fyi". It’s up to the AS to decide what to do with that information, and it’s even likely that an AS would defer to whatever configuration settings it has for the RO once they’ve authenticated during the interaction phases. Can you please expand where you think this would apply? Thank you.

In section 3, an informative or normative reference for DID should be provided.


Instead of that, we’ve added an explicit reference to RFC9493 which defines the "did" format. We have a normative dependency on RFC9493, not DID, which is only used in this one example.

In section 3.4

"""
value (string):
The assertion value as the JSON string serialization of the assertion. REQUIRED.
"""

The example starts with "eyj...", is this value meant to also be base64url
encoded?

It seems like this might also be a JWT, in which case, its not a JSON string,
its a string of base64url encoded JSON strings separated by periods.

A less elided example might be helpful here.

That is an incorrect reading. The value is a JSON string. The content of that string is based on the assertion type. For an ID token, that’s going to be a compact JOSE (so base64url plus JOSE stuff).

To make this clearer, we’ve added a new subsection calling out the assertion formats directly.


Section 4.1.2 rightly warns of deceptive / indistinguishable unicode, you might
consider a citation to https://datatracker.ietf.org/doc/rfc8264/


Thanks for the reference, this has been added.

Section 5.3

"""
 The AS SHOULD check that this presented user information is
  consistent with any user information previously presented by the
  client instance or otherwise associated with this grant request.
"""

What happens if this check is not performed or the information does not match?

As with many things in a modification, it’s up to the AS how to handle it. The AS can ignore the presented user information in favor of the RO, it could reject the request because the user isn’t the same and the AS doesn’t want that, it could modify the request to update the RO because it trusts the client instance to know the user (maybe through an assertion the client instance presents).


References to ietf-httpbis-message-signatures should be updated to
https://datatracker.ietf.org/doc/html/rfc9421

Thank you, done.


In section 7.3.3

+jwsd is introduced, but there is no corrosponding registered Structured
Syntax Suffixes in
https://www.iana.org/assignments/media-type-structured-suffix/media-type-structured-suffix.xhtml
or in the document.

Similarly  `typ: gnap-binding+...` implies a registered media type of
`application/gnap-binding+...`

`gnap-binding-rotation` is also present but not requested to be registered via
IANA media types registry.

This has been done in a separate PR: https://github.com/ietf-wg-gnap/gnap-core-protocol/pull/533


In section 11, you may wish to comment on if expired drafts are acceptable
references for specification required, so that experts have guidance on this
topic.

In 11, I wonder if it is truly necessary to request 16 registries from IANA,
given the initial entries for some of the registries contain only a single
reference.

The intent was to create a single page within IANA to house all the registries, such as is done with other protocols, such as OAuth 2: https://www.iana.org/assignments/oauth-parameters/oauth-parameters.xhtml and HTTP Signatures: https://www.iana.org/assignments/http-message-signature/http-message-signature.xhtml

Each section is a separate registry, but all are collected together under a single heading. If there is a better way to format this request, I would be happy to reformat this.

 — Justin