songkick / oauth2-provider

Simple OAuth 2.0 provider toolkit
MIT License
529 stars 148 forks source link

Let client_secret to be optional for assertion grants #19

Open bugant opened 12 years ago

bugant commented 12 years ago

OAuth2's draft#10 state that client credentials must be verified for an assertion grant only if present. http://tools.ietf.org/html/draft-ietf-oauth-v2-10#section-4.1.3

It is now possible to obtain an access token for a specific client via an assertion grant request without revealing the client's secret.

jcoglan commented 12 years ago

I'm not sure about this. The spec is slightly ambiguous about this; client_secret is also not listed as a required parameter for authorization_code exchanges. Whether this is because you can pass it as a header or because it's really not required is not clear.

In any case, not requiring client_secret means the client's identity is not proven, unless the assertion type provides a way of proving the client's identity. What kind of assertions are you using?

bugant commented 12 years ago

James Coglan said:

I'm not sure about this. The spec is slightly ambiguous about this; client_secret is also not listed as a required parameter for authorization_code exchanges. Whether this is because you can pass it as a header or because it's really not required is not clear.

exactly! I have the same doubts you have.

In any case, not requiring client_secret means the client'd identity is not proven, unless the assertion type provides a way of proving the client's identity. What kind of assertions are you using?

You're right. I'm using Facebook and similar providers for my assertions. The thing is, I would not like to put the client secret in all clients code (these will be mobile and web apps). In this way, though, I'm only authenticating a user, not the client app.

jcoglan commented 12 years ago

Certainly, if the secret is stored client-side, then you can't trust the identity of the client making the assertion, unless the assertion somehow contains the client's identity, which Facebook access tokens don't (although they do indicate a certain level of trust between the user and the client app).

While the spec is not clear on this, and appears to make client_secret optional, I wonder if our API may need adjusting to reflect this, i.e. that assertion handlers do not guarantee the client's identity, or make it optional. Maybe it's worth reading the latest draft for changes in this area for possible approaches we could take.

bugant commented 12 years ago

Looking at the last draft, they removed grant_type "assertion" and instead provide an absolute URI (the one that was previously supplied via the assertion_type) to use assertions.

I cannot find any mention of client_id and client_secret in the assertion specification and in its example. This is not entirely clear to me... That way they are issuing a token for no client, right?

jcoglan commented 12 years ago

Okay that sounds... weird. Let me find time to read it and see what I think.

With your application, does it only exist client side, or does it run on confidential servers too? That is, does exposing the client_secret compromise the security of other software that would otherwise be secure?

bugant commented 12 years ago

At the moment, exposing the client_secret would not lead to any security issue. But this sounds strange since it shoud be kept secret.

jcoglan commented 12 years ago

It should be secret, but as the spec acknowledges, some clients cannot keep their credentials secret -- this is made explicit in later drafts with the public and confidential client types.

For authorization, the redirect_uri protects against forgery by guaranteeing the provider will only deliver tokens to the correct host, but for exchanges there is no such guarantee since it's just a normal requests that's not mediated by a user-agent.

swishstache commented 11 years ago

James, per your gist (https://gist.github.com/3054344) it would seem clarification was made in later drafts that the client_id and client_secret need only be validated if they're both provided. Given that, do you anticipate merging this change?

jcoglan commented 11 years ago

That gist refers to a different draft. In cases where draft-10 is ambiguous or undefined, I tend to refer to future drafts and/or common sense for security advice.

I'm still not sure what to do about this.

jcoglan commented 11 years ago

The spec actually says "validate the client credentials (if present)" for all request types. This seems like a misfeature, since if you allow an authorization_code request (designed for server-side clients) without credentials then the client never authenticates. This case is the one where you can keep the credentials secret and have decent protection against fraudulent clients.

However you could imagine a client that operates in multiple places and therefore has its credentials exposed. If we allow credential-less exchanges then the client can authenticate properly in cases where its credentials are available, and not require those credentials to be deployed to unsafe environments. But this provides no incentive for the client to authenticate at all, and seems like a weird design. Anyone have any further thoughts on this?

swishstache commented 11 years ago

FWIW: I had interest in this initially because our clients were in javascript and mobile leading me to only "authenticate" the client based on the client_id. Since then we've moved to issuing per-client credentials (both an id and secret). This solved a number of things including allowing me to authenticate client identities more confidently.

As mentioned earlier, if the assertion can also guarantee the client identity, this seems reasonable. Given that, this feels edge-casey to me.

bugant commented 10 years ago

@jcoglan if you like I can re-post the PR to match the new code layout. Please, let me know if you're interested in getting it