Skip to content

Commit

Permalink
squash later: address review comments, add roles modifications
Browse files Browse the repository at this point in the history
  • Loading branch information
stlaz committed Oct 27, 2020
1 parent 92df640 commit 1d87885
Showing 1 changed file with 42 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,10 @@ The user is determined based on the context of the request, as supplied by the g
API server logic.

Upon receiving a REST request to the new endpoint, the API server internally
wires get/list/delete requests to the `oauthaccesstoken` API endpoint.
wires the get/list/watch/delete requests to the `oauthaccesstoken` API endpoint.
It then converts the results and returns them as `useroauthaccesstoken` type.

To list the tokens internally, a field selector of the form `userName=<userName>`
To list/watch the tokens internally, a field selector of the form `userName=<userName>`
is used. The `userName` field selector must not be overridden by any
user-specified selector.

Expand All @@ -94,6 +95,29 @@ and the `userName` retrieved as described above. If such a match does not occur,
the API server must return "404 - Not Found" so that it is not possible to guess
other people's token object names.

#### Roles modifications

The `basic-user` clusterrole should now additionally receive the following rule:
```
apiGroups:
- oauth.openshift.io
resources:
- useroauthaccesstokens
verbs:
- get
- list
- watch
- delete
```

Since the new API allows to remove user-owned access tokens, it should no longer
be necessary to keep the `system:oauth-token-deleter` clusterrole bound to the
`system:authenticated` group. On the contrary, this opens a small security hole
where a user might be able to delete a token of a different user (although, due to
the default token name length, it is quite improbable). The clusterrolebinding
`system:oauth-token-deleters` will therefore get deprecated as described in
[Graduation Criteria - Removing a Deprecated Feature](#graduation-criteria---removing-a-deprecated-feature).

### Risks and Mitigations

The proposal is risky as it allows (limited) access to a resource that was
Expand All @@ -104,27 +128,37 @@ The level of confidentiality of the `OAuthAccessToken` objects was lowered in
https://github.com/openshift/enhancements/pull/323 and therefore, should a
token leak, it is no longer possible to use the name of such an object to
log in as a different user. On the other hand, the token still contains
information about the user and which services they might be accessing.

Since another aspect of security is to always allow access to legitimate users
when they request it. Unauthorized removal of access tokens would prevent that.
information about the user and which services they might be accessing, and
with in the current RBAC rules, it would also allow a malicious user to
remove the token of the victim.

The above risks should be considered during implementation reviews and when
setting up testing for this feature.

## Design Details

### Graduation Criteria - Removing a Deprecated Feature

Versions: 4.n is the version where the feature gets released, 4.n+1 is the major version succeeding 4.n

In 4.n, the release notes must mention the deprecation of the `system:oauth-token-deleters`
clusterrolebinding and its removal in 4.n+1. This is going to happen in favour of having to
specifically bind the `system:oauth-token-deleter` clusterrole to the subject that needs to
be able to remove oauthaccesstokens directly. Any other self-management of tokens (even
when impersonated) should happen by using the new `useroauthaccesstoken` API.

### Test Plan

New e2e tests:
1. using the new endpoint, a user can list all of the tokens that were issued to them
2. a user cannot get/list/delete tokens for any other user
1. using the new endpoint, a user can get/list/watch/delete all of the tokens that were issued to them
2. a user cannot get/list/watch/delete tokens for any other user

#### Examples

Without changes to the CLI code, the following will be possible:
```
$ oc get useroauthaccesstokens
$ oc get -w useroauthaccesstokens
$ oc get useroauthaccesstokens <token-name> [output options]
$ oc delete useroauthaccesstokens <user-owned-token-name>
```
Expand Down

0 comments on commit 1d87885

Please sign in to comment.