[Gen-art] Re: [GNAP] Genart early review of draft-ietf-gnap-resource-servers-06

[Justin Richer <[e-mail]>]

Hi Lars, thanks for the review. Answers inline, and changes where applicable are at https://github.com/ietf-wg-gnap/gnap-resource-servers/pull/92


— Justin

On Jul 9, 2024, at 7:23 AM, Lars Eggert via Datatracker <[e-mail]> wrote:

Reviewer: Lars Eggert
Review result: Ready with Nits

# genart-early review of draft-ietf-gnap-resource-servers-06

CC @larseggert

I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the [FAQ](https://wiki.ietf.org/group/gen/GenArtFAQ).
Please resolve these comments along with any other comments you may receive.

## Comments

I don't know much about this area. The doc is well-written and seems
basically in good shape.

### Missing RFC status

Datatracker does not record an intended RFC status for this document.

It’s supposed to be tracked as proposed standard, perhaps the chairs need to update something? The document itself says "Intended Status: Standards Track" right at the top.


### Section 2.1.1, paragraph 1
```
    All access tokens have a unique value.  This is the string that is
```
"Unique" in which sense? Between the parties at the time of usage,
between the parties at any time, within the entire domain,
globally?

Thanks, this can be clarified — we mean unique from the same source during the time the token is valid. This is for security purposes — two different access tokens should never have the same value and be valid at the same time, or even within a reasonable window of each other.


### Section 2.1.1, paragraph 3
```
    The format and content of the access token value is opaque to the
    client software.  While the client software needs to be able to carry
    and present the access token value, the client software is never
    expected nor intended to be able to understand the token value
    itself.
```
Will it need to understand it sufficiently to determine that it is
unique in some way? (See above.) Also, this concept of opaqueness comes
up in other parts of the document, and it would be good to clarify if the
various parties involved in this protocol are required to verify that
property (and fail operations if it is violated?).

No, the client does not need to understand the token contents at all and it does not have any responsibility enforcing the uniqueness of the token. As far as the client is concerned, the value is a string that it uses as-is in the protocol. If an AS hands the client the same token value ten times in a row, the client shouldn’t care, since that’s the AS’s decision, not the client’s.

How would one verify opaqueness of the token from the client’s perspective? The client has no responsibility regarding the content of the token and no actions it can take regarding the token’s content. I don’t believe this is an actionable text suggestion.


### Section 2.1.2, paragraph 3
```
    In a [JWT] formatted token or a token introspection response, this
    corresponds to the iss claim.
```
What is an "iss claim"? (A reference may be helpful.)

This is defined by [JWT], already referenced here.


### Section 2.1.3, paragraph 2
```
    In a [JWT] formatted token or token introspection response, this
    corresponds to the aud claim.
```
What is an "aud claim"? (A reference may be helpful.)


This is defined by [JWT], already referenced here.

### Section 5.3, paragraph 4
```
    *  the format's definition is sufficiently unique from other formats
       provided by existing parameters.
```
"sufficiently unique" is pretty vague, and give a lot of leeway to the
experts. What would an example of "not sufficiently unique" be?

The goal of the token format is to be a top-level container definition like the other values registered by this document. Trying to register "JWT signed with JWS and RSA256" when "JWT" on its own is already registered — this would be an error and should be rejected. We would expect the DE to understand token formats sufficiently to make this decision.


### Section 5.5, paragraph 4
```
    *  the claim's definition is sufficiently orthogonal to other claims
       defined in the registry so as avoid overlapping functionality.
```
See above, wrt "sufficiently unique". Other registries below have
similar verbiage; it would be nice to be a bit more descriptive in what the
expert should actually check here.

We would expect the DE to be sufficiently expert enough in the protocol to determine whether the newly-requested registration would be better covered by some existing claim, and this applies to all such cases. I’ve seen this approach in a number of other registries, but if there’s a better way to word this expectation, please provide a suggested change.


## Nits

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool) so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

### Grammar/style

#### Section 2.1.4, paragraph 4
```
ss_token section of the grant response response in Section 3.2 of [GNAP]. Fo
                             ^^^^^^^^^^^^^^^^^
```
Possible typo: you repeated a word.

#### Section 3.1, paragraph 3
```
ys by reference or by value in a similar fashion to a client instance callin
                           ^^^^^^^^^^^^^^^^^^^^
```
Consider replacing this phrase with the adverb "similarly" to avoid wordiness.

#### Section 3.3, paragraph 19
```
oken is no longer valid. Expressed as a integer seconds from UNIX Epoch. iat
                                     ^
```
Use "an" instead of "a" if the following word starts with a vowel sound, e.g.
"an article", "an hour".

#### Section 3.3, paragraph 20
```
en was issued by the AS. Expressed as a integer seconds from UNIX Epoch. nbf
                                     ^
```
Use "an" instead of "a" if the following word starts with a vowel sound, e.g.
"an article", "an hour".

#### Section 3.3, paragraph 20
```
this token is not valid. Expressed as a integer seconds from UNIX Epoch. aud
                                     ^
```
Use "an" instead of "a" if the following word starts with a vowel sound, e.g.
"an article", "an hour".

#### Section 6.1, paragraph 1
```
, a system can follow a principle of least disclosure to each RS. 6.9. Resour
                                    ^^^^^
```
A determiner may be missing.

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. Review generated by the [`ietf-reviewtool`][IRT].

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments
[IRT]: https://github.com/larseggert/ietf-reviewtool


--
TXAuth mailing list -- [e-mail]
To unsubscribe send an email to [e-mail]