Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let client_secret to be optional for assertion grants #19

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bugant
Copy link

@bugant bugant commented Mar 23, 2012

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.

Matteo Centenaro added 2 commits March 23, 2012 16:43
@jcoglan
Copy link
Contributor

jcoglan commented Apr 2, 2012

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
Copy link
Author

bugant commented Apr 2, 2012

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
Copy link
Contributor

jcoglan commented Apr 3, 2012

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
Copy link
Author

bugant commented Apr 3, 2012

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
Copy link
Contributor

jcoglan commented Apr 3, 2012

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
Copy link
Author

bugant commented Apr 3, 2012

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
Copy link
Contributor

jcoglan commented Apr 3, 2012

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
Copy link

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
Copy link
Contributor

jcoglan commented Aug 9, 2012

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
Copy link
Contributor

jcoglan commented Aug 24, 2012

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
Copy link

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
Copy link
Author

bugant commented Jul 24, 2013

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants