search

zf-fr / zfr-oauth2-server

PHP library for creating an OAuth 2 server (currently proof of concept)
BSD 3-Clause "New" or "Revised" License
36 stars 13 forks source link

[0.7] Refactor of ZfrOAuth2 Server #22

Closed bakura10 closed 7 years ago

bakura10 commented 10 years ago

Hi everyone (ping @Ocramius @macnibblet),

In overall, I'm pretty happy with the way ZfrOAuth2 Server works. It proved to be very easy to use, and architecturally much better than any other PHP solutions. The ZF2 module makes it very easy to use, too.

There are a few parts that I'm not really happy though, and I'd love if someone could do a refactor try, as each time I tried, I lamentably failed.

Implicit Grant

This is the only grant that is missing. It looks a lot like the Authorization Code grant, so it should be pretty easy to do.

Add support for second-level cache

I'd like, ideally, to increase minimum dependency of 0.6 to Doctrine 2.5 (I hope it won't come too late), so that it can has built-in support for second level cache. This would enable to reduce to nearly zero the DB calls made by ZfrOAuth2 (currently, it does one or two DB calls per requests while access token are guaranteed to be read-only entities).

This is pretty easy as it's only modifying the entities. If Doctrine 2.5 is still delayed, I may add this feature for 0.7.

Making the grants not aware of responses

This is the part I'm not happy at all with current architecture.

Currently, the grant interfaces all return HttpResponse objects (https://github.com/zf-fr/zfr-oauth2-server/blob/master/src/ZfrOAuth2/Server/Grant/GrantInterface.php#L52).

This has the following advantages:

I'm not sure how I could transmit this information without creating the response in the grant itself. The implicit grant is even worse as its logic implies to append a hash (#access_token=foo) to the response.

For the token responses (example: https://github.com/zf-fr/zfr-oauth2-server/blob/master/src/ZfrOAuth2/Server/Grant/AuthorizationGrant.php#L167) this is much simpler as it could be simplified to only returns an array of params that could be added to the body by the authorization server.

The two biggest problems to creating the response in the grants are:

Currently I feel the Authorization Server is a bit too complicated. With the new implementation of the revocation spec, I even needed to inject the access token service and refresh token service into the Authorization Server: https://github.com/zf-fr/zfr-oauth2-server/blob/master/src/ZfrOAuth2/Server/AuthorizationServer.php#L80

I'd like to avoid that and possibly splitting the Authorization Server into different classes, or using simpler traits.

Better support for removing old tokens

As said by @Ocramius and @mac_nibblet, this part is bad: https://github.com/zf-fr/zfr-oauth2-server/blob/master/src/ZfrOAuth2/Server/Service/TokenService.php#L174

It needs to be changed so that we can take advantage of native DELETE.

Increase testing coverage

Some parts are heavily tested, however some parts like the Authorization Server (because of its path complexity) lacks a lot of tests. I'd like to reach a much better coverage.

Not hashing client secret

I unfortunately do not use the authorization grant, but in most of the applications I've used that implement this grant, they usually give you your "client_secret" and "client_id". Those are usually shown in your account, and they tell you to keep your client_secret really secret blablabla...

However currently in ZfrOAuth2 we crypt the client_secret: https://github.com/zf-fr/zfr-oauth2-server/blob/master/src/ZfrOAuth2/Server/Service/ClientService.php#L75

I've made this because it was done by @weierophinney on ZFOAuth2 but I can't understand that. If we hash the client_secret... how can we show it to our users in their admin panel? It does not make a lot of sense to me, I'd be happy if someone using this grant could give me some hints.

EDIT: after some talk with @ezimuel (see below), it appears that the idea is to have a mechanism similar to AWS account: you can create a client, and the GUI shows you the secret only once. If you forget it... you are force to regenerate the secrets. This allows to protect the database in case it is attacked, so that even if an attacker can read the data, it cannot do API calls with the client secret.

Enrico suggested to have an option to disabling this automatic hashing. I like the idea.

Security audit

There was not any security audit yet. I'd love someone to do one.

Path to 1.0

I think 1.0 won't come before ZF 3.0. I'd like ZfrOAuth2 to be based on ZF 3.0 components (both event manager and any modification to the Response/Request interfaces).

weierophinney commented 10 years ago

Actually, it was @ezimuel who added hashing to the client_secret. I've pinged him so he can comment on the rationale.

bakura10 commented 10 years ago

Thanks. I've tried to have a look at the bshaffer OAuth2 library, but couldn't find the exact place where this is hashed, so maybe this has been removed...

macnibblet commented 10 years ago

@bakura10 i would like to see the integration of the evm as well if this has not already been done. I know i whined about it a while ago.

bakura10 commented 10 years ago

Integration of EVM is already done since 0.4 iirc. I've used it for a project to alter the response and it works pretty well!

bakura10 commented 10 years ago

It's available since 0.3, sorry.

EDIT: the doc is here: https://github.com/zf-fr/zfr-oauth2-server#event-manager

ezimuel commented 10 years ago

@bakura10 Regarding the hashing of the client_secret I added it to protect the storage part. Regarding the dashboard you can offer the possibility to regenerate a new one, showing the secret only on that time, after the encryption, using the same approach of AWS when you create new client credentials. From a security point of view I think this solution is better than the one you proposed. The client_secret and the user's password are protected using bcrypt in Apigility, this encryption mode is not present in oauth2-server-php, I proposed the PR#282 for that long time ago.

bakura10 commented 10 years ago

@ezimuel , I had AWS in mind, actually, where they do exactly that (they suggest you to save your client secret when you create a client and... if you forget it, too bad for you). That's actually how it works currently natively in ZfrOAuth2.

I think you're right from a security point of view. It's just that excepting AWS, I've never seen a website that uses this policy, so maybe it's a bit overkill and complicated from a user point of view.

But for now I'll stick with that then (I really hope you will be able to switch to ZfrOAuth2 one day for Apigility :p).

ezimuel commented 10 years ago

@bakura10 Yes, I would love to use your implementation. I really like the code. Actually, I would have more time to contribute to the project. Regarding the security restriction that's true, maybe we can offer an option, to hash or not the client_secret (enabled by default).

bakura10 commented 10 years ago

Thanks!

I think it would be actually pretty easy for Apigility to switch without breaking everything (furthermore I'm using ZF Response and Request!). Apigility really helped ZfrCors to have contributions and bug fixes, I would love if ZfrOAuth2 could gain the same visibility thanks to Apigility.

Regarding the client, the idea of automatically hashing is very nice!! I'm going to update this issue.

ojhaujjwal commented 10 years ago

:+1:

bakura10 commented 10 years ago

I already tried the "making the grants not aware of responses" (this is the part I'm the least satisfied), but this is damn hard :'(.

basz commented 7 years ago

closing as obsolete, resolved or documented elsewhere