Re: [OAUTH-WG] Last call review of draft-ietf-oauth-dyn-reg-10

Phil Hunt <phil.hunt@oracle.com> Thu, 16 May 2013 01:17 UTC

Return-Path: <phil.hunt@oracle.com>
X-Original-To: oauth@ietfa.amsl.com
Delivered-To: oauth@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 5611711E80D7 for <oauth@ietfa.amsl.com>; Wed, 15 May 2013 18:17:32 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -5.348
X-Spam-Level:
X-Spam-Status: No, score=-5.348 tagged_above=-999 required=5 tests=[AWL=-1.030, BAYES_00=-2.599, HTML_FONT_FACE_BAD=0.884, HTML_MESSAGE=0.001, MIME_QP_LONG_LINE=1.396, RCVD_IN_DNSWL_MED=-4]
Received: from mail.ietf.org ([12.22.58.30]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id pHQiz8rNgK+a for <oauth@ietfa.amsl.com>; Wed, 15 May 2013 18:17:26 -0700 (PDT)
Received: from userp1040.oracle.com (userp1040.oracle.com [156.151.31.81]) by ietfa.amsl.com (Postfix) with ESMTP id 2E53711E80E9 for <oauth@ietf.org>; Wed, 15 May 2013 18:17:26 -0700 (PDT)
Received: from ucsinet22.oracle.com (ucsinet22.oracle.com [156.151.31.94]) by userp1040.oracle.com (Sentrion-MTA-4.3.1/Sentrion-MTA-4.3.1) with ESMTP id r4G1HNvj005195 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 16 May 2013 01:17:23 GMT
Received: from aserz7022.oracle.com (aserz7022.oracle.com [141.146.126.231]) by ucsinet22.oracle.com (8.14.4+Sun/8.14.4) with ESMTP id r4G1HNeP020400 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=FAIL); Thu, 16 May 2013 01:17:24 GMT
Received: from abhmt120.oracle.com (abhmt120.oracle.com [141.146.116.72]) by aserz7022.oracle.com (8.14.4+Sun/8.14.4) with ESMTP id r4G1HNsq027923; Thu, 16 May 2013 01:17:23 GMT
Received: from [192.168.1.125] (/24.85.226.208) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 15 May 2013 18:17:22 -0700
References: <85AA2C66-108B-4276-92EE-2D7566E54990@oracle.com> <6AF52CCD-4D6B-4696-8465-3345FFFDBE9C@mitre.org>
Mime-Version: 1.0 (1.0)
In-Reply-To: <6AF52CCD-4D6B-4696-8465-3345FFFDBE9C@mitre.org>
Content-Type: multipart/alternative; boundary=Apple-Mail-4F579264-EB8D-494B-A911-846D1B434983
Content-Transfer-Encoding: 7bit
Message-Id: <8E963F09-3631-4254-9C06-69E39461D094@oracle.com>
X-Mailer: iPhone Mail (10B329)
From: Phil Hunt <phil.hunt@oracle.com>
Date: Wed, 15 May 2013 18:17:18 -0700
To: "Richer, Justin P." <jricher@mitre.org>
X-Source-IP: ucsinet22.oracle.com [156.151.31.94]
Cc: "oauth@ietf.org WG" <oauth@ietf.org>
Subject: Re: [OAUTH-WG] Last call review of draft-ietf-oauth-dyn-reg-10
X-BeenThere: oauth@ietf.org
X-Mailman-Version: 2.1.12
Precedence: list
List-Id: OAUTH WG <oauth.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/oauth>, <mailto:oauth-request@ietf.org?subject=unsubscribe>
List-Archive: <http://www.ietf.org/mail-archive/web/oauth>
List-Post: <mailto:oauth@ietf.org>
List-Help: <mailto:oauth-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/oauth>, <mailto:oauth-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 16 May 2013 01:17:32 -0000

Justin,

I will look over your comments. But looking at some, i was not looking for an explanation but rather I think the text should explain.  

For example the registration access token seemed very mysterious. It took a lot of effort to understand what was going on. 

Maybe i am seeing this as I am looking at the spec with fresh eyes?

Phil

On 2013-05-15, at 17:53, "Richer, Justin P." <jricher@mitre.org> wrote:

> Phil, many thanks for the extremely thorough review! It is very useful indeed. 
> 
> My comments and responses to each point are inline.  
> 
> On May 15, 2013, at 4:30 PM, Phil Hunt <phil.hunt@oracle.com> wrote:
> 
>> It seems unfortunate that I have not gotten a chance to get into this level of detail much earlier. But then, I guess that's what LC review is for! My apologies for not getting many of these concerns to the WG much earlier.
>> 
>> Overall  
>> -----------
>> I think dynamic registration is a critical part of the OAuth framework now that we are starting to consider how individual client applications should operate when there is one or more deployments of a particular resource API. We've moved from the simple use case of a single API provider like Facebook or Flickr and moved on to standards based, open source based, and commercial based deployments where there are multiple service endpoints and many clients to manage.
>> 
>> The dynamic registration spec is actually dealing with a couple of issues that I would like to see made more clear in early part of the spec:
>> 
>> 1.  How is a new client application recognized for the first time when deployed against a particular SP endpoint?  Should clients actually assert an application_id UUID that never changes and possibly a version id that does change with versions?
> 
> In the general case, why is any recognition required? If you're doing things as part of a larger protocol ecosystem, like Blue Button+ or a particular OpenID Connect provider, then you can define semantics for tying together classes of clients (see below for more discussion on this very point). But in general, a client is just going to show up as a new instance to the AS and get issued a new, unique client_id, and that's that. 
> 
>> 
>> 2.  How are client credentials managed. Are we assuming client credentials have a limited lifetime or rotation policy?
> 
> The intent was that client_secret could be rotated, as indicated by the expires_at member of the response. If a client's secret expires, it calls the read operation on the Client Configuration Endpoint (§4.2) to get its new client_secret. If this is unclear in the current text (which I suspect it may be after multiple refactorings), then I welcome suggestions and specific text as to how to make that clear. 
> 
>>   How does a client authenticate the first time and subsequent times to the registration service?
> 
> This is a separate question all together, as it does not involve the client_secret or client_id at all. Rather, the first time the client shows up to the registration service, it may either:
>   - Not authenticate at all (this is the true public, open registration, and it is recommended that servers do this)
>  - Authenticate using an OAuth 2.0 token (which ATM means a bearer token). How the client gets that bearer token and what the bearer token means to the AS beyond "this client is authorized to register" is out of scope for the spec, by design.
> 
> Subsequent times that the same registered client (by which we mean the same instance of a client with a particular client_id) shows up at the registration endpoint (actually, the Client Configuration Endpoint), it uses its Registration Access Token that it was issued on initial registration. This is an OAuth 2.0 Bearer token that is unique to the client instance.
> 
>> 
>> 3.  How is versioning of clients managed? Should each new version of a client require a change in client registration including possibly changing client_id and authentication credential? I don't have a strong feeling, but it is something that implementors should consider.
> 
> This is up to the AS, really, and I don't think that any global policy would ever fly here. Especially if you consider that the entire notion of "version" is more fluid these days than it ever has been. I wouldn't mind adding a discussion in the security considerations if it merits mentioning though, so that we can help both client developers and server developers institute reasonably good policy.
> 
>> 
>> 4.  What is the registration access token? Why is is used? What is its life-cycle?  When is it issued, when is it changed? When is it deleted?
> 
> See the response above above and the definition in §1.2: 
> 
> Registration Access Token: An OAuth 2.0 Bearer Token issued by the Authorization Server through the Client Registration Endpoint which is used by the Client to authenticate itself during read, update, and delete operations. This token is associated with a particular Client.
> 
> If this can be clarified, I welcome text suggestions.
> 
>> 
>> If we distinguish between first time registration of a particular client software package, it is possible that somethings like authentication method can be negotiate in or out-of-band at that time. Subsequent registrations should only have parameters for items that could change per deployment (like tos_uri).  I think token_endpoint_auth_method is one thing that should not be negotiated per instance.
> 
>> When subsequent instances register, I have to ask the question, for example when would things like "token_endpoint_auth_method" be negotiated or be different for the same client software? Should not all instances use the same authentication method.
> 
> 
> I'm confused by this -- as far as the dynamic registration spec is concerned, all instances are separate from each other. All parameters change per instance. And instance, you should keep in mind, is defined as any one copy of the client code connecting  to any new authorization server. That pairing creates the client_id, and therefore the instance, and therefore the registration access token, and therefore the registration itself at a conceptual level. So there is no way other than per-instance for a client to ask for a particular token_endpoint_auth_method. Where else would the AS find out about it? And there's no way other than per-instance for the server to tell the client which token_endpoint_auth_method to use. All instances will probably ask for the same token_endpoint_auth_method from all auth servers they talk to, right? And each AS will tell each instance that registers with it to use a particular auth method. There is no way to tie together different instances across (or within) auth servers without specifying a significant amount of other machinery.
> 
> Which is not to say that it's not useful in some circumstances to tie together different instances of the same code across (or within) auth servers. This is why, with Blue Button+, we specified a specific token format for the initial access token that the clients use to call the registration endpoint the first time,  as well as a discovery protocol against a centralized server that handles pre-registration. All of this machinery is, in my opinion, a stupendous overkill for the general case, though if some folks find use for it outside of BB+ then it'd be a good thing to abstract out and make its own spec that extends the Dyn Reg spec in a fully compatible way. Furthermore, even in Blue Button+ we specify that all auth servers MUST also accept open registration, without an initial access token, where the client simply shows up and says "hey, here are my parameters." The auth server has no way to know in this case any kind of out-of-band negotiation for different things. In BB+ we are writing very specific policy guidelines about how to present the UX and things to the end user for all of these different cases. But again, all of this is specific to the BB+ use case, and I don't see value in dragging it in to the registration spec on its own. I believe it would be far too limiting.
> 
>> 
>> Finally, there seems to be an inconsistent style approach with draft-hardjono-oauth-resource-reg which uses ETags. Should this draft do so as well?
> 
> That's an individual submission, not a working group draft. Nobody has, until now, even mentioned the use of ETags here. UMA (where the resource registration draft comes from) uses ETags, hence their inclusion there. I personally don't see their utility here, though, and I wouldn't want to include a wholly new mechanism this late.
> 
>> 
>> Specific items:
>> ---------------------
>> "token_endpoint_auth_method"
>> 
>> There is some confusion as to whether this applies to server or client authentication.  Suggest renaming parameter to "token_endpoint_client_auth_method"
> 
> This is the first I've heard of this particular confusion. I'm fine with either name but at this stage I don't want to make syntax changes without very, very compelling reasons to do so.
> 
>> 
>> * Currently, the API only supports a single value instead of an array.  If the purpose is to allow the client to know what auth methods it supports, this should be an array indicated what methods the client supports - or it should not be used.
> 
> I would rather like this to be an array, personally, and brought it up with the OpenID Connect working group about six months ago. But there it was decided that a single value was simpler and sufficient for the purpose. The IETF draft has inherited this decision. I *believe* (though am not 100% positive) that I brought up this very issue in the WG here but didn't receive any traction on it, so single it remains.
> 
> Also note that this field, like all others in §2, represents two things: the client telling the server "I want to use this value for this field", and the server telling the client "this is the value you have for this field". It's not exactly a negotiation, more like making a polite request to an absolute dictator who has the last word anyway. But at least this dictator is nice enough to tell you what their decision was instead of just decapitating you.
> 
>> 
>> * There is no authn method for "client_secret_saml" or "private_key_saml".
> 
> Nobody has really asked that these two be included or specified. The extension mechanism (using an absolute URI) would allow someone else to define these. Is the definition in the SAML Assertion draft sufficient for their use?
> 
>> 
>> There are no profiles referenced or defined for "client_secret_jwt" or "private_key_jwt". Neither of the JWT or SAML Bearer drafts referenced cover these types of flows. They only cover bearer flows.  "client_secret_jwt" and "private_key_jwt" seem to have some meaning within OpenID Connect, but I note that OIDC does not fully define these either.
> 
> The JWT assertion draft does say how to use a JWT for client authentication, and the DynReg text differentiates between a client signing said JWT with its own secret symmetrically vs. a client using its own private key, asymmetrically.
> 
>> 
>> There is no authentication method defined for "client_bearer_saml" or "client_bearer_jwt" or "client_bearer_ref".  Since the bearer specs say this is acceptable,  the dynamic registration spec should accept these.
> 
> I don't understand this bit -- where are these defined in RFC6750? I don't even know what client_bearer_ref would refer to.
> 
>> 
>> A possible suggestion is to remove client_secret_jwt and private_key_jwt and define those as register extensions since these profiles are not defined.
> 
> That's possible, but they are in active use already. 
> 
>> 
>> 
>> About "tos_uri" and "policy_uri"
>> 
>> The distinction between tos_uri and policy_uri is unclear.  Can we clarify or combine them?
> 
> Terms of service and policy are two different documents. One is something that a user accepts (generally describing what the user will do), one is a statement of what the service provider (in this case, the client) will do. More importantly, we used to have only one, and several people asked for them to be split.
>  
>> 
>> Also, aren't these really URIs or are they meant to be URLs?
> 
> There was already discussion about this on the list: The IETF is apparently trying to deprecate URL in favor of URI in new specs. So in practice they'll nearly always be URLs, but since all URLs are URIs we're not technically incorrect in following the new policy. And it makes the IESG less mad at us, which is a plus.
> 
>> 
>> About "jwks_uri"
>> 
>> A normative reference for http://tools.ietf.org/html/draft-ietf-jose-json-web-key-09 is needed. 
> 
> Yes, you're correct, I'll add that.
> 
>>  
>> Should be URL instead of URI?
> 
> See above. :)
> 
>> 
>> Section 2.1
>> 
>> In the table urn:ietf:params:oauth:grant-type:jwt-bearer
>> is missing.
> 
> It's there in the copy of -10 I'm reading off of ietf.org right now … ?
> 
>> 
>> “As such, a server supporting these fields SHOULD take steps to ensure that a client cannot register itself into an inconsistent state.”
>> 
>> We may want to define more detailed HTTP error response. E.g. 400 status code + defined error code (“invalid_client_metadata”)?
> 
> I'd be fine with defining a more explicit error state in this case. I think it would help interop for the servers that want to enforce grant-type and response-type restrictions, but servers that can't or don't care about restricting grants types and whatnot don't have to do anything special.
> 
>> 
>> Section 2.2
>> 
>> May want to add:
>> 
>> When “#” language tag is missing, (e.g. “#en” is missing in “client_name”, instead of “client_name#en”) the OAuth server may use interpret the language used based on server configuration or heuristics.
> 
> That seems inconsistent with what we already have:
> 
> If any human-readable field is sent without a language tag, parties using it MUST NOT make any assumptions about the language, character set, or script of the string value, and the string value MUST be used as-is wherever it is presented in a user interface.
> 
> Which is to say, treat it as a raw byte-value-string and don't try to get fancy.
> 
>> 
>> Section 3
>> 
>> Existing text:
>> 
>> “In order to support open registration and facilitate wider interoperability, the Client Registration Endpoint SHOULD allow initial registration requests with no authentication.  These requests MAY be rate-limited or otherwise limited to prevent a denial-of-service attack on the Client Registration Endpoint.”
>> 
>> I would suggest to change the first “SHOULD” to “MAY”.   In most cloud services, the first client is registered by a human user. Then, other clients can be further used to automate other client registration.  So, the first request would typically come with an authenticated user identity. 
> 
> I think the weight of "SHOULD" is appropriate here, because I believe that turning off open registration at the AS (which is what this is talking about) really ought to be the exception rather than the rule. 
> 
>> 
>> On the flip side, the earlier paragraph:
>> 
>> “The Client Registration Endpoint MAY accept an initial authorization credential in the form of an OAuth 2.0 [RFC6749] access token in order to limit registration to only previously authorized parties. The method by which this access token is obtained by the registrant is generally out-of-band and is out of scope of this specification.”
>> 
>> I tend to think it would be better to change this “MAY” to “SHOULD”.
>> 
>> That access token would carry a human user authenticated identity somehow. In some cases, it can be a pure authenticated user assertion token.
> 
> Again, disagree, for the same reasoning as above.
> 
>> 
>> About section 4.3:
>> 
>> If the client does not exist on this server, the server MUST respond
>>    with HTTP 401 Unauthorized, and the Registration Access Token used to
>>    make this request SHOULD be immediately revoked.
>> 
>> If the Client does not exist on this server, shouldn't it return a "404 Not Found"?
>> 
>> If revoking the registration access token, is it bound to a single client registration?  This is not clear.  What is the lifecycle around registration access token? Only hint is in the Client Information Response in section 5.1.
> 
> The language about the 401 here (and in other nearby sections) is specifically so that you treat a missing client and a bad registration access token the same way. You see, returning a 404 here actually indicates things were in an inconsistent state. Namely, that the registration access token was still valid but the client that the registration access token was attached to doesn't exist anymore. The registration access token in meant to be tied to a client's instance (or registration), so it's actually more sensible to act as though the registration access token isn't valid anymore. In at least some implementations (specifically ours at MITRE that's built on SECOAUTH in Java), you'd never be able to reach the 404 state due to consistency checking with the inbound token.
> 
> Since the intent of the registration access token is that it's bound to a single instance, its lifecycle is generally tied to the lifecycle begins at the issuance of a new client_id with that instance. That token might be revoked and a new one issued on Read and Update requests to the Client Configuration Endpoint (and the client needs to be prepared for that -- same as the client_secret), and it will be revoked when the client is deleted either with a Delete call to the Client Configuration Endpoint or something happening out-of-band to kill the client. 
> 
> Should we add more explanatory text to the definition in the terminology section? Elsewhere? I'm very open to concrete suggestions with this, since I don't think it's as clear as we'd like.
> 
>>  
>> About section 5.1:
>> 
>> Is registration_access_token unique?  Or is it shared by multiple instances?   If shared, then registration_access_token can't be revoked on delete of client.
>> 
>> Suggest rename “expires_at” to “client_secret_expires_at”, 
>> 
>> Suggest to rename “issued_at” to “id_issued_at”
> 
> While I do like the suggestion of changing these to client_secret_expires_at and client_id_issued_at, and I think these are more clear and readable, I don't want to change the syntax during last call unless there is a clear need and a clear consensus for doing so.
> 
>> 
>> Section 7
>> 
>> When a client_secret expires is it the intent that clients do an update or refresh to get a new client secret?
> 
> Yes, the client is supposed to either call the read or update methods on the client configuration endpoint to get its new secret. As discussed above, I'm not sure that's as clear as it needs to be, and I welcome suggestions on how to clarify this.
> 
> 
> 
> Again, thanks for the very thorough read through. Have you implemented any of the spec yet, either as-is or with any of your suggested changes?
> 
>  -- Justin