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

[Paul Wouters <[e-mail]>]

Hi Justin,

Thanks for the quick turnaround and thanks for the explanations. With the
PR merged, my DISCUSS is resolved. I do
hope you don't forget to talk to the RFC Editor on those long blue bleeds
into the document and get those fixed. That would
make the document more readable.

Again thanks for the document. I think this was the only time I ever had to
delete like 4 concerns I had when I reached
the Security Considerations section and those concerns were talked about
there :)

Paul




On Fri, Mar 8, 2024 at 4:23 PM Justin Richer <[e-mail]> wrote:

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