A Set of Simple Authentication and Security Layer (SASL) Mechanisms for OAuth
draft-ietf-kitten-sasl-oauth-23

Another excellent, informative shepherd writeup, which helped with my review of the document.  (And, I'll say, most of the really useful writeups, as this one, seem to use the "essay" format, rather than the Q&A format.)  Thanks, Ben, for the writeup.

-- Section 1 --

   way to use the Simple Authentication and Security Layer (SASL)
   [RFC4422] to gain access to resources when using non-HTTP-based
   protocols, such as the Internet Message Access Protocol (IMAP)
   [RFC3501] and the Simple Mail Transfer Protocol (SMTP) [RFC5321],
   which is what this memo uses in the examples.

When I first read this, I thought the last phrase was bound to SMTP... but then I see that both IMAP and SMTP are used in the examples.  So, a small thing, but I'd end the sentence after the 5321 reference, and make the last phrase into another sentence, like, "This document gives examples of use in IMAP and SMTP."

-- Section 2 --

   Note that the IMAP SASL specification requires base64 encoding, see
   Section 4 of [RFC4648], not this memo.

Nit: That seems kind of an odd way to put it, in addition to its having a comma splice.  Why not this?:

NEW
   Note that the IMAP SASL specification requires base64 encoding, as
   specified in Section 4 of [RFC4648].
END

-- Section 3.2 --
Nit: "vaialble" -> "available"

-- Section 3.2.2 --

      scope (OPTIONAL):  An OAuth scope which is valid to access the
         service.  This may be omitted which implies that unscoped
         tokens are required.  If a scope is specified then a single
         scope is preferred, use of a space separated list of scopes is
         NOT RECOMMENDED.

Why is it "NOT RECOMMENDED"?  What interoperability or security issue is in play here that makes this "SHOULD NOT"?

         The server MAY return different URLs for users in
         different domains and the client SHOULD NOT cache a single
         returned value and assume it applies for all users/domains that
         the server suports.  The returned discovery document SHOULD
         have all data elements required by the OpenID Connect Discovery
         specification populated.

Similar questions for the SHOULD NOT and SHOULD here.  In this case, I don't understand why they aren't "MUST NOT" and "MUST": for the first, I don't understand how a client can interoperate with a server if it violates this, and for the second, if they data elements are "required", why isn't including them a "MUST"?

(Nit: The "if available" at the end of the paragraph is unnecessary; if one is not available, it certainly can't be used.)

-- Section 4.1 --

   The
   underlying TLS establishment is not shown but is required for using
   Bearer Tokens per that specification.

   S: * OK IMAP4rev1 Server Ready
   C: t0 CAPABILITY
   S: * CAPABILITY IMAP4rev1 AUTH=OAUTHBEARER SASL-IR
   S: t0 OK Completed

The problem with this is that if the underlying TLS establishment is done through STARTTLS on port 143, that would come between the first line ("S: * OK IMAP4rev1 Server Ready") and the second.  And that makes the example a bit odd.  The easiest way out of that is simply to remove the first line.  Or, better, to replace the first line with something like "[Initial connection and TLS establishment...]".  (This applies to the subsequent sections as well.)

I wonder why you don't wrap the decoded payload here the same way you do in Section 4.2 (or there the same as here).  I suggest that there'd be less room for any confusion if you did them all (4.3 also) the same way (I don't care which way you choose).

In the SMTP part, you say this:

   Note
   that line breaks are inserted for readability, and that the SMTP
   protocol terminates lines with CR and LF characters (ASCII values
   0x0D and 0x0A), these are not displayed explicitly in the example.

The same is true for the IMAP protocol and example, but you don't say this there.  Actually, I don't think you need to say it in either place, and suggest simply removing it here (and in Section 4.4).

-- Section 4.4 --
In the SMTP protocol, you should have "[Negotiate TLS...]" before the SMTP AUTH command, as you do in Section 4.1.

[All of the comments below have been addressed by the authors]

-- section 1, description of step A:

if the preferred way is different than the diagram, why not show it the preferred way in the diagram in the first place?

-- 1, 2nd to last paragraph

The "SHOULD" means the reference to I-D.ietf-oauth-dyn-reg needs to be be normative.

-- 3: "Such a new SASL OAuth mechanism can be added by simply
   registering the new name(s)"

Register them where?

-- 3.2, 2nd paragraph : "... known to the application."

Known to the "resource server"?

Editorial Stuff:

-- 3.1, "Port":

I assume that means the destination port to which the client connected? (similar to Host?)

-- 3.1.1 "Post": default value is "". 

Does "" represent an empty string?

-- 3.2, first sentence"

s/" ... according the specification..." / "... according to the specification..."

- 5: "Lifetime of the appliation sessions"

s/appliation/application

"It is possible that SASL will be authenticating..."

s/"... be authenticating..." / "... be used to authenticate..."

SASL mechanisms using this document as their definition do not
   provide a data security layer; that is, they cannot provide integrity
   or confidentiality protection for application messages after the
   initial authentication.  If such protection is needed, TLS or some
   similar solution should be used.  Additionally, for the two
   mechanisms specified in this document, TLS MUST be used for
   OAUTHBEARER to protect the bearer token; for OAUTH10A the use of TLS
   is RECOMMENDED.

Can someone explain to me situation were intergrity protection is not desirable (possibly rhetorical). it seems like it might be better to clarify what the exception is and use a blanket must for everything else.

Thanks for your work on this draft and for requiring TLS on bearer token use.