Coder Social home page Coder Social logo

Comments (25)

danilobuerger avatar danilobuerger commented on April 28, 2024 1

Sure, i could give it a shot.

from fosite.

aeneasr avatar aeneasr commented on April 28, 2024 1

@danilobuerger thanks for being persistent on this issue, you are now a part of the hall of fame ec24a6e

from fosite.

aeneasr avatar aeneasr commented on April 28, 2024

Access Requests are requests on the oauth2/token endpoint which does not support public clients as per IETF spec. Public clients are supported through the oauth2/auth endpoint and grant types like implicit or hybrid.

from fosite.

danilobuerger avatar danilobuerger commented on April 28, 2024

Could you please point me to the relevant section in the rfc where it excludes public clients from the token endpoint?

from fosite.

aeneasr avatar aeneasr commented on April 28, 2024

https://tools.ietf.org/html/rfc6749#section-3.2.1

from fosite.

aeneasr avatar aeneasr commented on April 28, 2024

The only grant type that allows public clients is the implicit flow as defined here. This flow does not support refresh tokens and thus does not need the /token endpoint. Hence the authentication restriction on the /token endpoint. Without proper authorization on that endpoint, your system will be highly vulnerable.

from fosite.

danilobuerger avatar danilobuerger commented on April 28, 2024

One of us must be reading that section wrong. I can't seem to find any mention that public clients should not make requests to the token endpoint.

As an example see https://tools.ietf.org/html/rfc6749#section-4.1.3 (which is a post to /token):

ensure that the authorization code was issued to the authenticated
confidential client, or if the client is public, ensure that the
code was issued to "client_id" in the request,

Why would it state if the client is public if a public client is not even allowed to use the /token endpoint?

from fosite.

aeneasr avatar aeneasr commented on April 28, 2024

You skipped the important part ;) The request body parameters are hints, clients MUST still authenticate:

If the client type is confidential or the client was issued client
credentials (or assigned other authentication requirements), the
client MUST authenticate with the authorization server as described
in Section 3.2.1.

from fosite.

aeneasr avatar aeneasr commented on April 28, 2024

Ah sorry, my bad, I overread that if the client was issued client credentials. I must investigate that.

from fosite.

danilobuerger avatar danilobuerger commented on April 28, 2024

I didn't skip anything. What you cited actually makes my case. The important word there is If.

from fosite.

aeneasr avatar aeneasr commented on April 28, 2024

But apart from that, could you specify your use case in which a public client would need access to the token endpoint?

ps: Sorry if this sounds condescending, it's not my intent. Just a bit in a hurry ;)

from fosite.

danilobuerger avatar danilobuerger commented on April 28, 2024

What you cited just means that _IF_ the client is confidential (etc.), _THEN_ you must authenticate it. Meaning that if it's not confidential (i.e. public), this does not apply.

Another hint is here:

https://tools.ietf.org/html/rfc6749#section-9

Native applications that use the authorization code grant type
SHOULD do so without using client credentials, due to the native
application's inability to keep client credentials confidential.

Again meaning that a native app using the authroization code grant should be a public client.

Further, the only grant that explicitly is only for confidential clients is the client grant:

The client credentials grant type MUST only be used by confidential clients.

All other grants allow public clients.

Apart from fosite wanting to be RFC compliant

This library implements peer-reviewed IETF RFC6749

and you challenging people to write tests

Everyone writing an RFC conform test that breaks with the current implementation, will receive a place in the Hall of Fame!

my use case is:

Most first-party mobile apps would prefer implementing their login via the Resource Owner Password Credentials grant for the sake of not having to jump to the browser or implement a web view. Since they should be public (https://tools.ietf.org/html/rfc6749#section-2.1), they normally don't have a client secret.

from fosite.

aeneasr avatar aeneasr commented on April 28, 2024

Thank you for the sum up and sorry again for the hasty answers :)

First, here is the reasoning I originally took for this decision:

There are a couple of sections in RFC 6819 which where taken into account when desinging fosite and which are a USP of this library. None of them are mandatory but if used reduce possible attack vectors. Amongst them are:

The authorization server will require the client to authenticate
wherever possible, so the binding of the authorization "code" to a
certain client can be validated in a reliable way (see
Section 5.2.4.4). - Source: https://tools.ietf.org/html/rfc6819#section-4.4.1.1

and especially for mobile devices (which do not have FQDN):

The authorization server should validate the client's redirect URI
against the pre-registered redirect URI, if one exists (see
Section 5.2.3.5). Note: An invalid redirect URI indicates an
invalid client, whereas a valid redirect URI does not necessarily
indicate a valid client. The level of confidence depends on the
client type. For web applications, the level of confidence is
high, since the redirect URI refers to the globally unique network
endpoint of this application, whose fully qualified domain name
(FQDN) is also validated using HTTPS server authentication by the
user agent. In contrast, for native clients, the redirect URI
typically refers to device local resources, e.g., a custom scheme.
So, a malicious client on a particular device can use the valid
redirect URI the legitimate client uses on all other devices. - Source: https://tools.ietf.org/html/rfc6819#section-4.4.1.4

and from the same section:

The authorization server should authenticate the client, if
possible (see Section 5.2.3.4). Note: The authentication takes
place after the end user has authorized the access. - Source: https://tools.ietf.org/html/rfc6819#section-4.4.1.4

Regarding the resource owner password flow, it is actually discouraged and called an anti-pattern, see https://tools.ietf.org/html/rfc6819#section-4.4.3

I know about the limitation of certain client side libraries for mobile devices regarding this flow. I think it sucks that you need to open a browser window to complete the flow. But, IMHO, these libraries mistake OAuth2, a protocol for delegation, for an authentication protocol. The flow is similar posting to a /user/login REST endpoint. There are no scopes, no consent screen, no revokation - the app is basically all-mighty without the user's knowledge. This is ok for highly priviledged apps but also a security risk if wrongly used. It's not what OAuth2 is for, it's an edge case where you say "hm ok we can get away with this" and Fosite aims to be security-first - this includes cutting off some of the shortcuts such as this one.

But... I am open to discussing support of public requests on the token endpoint. But I need to be convinced that security is not impacted as this is the one promise this library makes.

from fosite.

danilobuerger avatar danilobuerger commented on April 28, 2024

All of the citations apply to the authorization code grant only and not the resource owner password credentials grant.

I agree with you that the authorization code grant should only accept confidential clients (although the spec allows public clients). That's what it's optimized for anyway:

The authorization code grant type is used to obtain both access
tokens and refresh tokens and is optimized for confidential clients.

I would argue for having public clients for the implicit (which is already the case), resource owner password credentials, and refresh token grant.

Sure the password grant is an anti-pattern, but it is so with or without having a confidential client.

Fosite isn't an oauth2 server you can just drop-in in 5 minutes. You can create your own with it. And thus it should be up to the implementor what he want's to do and what not. If he wants to enable having public clients, why not. Its the same thing with the hasher: I could implement a ROT13 hasher with fosite. That doesn't make fosite insecure, it just means I, as an implementor, can make it insecure.

You did such a good job on moving away from osin and creating this library, which could be the defacto standard on golang. But i think that will only happen if you support the whole of the RFC and let the implementor decide what features he needs. Fosite itself will still be secure even then. I am probably not the only one who wants / needs this as a feature. It would be a shame if someone needs to fork because of it.

from fosite.

danilobuerger avatar danilobuerger commented on April 28, 2024

And consider the following: If I implement an oauth2 server with fosite now and I want my mobile / client web app to use the password grant, I would just issue it some random secret (which I know is not secret). Now someone could disassemble or MITM my app and get the secret along with the id (instead of just the id). However, the oauth2 server would think the client is a confidential client without it actually being one. Then I can't have server-side security measures explicitly for public clients as the server is not seeing them as such. Or imagine that someday you will have lax rules on redirection uris for confidential clients only. These would then also apply to my fake confidential client.

from fosite.

aeneasr avatar aeneasr commented on April 28, 2024

Sure the password grant is an anti-pattern, but it is so with or without having a confidential client.

That's a good point. I think fosite already has some security features in place to prevent misuse

  1. You must create a client that explicitly has no secret set
  2. The client must be granted grant_types=resource_owner_password_credentials and be granted scopes too. So it's not really possible to create a client that is public and allowed to do anything by default (read: accident). Maybe this needs to be documented somwhere prominent.

However, right now, I believe that the only flow that should work without secret on the token endpoint is the resource owner grant, because:

  1. A refresh token is never granted to a public client as refresh tokens are only issued when using the authorize code flow. A public client is not allowed to perform this flow, can't ever receive a refresh token and thus is never capable of performing that flow.
  2. A public client can't request a token using the client_credentials grant.
  3. A public client can't use the authorize code flow (or do you think otherwise?)

What do you think about this?

from fosite.

aeneasr avatar aeneasr commented on April 28, 2024

@leetal @faxal I would like your take on this

from fosite.

danilobuerger avatar danilobuerger commented on April 28, 2024

I agree with 2. as this is per spec

The client credentials grant type MUST only be used by confidential clients.

I also agree with 3. But that's just a personal opinion as the spec actually allows public clients, but prefers confidential clients. So this is a situation (like the redirection uris) where I think fosite should make the should a must and just allow confidential clients.

However, I do not agree with 1.:

First of, the password grant actually does issue refresh tokens:

https://tools.ietf.org/html/rfc6749#section-4.3.3

If the access token request is valid and authorized, the
authorization server issues an access token and optional refresh
token as described in Section 5.1.

And second:

https://tools.ietf.org/html/rfc6749#section-6

Because refresh tokens are typically long-lasting credentials used to
request additional access tokens, the refresh token is bound to the
client to which it was issued.

So a public client can only use the refresh token grant with refresh tokens it received itself through the password grant (or refresh token grant). It can't use valid refresh tokens from other clients.

from fosite.

aeneasr avatar aeneasr commented on April 28, 2024

You're right, issuing refresh tokens on that grant type is not only allowed, it's smart too. Otherwise you would have to

  1. store the user credentials on the device to re-authenticate, otherwise log in every hour
  2. create long living access tokens
  3. revoke access of a client

Do you want to take a shot at this? If yes, I'd define for both our understanding what we concluded. Thanks for your input and persistence, it's really appreciated.

from fosite.

aeneasr avatar aeneasr commented on April 28, 2024

The idea would be to:

  1. Replace the credentials check in the access request creator with a simple id check (take into account the recommendations with the redirect_url, if existent)
  2. Add the credentials check in the authorize code token endpoint handler
  3. Give the resource owner credentials grant the power to issue refresh tokens, when the offline scope is granted to both the request and the client
  4. Enforce audience matching in the refresh token handler (if not already present)

Did I miss something?

from fosite.

danilobuerger avatar danilobuerger commented on April 28, 2024

1-2) I wouldn't move it from the access request creator. If the client is confidential we should still check its secret:

https://tools.ietf.org/html/rfc6749#section-4.3.2

If the client type is confidential or the client was issued client
credentials (or assigned other authentication requirements), the
client MUST authenticate with the authorization server as described
in Section 3.2.1.

We could establish the convention that if Client.GetHashedSecret() returns nil it's a public client or add a new method like IsPublic() bool to Client or add a new interface PublicClient which we could cast check.

  1. should probably be done in a separate PR. However, I wonder what a scope has to do with it? Is that something implementation specific to fosite or in the spec? In any case, giving out refresh option should also be controllable on a per client level:

https://tools.ietf.org/html/rfc6749#section-10.4

Authorization servers MAY issue refresh tokens to web application
clients and native application clients.

  1. seems to be already in place:

https://github.com/ory-am/fosite/blob/master/handler/oauth2/flow_refresh.go#L56

    // The authorization server MUST ... and ensure that the refresh token was issued to the authenticated client
    if accessRequest.GetClient().GetID() != request.GetClient().GetID() {
        return errors.Wrap(fosite.ErrInvalidRequest, "Client ID mismatch")
    }

from fosite.

aeneasr avatar aeneasr commented on April 28, 2024

1-2) I wouldn't move it from the access request creator. If the client is confidential we should still check its secret:

Right, it should be:

  • The access requestor validates requests according to the authentication method used which allows public (id lookup) and confidential (credentials).
  • client_credentials and authorize_code grant handlers must perform only confidential authentication and they need to do it themselves.

We could establish the convention that if Client.GetHashedSecret() returns nil it's a public client or add a new method like IsPublic() bool to Client or add a new interface PublicClient which we could cast check.

+1 for adding IsPublic() bool

  1. should probably be done in a separate PR. However, I wonder what a scope has to do with it? Is that something implementation specific to fosite or in the spec? In any case, giving out refresh option should also be controllable on a per client level:

That is something fosite specific. The offline scope is used amongst the big ones (Google, Microsoft) to request refresh tokens. I think this is very smart as it prohibits accidental issuance of refresh tokens and forces developers to think about long living credentials.

It's ok to do that in a separate PR.

  1. seems to be already in place:

Perfect.

from fosite.

aeneasr avatar aeneasr commented on April 28, 2024
  1. Test for public client in request handler
  2. In enforcing endpoints, check if public or confidential client

from fosite.

aeneasr avatar aeneasr commented on April 28, 2024

Except from patching the test stubs, this is straight forward to implement, assigning easy label

from fosite.

aeneasr avatar aeneasr commented on April 28, 2024

closing by c3ffbb8 and because:

  1. should probably be done in a separate PR. However, I wonder what a scope has to do with it? Is that something implementation specific to fosite or in the spec? In any case, giving out refresh option should also be controllable on a per client level:

This is already possible by not including the offline scope in the client.GetScopes() list.

from fosite.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.