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

[Justin Richer <[e-mail]>]

Hi Paul, thanks for the thorough review. Comments inline below.

Where changes have been made, they have been applied in this PR unless otherwise indicated: https://github.com/ietf-wg-gnap/gnap-core-protocol/pull/535

— Justin

On Mar 8, 2024, at 12:15 PM, Paul Wouters via Datatracker <[e-mail]> wrote:

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?


This is an incorrect reading of the section. The "access_token" field is an object whose structure is defined by section 2.1.1 (or an array of those objects, if you’re requesting multiple tokens, defined in 2.1.2). The "access_token" field includes the "access" field, which is not undocumented at all — it’s one of the fields defined in section 2.1.1. The "access" is only one part of the request, as it could also include labels, flags, or other extensions not yet defined. All of these are things that modify the request for the access token specifically. This is why it’s described as "rights and properties", not just "access rights". So to summarize:

- access_token: object or array of those objects that describe what you want in the access token you get back

Each of these objects consists of:

- access: array of objects or strings (or a combination) that says what you want the token to be able to do
- label: a machine-facing string for the client to know the token by (required when you have multiple in a single request, optional otherwise)
- flags: array of strings that turn on/off specific functionality about how the token is handled and override defaults (such as enabling bearer tokens)

There’s already a forward reference from the access_token field definition to section 2.1. This is a fairly fundamental part of the protocol, but if there’s a better way to phrase this connection between the items, we’d appreciate a proposal. The above set of suggestions is not correct and will not be applied as they change the protocol.

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"?
:)

I am not an expert on pseudorandom number generation and will gladly take advice on what we should guide people to use here to avoid the problem described.



----------------------------------------------------------------------
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 ?

Thanks, we’ve added a note that it’s an "invalid_flag" error.


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

Thanks, updated UTA (the sec event update is in https://github.com/ietf-wg-gnap/gnap-core-protocol/pull/534)


       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 :/

This is the term used internally by OpenID Connect and was requested by members of the OpenID community back in the early days of GNAP.


       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?

This is just one possible signaling mechanism for describing user information, others could be some keying mechanism on top of the client’s, an out of band communication method like DIDComm, or other kinds of things. A fully-featured identity protocol would need to be built on top of GNAP in order to fully define all the corner cases that crop up. The thinking here was that we can at best provide guidance of expected behaviors, and thus this was made a SHOULD.


       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?

Yes, that’s right — we’ve removed the normative and changed it to declarative.


       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"?

Point taken - though I personally think this could be tested via policy review and documentation. We changed to declarative, as in the previous one.


       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".

We are giving guidance on two options:

 - return a specific error
 - pretend like the field was never sent (ignore it)

Reading this again, a MUST makes more sense here and has been changed and the resulting sentence simplified.


       MAY accept or reject the request

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

The goal here was to say that the AS has the choice of what to do if it hasn’t seen the key before — in other words, it can do a dynamic trust based on some external element, or it can decide that’s too dangerous and shut it down right there.

We changed this to a declarative statement that the AS can process the request and it has flexibility over what it does with the results.


       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"?

That’s fair, changed to declarative.


        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?

In case the AS has a policy to remember these values sent at runtime.


       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?

Because it’s not always up to the client to make new keys, and it’s undetectable by an AS if the client is using a different key anyway. Additionally, the attack can be mitigated by having a strong association between AS and RS, which is likely to be more common in practice anyway (if OAuth deployment patterns have shown us anything).


       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?

The former is a protocol action and indicates choice, the latter is a policy action. It’s not a hill I really want to die on and they could be changed to be declarative, but I do think it makes sense in this case.


       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.

This wording is confusing — it is talking about referral by reference instead of value, and I don’t think that it actually has the right normative requirement as intended. Since the driving requirement is that symmetric keys are never sent by-value over the wire, we were trying to point out that one way to use symmetric cryptography is to pass client information entirely by reference. However, that is ancillary to what this section is about, so we’ve simply deleted this sentence to avoid confusion. The use case is better covered elsewhere, so we did that.


       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?

I’m not sure what you mean here — the display name is presented by the client for use during interaction and management of grants. How does this value leak privacy or identify vulnerable software?

It’s equivalent to the `client_name` field in RFC7591: https://datatracker.ietf.org/doc/html/rfc7591#section-2


       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 ?

The SHOULD is for the whole sentence. As stated in the other response thread:

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.


NITS:

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

I think it’s MTLS but I’m happy to be corrected here if that’s wrong. If anything, I’ve seen both in the wild, but that’s not indication of what’s the right way to do it. RFC8705 always spells out "mutual TLS", so we could probably take that tactic to avoid this question entirely.


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.

Thanks, this is probably an issue with the tooling more than anything. The markdown source uses the longer link format, which is probably the source of this behavior. I think we can work through that with the RFC Editor.


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" ?

Good point, we’ve scanned through and changed that to be consistent and clearer.


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.

The short URLs are a usability consideration as much as a security consideration — there are trade offs. The goal here is that we don’t want to rely on a user typing in a long and complex URL by hand, especially from a device that can’t launch a browser directly (like a set-top box or smart speaker, for example).

QR codes are a good alternative — but if you’re doing that, then they don’t need to be short anymore, and you can use the `redirect` mechanism to communicate a full URL with high entropy.


       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" ?

Again with the above, it’s meant to be implementation guidance in the trade offs between usability and security. In my personal experience, these tend to be 6 or 8 characters long, so we can add that to the recommendation.


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

Section 3.4 does have that text. The subsections of 3.3 don’t, but they’re all single simple examples so I’m not sure we need the preamble there in the same way.


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?

It’s named "code" because it’s underneath the `error` data structure already. So it comes out to:

{
"error": {
"code": "invalid_client",
"description": "Don’t do that you’ll break things"
}
}


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.

This is a perennial issue to balance between a descriptive protocol and preventing oracle attacks. I think it makes sense to differentiate between errors when the response to the error could be different in well-meaning software, which is where we tried to draw the line with the defined errors.


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)

They are free strings that are meant to explain the error in plain language (or, at least, developer-facing language). We’ve added an explanation to that effect.


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.

Long ago there was only one method with an optional parameter, and it was broken out based on developer and WG feedback. The editors felt it was better to have a complete explanation of each method instead of trying to build an abstraction between the two.


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?

Different token ecosystems have different capabilities around token revocation and rotation. Ultimately, rotation the continuation token is a security decision. It’s similar to OAuth 2’s "refresh token" and follows similar rotation advice.


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?

Thanks, the intended error code has been added.


Section 6.2:

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

The second is a "MUST if possible" and it’s specific to a circumstance, so it’s stronger advice to implementors than the SHOULD in the general case.




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