[GNAP] Paul Wouters' Discuss on draft-ietf-gnap-core-protocol-18: (with DISCUSS and COMMENT)

[Paul Wouters via Datatracker <[e-mail]>]

Paul Wouters has entered the following ballot position for
draft-ietf-gnap-core-protocol-18: Discuss

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/



----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

[updated on 08/03/2024, apologies for the delay)

First, let me thank you for a very clear document with the largest Security
Considerations section I've ever seen and an extensive Privacy Considerations
Section. I'll be using these as examples in the future :)

I have a a number of questions regarding the normative language in the document
that I'm populating mostly in the comments, but I wanted to hightlight two in
the discuss section:

1) Section 2:

        access_token (object / array of objects):

        Describes the rights and properties associated with the requested
        access token. REQUIRED if requesting an access token. See
        Section 2.1.

I believe "access_token" should be "access"? The whole thing is an
"access_token" and the example shows an undocumented "access" field. It also
matches the text and structure of Section 2.1.1 if this was called "access".
This also requires updating section 11.2.2. If "access_token" is really meant
here, the field below is misnamed "access". I am a bit confused as Section 12
shows a lot of implementations, so what am I missing here?

2) Section 13.28 on Exhaustion of Random Value Space

Why does this section not recommend using a KDF to produce random to prevent
exhaustion. Or if one believes that is dangerous as a default, to only do
this when it is running low on random? Even if reseeding a KDF every 5 minutes,
that is still a 99.999% reduction in required random ? Or if this is a really
bad idea, maybe add text to prevent people like me implementing it as "smart"?
:)


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

        Flag values MUST NOT be included more than once.

What is the action expected when it does appear more than once? Presumably
return an "invalid_request" error ?

Update [I-D.ietf-secevent-subject-identifiers] to [RFC9493]
Update [draft-ietf-uta-rfc6125bis-15] to [RFC9525]

        Possible values include id_token for an OpenID Connect ID Token

Is there a reason this is called "id_token" and not "openid_token" which
seems far more descriptive of what it is? I guess it is probably too late
to change this now :/

        If omitted, the AS SHOULD assume that subject information requests
        are about the current user and SHOULD require direct interaction
        or proof of presence before releasing information.

Why are these SHOULDs and not MUSTs? When can it be about a non-current user?
When can it omit requiring proo of presence?

        Client information MUST either be sent by value as an object or by
        reference as a string

This "MUST" really seems a "can be". I mean what other options are there than
by value or by reference?

        MUST exercise caution

What normative caution is that? In other words, if an implementer writes a test
case for each MUST and SHOULD in the document, how does one test this "MUST"?

        it SHOULD return an invalid_client error (Section 3.6) or
        interpret the request as if the class_id were not present

I don't know how the parse the applicability of this SHOULD combined with the
"or".

        MAY accept or reject the request

Similar here. What is the "not" clause of this MAY ?

        The client instance's key MAY be pre-registered with the AS
        ahead of time and associated with a set of policies and allowable
        actions pertaining to that client.

Here I also think a normative MAY does not make sense. It is a just a "the
client can"?

         Additional fields sent during a request but not present in a
         pre-registered client instance record at the AS SHOULD NOT be
         added to the client's pre-registered record.

Why not MUST NOT?

        A client instance that is capable of talking to multiple AS's SHOULD
        use a different key

Why not MUST? if it prevents known attacks?

        The instance identifier MAY be assigned to a client instance at
        runtime through a grant response (Section 3.5) or MAY be obtained
        in another fashion, such as a static registration process at
        the AS.

This also seems the MAYs are really "can". Compare this for example with

        An individual AS can define its own policies and processes for
        deciding when and how to gather the necessary authorizations
        and consent

Why is "can" and not MAY used here?

        If the client instance is identified in this manner

What is "in this matter"? The text leading up to this sentence talks about a
fatal error, so I'm confused.

        Display name of the client software. RECOMMENDED.

Why is this recommended? Isn't this a privacy leak that could make it easier to
find vulnerable software?

        the AS SHOULD reject the request with an unknown_user error (Section
        3.6).

Does the SHOULD bind to "reject" the the specific error "unknown_user" ? If it
binds to the reject, what is a reason to not reject a disallowed cross-user
authorization ?

NITS:

Is the common spelling "mTLS" or "MTLS" ?

I find the double links used a bit weird, eg:

         Additional assertion formats are defined by the GNAP Assertion Formats
         Registry (Section 11.5).

This makes the text "GNAP Assertion Formats Registry" a link as well as
"Section 11.5" and the links are to the same thing. I think only making the
"Section 11.5" a link would improve the readability by preventing very long
links (which also look like they could be links to outside the document. It
also sometimes uses "Section X" and sometimes "(Section X)". There are
paragraphs that are half blue due to this method of using linking. I really
personally like [link] to be clearly within the same document - currently the
links without brackets appear to be external links even though they are not.

If a number of examples follow each other, such as in Section 2.5, the text:

        In this non-normative example,

it becomes very confusing. If one has been scrolling, it is not clear if the
text applies to the example above or below the text. Maybe instead write "In
the following non-normative example" ?

Section 3.3.4:

 uri (string):

        The interaction URI that the client instance will direct the RO
        to. This URI MUST be short enough to be communicated to the end
        user by the client instance. It is RECOMMENDED that this URI be
        short enough for an end user to type in manually.

This worried me a bit as short URLs are mostly done by large wealthy
corporations, or via URL shorteners. The last one being a risk redirect. One
char off could get you a malicious page that looks like the real thing. Typing
in even a short URI by a user seems a security risk? I guess QR codes would
negate these though.

        It is RECOMMENDED that this code be no more than eight characters
        in length. REQUIRED.

How about 0? or 1 :P
As in, I think you are saying "a minimum of 8 and RECOMMENDED not more" ?

Section 3.3 and 3.4 seem to omit the "This non-normative example" preamble
that other sections have used.

Section 3.6

        If the AS determines that the request cannot be completed for
        any reason, it responds to the client instance with an error
        field in the response message. This field is either an object
        or a string.When returned as an object, the object contains the
        following fields:

        code (string):

        A single ASCII error code defining the error. REQUIRED.

Why is "code" not named "error_code" or "error_string" ? (error_code to
me feels numeric but I might just be too old). Again it is probably too
late to change this?

The error codes also return very specific error failure mode that could
help an attacker. Would it perhaps make sense to limit these to one
"request_denied" ? I know this is an issue of "easier debugging" vs "making
attacks harder" issue.

Are the error code descriptions flexible? Eg can they be changed based on the
known user's language preference? Are these descriptions static mandatory
strings (which is not clear to me that they are)

Section 4.1.2 and 4.1.3 contain a lot of duplicate text. I think it would be
clearer to reference it and highlight the differences (if any). Now one needs
to stare at trying to find differences.

Section 5:

        The new continue response MUST include a continuation access
        token as well, and this token SHOULD be a new access token,
        invalidating the previous access token.

Why SHOULD and not MUST?

Section 5.4:

        If the request is successfully revoked, the AS responds with
        status code HTTP 204 (No Content).

What should be returned if this fails?

Section 6.2:

The first SHOULD and MUST seem to not both be possible ?