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

[Brian Campbell <[e-mail]>]

Barry, thanks for the review, ballot position and comments. Please see
inline below for my replies to the latter.


On Thu, Jul 18, 2019 at 3:06 PM Barry Leiba via Datatracker <
[e-mail]> wrote:

> Barry Leiba has entered the following ballot position for
> draft-ietf-oauth-token-exchange-18: No Objection
>
> 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.


I always endeavor 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?
>

My initial response was going to be "sure, I'll add some bits in sec 1.1
along those lines to clarify that." However, as I look again at that
section for good opportunities to make such additions, I feel like it is
already said that impersonation is controlled. Notably there are these bits
of text:

"or for A to
   be granted a *limited access credential *to C but that continues to
   identify B as the authorized entity ("impersonation")."

"When principal A impersonates principal B, A is given all the rights
   that B has *within some defined rights context*"

So I think it already says that and I'm gonna have to flip it back and ask
if you have concrete suggestions for changes or additions that would say it
more clearly or more to your liking?




>
> — Section 8.2 —
> RFC 8174 needs to be normative, along with 2119.
>

Of course. Not sure how those got separated in the first place but I'll
move 8174 to normative.



> — 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.”
>

Will change to try and avoid that potential trip-up.




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

I didn't include an explicit reference here because TLS is transitively
referenced by other normative references (including 6749 of which this
whole thing is an extension) and TLS is pretty widely recognized even
without citation.

Or maybe it was just an oversight when I added that particular text.

I'm happy to add a citation here but it does raise the question of what the
most appropriate way to cite TLS is right now - 1.3, 1.2, or the BCP or
some combination thereof?



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

 Will take the suggestion.




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

Okay, request-and-response it will be.



>
> — 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”).
>

Makes sense, I'll make them the same.  And I think I'll go with “are not
used".

-- 
_CONFIDENTIALITY NOTICE: This email may contain confidential and privileged 
material for the sole use of the intended recipient(s). Any review, use, 
distribution or disclosure by others is strictly prohibited.  If you have 
received this communication in error, please notify the sender immediately 
by e-mail and delete the message and any file attachments from your 
computer. Thank you._