Re: [OAUTH-WG] Barry Leiba's No Objection on draft-ietf-oauth-token-exchange-18: (with COMMENT)

[Benjamin Kaduk <[e-mail]>]

Just on one point...

On Thu, Jul 18, 2019 at 02:06:10PM -0700, Barry Leiba via Datatracker wrote:
> Barry Leiba has entered the following ballot position for
> draft-ietf-oauth-token-exchange-18: No Objection
> 
> 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/iesg/statement/discuss-criteria.html
> for more information about IESG DISCUSS and COMMENT positions.
> 
> 
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-oauth-token-exchange/
> 
> 
> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> I have comments below, a couple of which might have been DISCUSS except that
> there have been enough eyes on this and enough DISCUSSes already, and I trust
> the authors and responsible AD to do the right thing.  So I’ve divided the
> comments between “higher importance” and “purely editorial”.
> 
> Of higher importance:
> 
> — Section 1.1 —
> Given the extensive discussion of impersonation here, what strikes me as
> missing is pointing out that impersonation here is still controlled, that “A is
> B” but only to the extent that’s allowed by the token.  First, it might be
> limited by number of instances (one transaction only), by time of day (only for
> 10 minutes), and by scope (in regard to B’s address book, but not B’s email). 
> Second, there is accountability: audit information still shows that the token
> authorized acting as B.  Is that not worth clarifying?
> 
> — Section 8.2 —
> RFC 8174 needs to be normative, along with 2119.
> 
> — Section 2.2.2 —
> 
>    If the authorization server is unwilling or unable to issue a token
>    for all the target services indicated by the "resource" or "audience"
>    parameters, the "invalid_target" error code SHOULD be used in the
>    error response.
> 
> I always trip when I read “all” in a context like this.  I think you mean
> “any”, because you have “a token”.  You could arguably make it “tokens” and
> leave “all”, but I think changing to “any” is clearer:
> 
> NEW
>    If the authorization server is unwilling or unable to issue a token
>    for any target service indicated by the "resource" or "audience"
>    parameters, the "invalid_target" error code SHOULD be used in the
>    error response and no tokens are returned.
> END
> 
> The danger with “all” is having a reader interpret the error as only occurring
> when the server fails *all* services, and thinking that failing one out of
> three still constitutes success.  I have seen this be an issue often (not with
> OAuth, but in general).  If you want to be absolutely clear you could even add
> to the end, “A request is successful only when all requested tokens are issued.”

I think I was reading this as "if the server can't fulfil the exact request
as given, it returns an error and says "try again slightly differently".

-Ben

> — Section 5 —
> 
>    In addition, both delegation and impersonation introduce unique
>    security issues.  Any time one principal is delegated the rights of
>    another principal, the potential for abuse is a concern.  The use of
>    the "scope" claim is suggested to mitigate potential for such abuse,
>    as it restricts the contexts in which the delegated rights can be
>    exercised.
> 
> I’m ambivalent here: is it worth also mentioning limiting the time a token is
> valid and possibly make it a one-time-use token?  Or is it that that’s
> adequately covered in the other references and shouldn’t be repeated here?
> 
> Also, is it worth referring (not copying) here to the advice in section 2.1
> about the importance of authentication?
> 
> — Section 6 —
> Should “TLS” here have a citation and normative reference?
> 
> =====
> 
> Purely editorial:
> 
> — Section 1 —
> 
>    An OAuth resource server, for example, might assume
>    the role of the client during token exchange in order to trade an
>    access token, which it received in a protected resource request, for
>    a new token that is appropriate to include in a call to a backend
>    service.
> 
> A suggestion: I think this would work better using a restrictive clause, rather
> than a non-restrictive one.
> 
> NEW
>    An OAuth resource server, for example, might assume
>    the role of the client during token exchange in order to trade an
>    access token that it received in a protected resource request for
>    a new token that is appropriate to include in a call to a backend
>    service.
> END
> 
>    The scope of this specification is limited to the definition of a
>    basic request and response protocol for an STS-style token exchange
> 
> There should be hyphens in “request-and-reaponse protocol” (compound modifier).
> 
> — Sections 4.1 and 4.4 —
> 
> Section 4.1 says this:
>    Consequently, non-
>    identity claims (e.g., "exp", "nbf", and "aud") are not meaningful
>    when used within an "act" claim, and therefore must not be used.
> 
> Section 4.4 says this:
>    Consequently,
>    claims such as "exp", "nbf", and "aud" are not meaningful when used
>    within a "may_act" claim, and therefore should not be used.
> 
> I agree that neither of these should be BCP 14 key words, but I still think
> that being consistent is important, and I urge you to make them the same: both
> “must not be used” or both “should not be used” (or perhaps both “are not
> used”).
> 
> (I did not review the appendices.)
> 
>