Re: [GNAP] review of draft-ietf-gnap-resource-servers-05

[Justin Richer <[e-mail]>]

Hi Andrii, thanks for the review. Answers inline.

— Justin

On Apr 30, 2024, at 7:30 PM, Andrii Deinega <[e-mail]> wrote:

Hello GNAP WG,

I've reviewed this document and came up with these comments below.

1. in the abstract section my preference would be to have

"GNAP defines a mechanism for delegating authorization to a piece of
software, and conveying the results and artifacts of that delegation
to the software."

just like it's defined
https://www.ietf.org/archive/id/draft-ietf-gnap-core-protocol-20.html,
right now it says

"GNAP defines a mechanism for delegating authorization to a piece of
software, and conveying that delegation to the software."


Good catch, we will align these.

2. sentence "Additionally, this document defines a general-purpose
model for access tokens, which can be used in structured, formatted
access tokens or in the API."

could be rephrased & simplified. What was the intent behind the "API"?
Was it "an API-like mechanism like token introspection"?

Thanks, we will clarify what this meant — at first read I do think it means introspection.


3. "implementors" should be probably changed to "implementers". The
same applies to the use of "implementor" in this as well as in the
core spec.

Thank you, this does seem to be the preferred spelling. I’m sure the RFC Editor will pick up on that too.


4. sentence "This is not intended to be a universal or comprehensive
list, but instead to provide guidance to implementors when developing
data structures and associated systems across a GNAP deployment.""

could be simplified to something like

"This list is not universal nor comprehensive but rather serves as
guidance for implementers in developing data structures and associated
systems across a GNAP deployment."

5. sentence "In the case of an asymmetric algorithm, the model for the
AS and RS need only contain the public key, while the client instance
will also need to know the private key in order to present the token
appropriately."

could be simplified to

"In the case of an asymmetric algorithm, the model for the AS and RS
needs only to contain the public key, while the client instance will
also need to know the private key in order to present the token
appropriately."

6. sentence "The source of this key information can vary depending on
circumstance and deployment" should be changed to "The source of this
key information can vary depending on the circumstances and
deployment."

Thanks, can clarify the text for all of these.



7. section 7.3 should be called as "Caching Token Validation Result",
not "Cacheing Token Validation Result".

This also seems to be the preferred spelling, will address with above.


8. I suggest changing "the access_token request" and "the access_token
response" to "the access token request" and "the access token
response" accordingly in all places.

This was actually meant to talk about the request parameter named access_token so we will clarify that.


9. Some examples of HTTP requests and responses do not include the
Host HTTP header, as an example, the second example in section 3.3
(there are other similar places too).

Thanks, we will validate all HTTP examples before publication.


10. "," in "is appropriate for the access indicated (if present),"
should be removed or changed to "." (section 3.3)

Thank you, will fix!


11. The normative section refers to
https://www.rfc-editor.org/rfc/rfc7519 for JWTs. However, it can also
refer to https://www.rfc-editor.org/rfc/rfc9068 from Vittorio, and
that, in my eyes, could be a little better fit. Mainly, because of the
"typ" headers set explicitly to "at+jwt".

This can probably refer to both, in all honesty. We’ll add that reference in.


12. sentence "Expressed as a integer seconds from UNIX Epoch." should
be probably changed to "Expressed as an integer number of seconds from
the UNIX Epoch." in all places.


This language was fixed in core during IESG review, so we’ll align with that.


All the best,
Andrii

--
TXAuth mailing list
[e-mail]
https://www.ietf.org/mailman/listinfo/txauth