Subject: Re: [kitten] WGLC on draft-ietf-kitten-sasl-oauth-15

From: Benjamin Kaduk <[email-masked]>
Date: Fri, 5 Sep 2014 13:05:06 -0400 (EDT)

On Wed, 20 Aug 2014, Shawn M Emery wrote:

> This message officially starts the 3rd kitten Working Group Last Call
> for the following document:
>
> A set of SASL Mechanisms for OAuth
> http://tools.ietf.org/html/draft-ietf-kitten-sasl-oauth-15
>
> The Working Group Last Call for this document starts today on Wednesday,
> August 20th and will end on Wednesday, September 3rd.

Oops, I'm late.  Sorry.

I'm neither a SASL expert nor an OAuth one, but I think I can get the
general sense of things here.  OAuth is already well established for
webapps, and there's a desire to repurpose it for authorizing access to
resources over non-http protocols, in particular protocols using SASL for
authentication.  This document, then, specifies how to represent the OAuth
messages inside SASL mechanisms, for messages between the parties
described in the OAuth terminology as the "client" and "resource server".
Since the OAuth messages are already specified, this is mostly just a
translation, along with specifying which pieces are required.

The most significant comment I have concerns section 3.2.2, the server
response to failed authentication.  I fear that it is under-specified, and
inconsistent with the example in section 4.3:

Section 3.2.2 leaves me wishing there was more detail.  I see that section
4.3 includes an example of this JSON object, but I think that more detail
is appropriate in a protocol specification.  Are all of the values always
going to be strings?  (Is the "OAuth 2 core specification" just RFC 6749?
If so, it's better to cite it by number.)  The IANA OAuth Extensions Error
Registry seems to give errors by name, e.g., "invalid_token".  Pulling up
RFC 6750, it seemse that when invalid_token is used, "the resource SHOULD
respond with the HTTP 401 (Unauthorized) status code".  But the example in
section 4.3 uses the string "401", not the JSON number, nor the string
form of the name!  This is very confusing.
Likewise, for the 'scope' named value, what are valid values?  Is there a
list of them somewhere?  Are they arbitrary, chosen by the application?
This needs to be specified.
The oauth-configuration named value is in better shape, as a URL is pretty
inherently a string, but that could still be made explicit.

My other comments are largely editorial; I have placed the more important
ones first and the stuff that's probably very minor at the end (after a
note).

However, it's a fairly significant issue that all of the IMAP examples do
not actually indicate they are for IMAP.  The SMTP exchanges are indicated
as such, which makes me suspect that originally only IMAP examples were
present, with the SMTP examples added later and the IMAP examples not
adjusted to reflect that they are only one class of example.  In
particular, this affects the top of sections 4.1, and 4.3.

I see that the notation "%x01" is used throughout to indicate the octet
with value 0x01, and this is used without comment in RFC 7159 (the JSON
spec).  I guess this is standard in ABNF-land, but section 2 doesn't
mention that we assume familiarity with such terminology.

The C/S exchanges in section 4.3 seems to have lost the "Server Ready" and
"t0 CAPABILITY" lines that are present in the previous examples.

At the end of section 4.3, I would clarify that the required dummy
response (as seen in the example as "AQ==") is the base64 encoding of
%x01, which was mentioned earlier in the text.   This happens again at the
end of section 4.4, but may not need the additional clarification at the
second occurrence.

In section 5, under "Access tokens have a lifetime", I think that "The
client may request a new access token" should use the RFC 2119 "MAY".

In section 6, the example is given of "when SASL is used in the context of
IMAP the resource server may assert the resource owner's email address to
the IMAP server for usage in an email-based application.".  But isn't the
IMAP server the resource server here?  Maybe it is the authorization
server which is making this assertion?

In section 3.1, for 'host', can we be more specific?  Is this a DNS name?
The name that would have been presented in an HTTP header if this was
being carried out over HTTP?  Vague text like this could lead to
interoperability issues.

Also in 3.1, the sentence mandating that the server MUST fail requests
that do not have host and port values but use keyed message digests could
be made more clear.  I'm uncertain which sort of requests do require keyed
message digests, so I don't really have any suggestions.

The first sentence of section 3.1.1 mentions the query string, which is
covered in the previous section (3.1); I think one should be moved so they
are in the same place.

In 3.2.1, I would be more comfortable if the second paragraph gave me some
reason to think that doing so is possible.  Is there prior art, or some
other specification for a similar thing that gives an example?  We don't
need to enumerate every way it could be done, but it seems useful to give
an indication that this is not going to end up dropping a heraculean task
on the developer.

Editorial minutiae:

The phrase "or by allowing the third-party application to obtain access on
its own behalf", used in both the abstract and introduction, seems a
little confusing to me.  Is this just supposed to be differentiating
between flows where the user sees a prompt page from the authorization
server and flows where the user does not?

The first paragraph of page 4 has the sentence "The framework also
provides a protocol for security subsequent protocol exchanges within a
data security layer.", which uses the word 'protocol' twice with two
different usages.  It is probably best to add a qualifier to at least one
occurrence.

The end of section 1 notes that "In band discovery is not tenable if
clients support the OAuth 2.0 password grant."  It would be nice to say
something about why.

The text in the first paragraph of 3.1 about "carry the equivalent values
from an HTTP context [...]" could be more clear.  I might say that these
key/value pairs "take the place of the corresponding HTTP headers and
values to convey the information necessary to complete [...]".

I would probably replace "The following key/value pairs are defined in the
client response" with "The following keys and corresponding values are
defined in the client response", since the list is not of key/value pairs
but rather a list of keys, with the text content for each item being a
description of the value to be used for that key.

For 'auth', "for an equivalent HTTP OAuth authorization" is an awkward
phrasing, since "equivalent" is not really specified; I might use
something like "The payload that would be in the HTTP Authorization header
if this OAuth exchange was being carried out over HTTP".

Please insert a comma before "Figure" in the first sentence of the fourth
paragraph of section 1.

On page 5, in the large paragraph, "a discovery mechanisms" is a
singular/plural mismatch.

In section 3.1, replace "or may be empty" with "which may be empty".

In section 3.2, I would replace "per the specification" with "according to
the specification".

In section 3.2.2 (last sentence), "presume an empty scope (unscoped token)
is needed" is a little odd; it might be better to say "should be used"
instead of "is needed".

-Ben

---

From: Bill Mills <[email-masked]>
Date: Fri, 5 Sep 2014 17:14:58 -0700

Comments/responses inline below.

Updated  pre -16 copy available at 

text: https://drive.google.com/file/d/0B99YSkxS6ZefdHFKTUhodzY3cjA/edit?usp=sharing
html: https://drive.google.com/file/d/0B99YSkxS6ZefejFSLU93T1Nldmc/edit?usp=sharing

You'll need to download the HTML to view it in a browser.

On Friday, September 5, 2014 10:05 AM, Benjamin Kaduk <[email-masked]> wrote:

Oops, I'm late.  Sorry.

***
[bill] no worries

***

***
[bill] Yep, this was wrong and 401 is being changed to "invalid_token" in the example.

***

***
[bill] Scope is defined in OAuth 2.0 and is a string, and it may be a list of space separated 
scope names.  There are no predefined scopes other than the blank/empty one..

***

***
[bill]  yep.  fixing.
***

***
[bill] that's actually HTTP (URI?) entity encoding.
***

***
[bill]  yep.  fixing.
***

***
[bill]  yep.  fixing.
***

***
[bill]  OK
***

***
[bill]  it's the client asserting the email address, this now reads "in the context of IMAP the client may assert the resource owner's email address".
***
In section 3.1, for 'host', can we be more specific?  Is this a DNS name?
The name that would have been presented in an HTTP header if this was
being carried out over HTTP?  Vague text like this could lead to
interoperability issues.

***
[bill]  Changed to: "Contains the host name to which the client connected, in an HTTP context this is the value of the HTTP Host header.
***

***[bill]  Changed to: "For OAuth token types such as OAuth 1.0a that use keyed message digests"
***

***
[bill]  moved qs to the "reserved for future use section 3.1.1
***

***
[bill]  OAuth 1.0a tokens may well (and commonly have in some implementations) either carried or has access to the client identity.

For the question of whether both identities can be communicated to the app, it would be a new capability you'd have to add to the SASL implementation you're using.  Not herculean, but not trivial if you're going to patch something like Cyrus SASL and keep it in sync with current releases.
***

Editorial minutiae:

***
[bill] the difference is whether you're allowing FaceBook to access your mailbox, or whether its a desktop app like Thunderbird.
***

***
[bill] IIRC this is lifted from the SASL spec itself.  That said, deleting the second usage seems to do the trick, " The framework also provides a protocol for securing subsequent exchanges within a data security layer."
***
The end of section 1 notes that "In band discovery is not tenable if
clients support the OAuth 2.0 password grant."  It would be nice to say
something about why.

***
[bill] Now there's a fun topic.  There are folks that assert that allowing in band discovery if the password grant is in play might lead to an effective phishing attack.  Other's disagree.  No one hated the current language.  I'm really tempted to leave it alone.
***

***
[bill]  OK
***

***
[bill]  sure, why not.
***

***
[bill]  OK
***

***
[bill]  and in the XML it's 'environment, <xref target="overview"/>'
***

***
[bill]  OK
***

***
[bill]  no, because the empty client response containing only the 0x01 has no GS2 header, now reads, "header followed by zero or more key/value pairs, or may be empty."
***

***
[bill]  OK
***

***
[bill]  Now reads, "If the resource server provides no scope to the client then the client SHOULD presume an empty scope (unscoped token) is required to access the resource."
***

-Ben

***
[bill]  Many thanks!
***

_______________________________________________
Kitten mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/kitten

---

From: Benjamin Kaduk <[email-masked]>
Date: Mon, 8 Sep 2014 13:00:10 -0400 (EDT)

> [bill] that's actually HTTP (URI?) entity encoding.

So we think that familiarity with RFC 6749 (OAuth 2.0) implies an
understanding of this usage?  If so, that's fine by me.

> [bill]  it's the client asserting the email address, this now reads "in the
> context of IMAP the client may assert the resource owner's email address".
> ***

Okay, thanks.

> [bill]  Changed to: "For OAuth token types such as OAuth 1.0a that use keyed
> message digests"
> ***

That helps some.  I think it was the text which is normative for the
server's actions that left me unsatisfied, though.  Seeing this now, I
suggest "server MUST fail an authorization request requiring keyed message
digests that is not accompanied by host and port values" (so, changing
from "do not have" to "is not accompanied by").  This does slightly change
the assumption of what blob of data is an "authorization request", i.e.,
is it the client response, or the value corresponding to the auth key.  If
it's the former, then my suggestion is probably not correct.

> [bill]  OAuth 1.0a tokens may well (and commonly have in some implementations)
> either carried or has access to the client identity.
>
> For the question of whether both identities can be communicated to the app, it
> would be a new capability you'd have to add to the SASL implementation you're
> using.  Not herculean, but not trivial if you're going to patch something like
> Cyrus SASL and keep it in sync with current releases.
> ***

So, an extension to the SASL library's API?  I'm not immediately coming up
with a good way to include that in the text, so maybe we can leave it
as-is.

> [bill] IIRC this is lifted from the SASL spec itself.  That said, deleting the
> second usage seems to do the trick, " The framework also provides a protocol for
> securing subsequent exchanges within a data security layer."
> ***

Yup, that seems to work well.

> [bill] Now there's a fun topic.  There are folks that assert that allowing in
> band discovery if the password grant is in play might lead to an effective
> phishing attack.  Other's disagree.  No one hated the current language.  I'm
> really tempted to leave it alone.
> ***

Okay.  I don't want to go from uncontroversial text to controversial text.

> [bill]  no, because the empty client response containing only the 0x01 has no GS2
> header, now reads, "header followed by zero or more key/value pairs, or may be
> empty."
> ***

Ah, I see now.  Sorry for the confusion.

-Ben

---

From: Bill Mills <[email-masked]>
Date: Mon, 8 Sep 2014 13:32:18 -0700

Changed to your text on the host/port thing.

Thanks again!

On Monday, September 8, 2014 10:00 AM, Benjamin Kaduk <[email-masked]> wrote:

Okay, thanks.

-Ben