feat: add client API key authentication #8522
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
| return nil, false | ||
| } | ||
|
|
||
| _, token, err := m.UserProvider.GetUserByClaims(r.Context(), "userid", clientAPIKeyRecord.UserId) |
There was a problem hiding this comment.
Hm, I'd rather prefer not use the machine authprovider to generate the reva token here. Ideally we would have a separate auth manager in reva to handle the api keys.
IMO the machine authprovider is a crutch and we should get away from using it.
BTW there is already the appauth manager in reva (https://github.com/cs3org/reva/blob/edge/pkg/auth/manager/appauth/appauth.go) which seems to serve a similar purpose. Maybe we can use that?
There was a problem hiding this comment.
BTW there is already the
appauthmanager in reva (https://github.com/cs3org/reva/blob/edge/pkg/auth/manager/appauth/appauth.go) which seems to serve a similar purpose. Maybe we can use that?
there is currently only one implementation which stores the password in a json file - https://github.com/owncloud/ocis/blob/afc6ed1e416c8c1abef832df3e6912b7487cccbd/vendor/github.com/cs3org/reva/v2/pkg/appauth/manager/json/json.go
And ... oh lord ... so many layers 🤯 .... what happened to the good old KISS approach ;-)
There was a problem hiding this comment.
Unless I miss anything: in case the final implementation of this feature lives within reva it requires a grpc call which somehow violates the dos mitigation requirement as described in the ADR #8515 ......
DeepDiver1975
force-pushed
the
feat/middleware-client-api-keys
branch
from
50cd72e
to
fb0484f
Compare
|
@DeepDiver1975 @rhafer @micbar implementing the CS3 auth api with our auth services would require adding another service:
I think we should implement the client api key feature as a middleware like in this PR. In a subsequent PR we should align the auth services. Either move all behind the CS3 gateway, or use middlewares directly in the proxy. Then we can omit starting dedicated auth services until we open up the CS3 for external requests. |
|
@DeepDiver1975 you need to add some explaining text to the proxy readme.md |
DeepDiver1975
force-pushed
the
feat/middleware-client-api-keys
branch
from
fb0484f
to
9760ec6
Compare
see 9760ec6 |
|
DeepDiver1975
force-pushed
the
feat/middleware-client-api-keys
branch
from
9760ec6
to
6f46a99
Compare
DeepDiver1975
force-pushed
the
feat/middleware-client-api-keys
branch
from
6f46a99
to
a96dc54
Compare
|
|
@DeepDiver1975 @butonic @rhafer We decided to keep the scope clean. This PR is needed for migration purposes, where a super-powered admin user needs to impersonate all users with unlimited scope for migration purposes. This should
Other use cases like personal app tokens are tracked in another context. #7711 (comment) |
This PR was intended to implement the ADR-30 and to be used for e.g. #8357 and the migration service as started in #8463 We have to see how we move on with the ADR and the existing app auth manager in reva. This all collides somehow. |
Description
implements #8515
Related Issue
Motivation and Context
SSO is great until it sucks ....
Types of changes
Checklist: