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

fix(permissions): API permissions model using client token #599

Merged
merged 11 commits into from Aug 9, 2021

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Jul 23, 2021

Fixes #554
Fixes #575

@andrewazores andrewazores added feat New feature or request fix labels Jul 23, 2021
@andrewazores andrewazores force-pushed the api-permissions branch 5 times, most recently from 0418ce7 to f06fb0b Compare July 27, 2021 14:50
@andrewazores andrewazores requested a review from ebaron July 27, 2021 14:58
@andrewazores andrewazores marked this pull request as ready for review July 27, 2021 14:58
@andrewazores andrewazores force-pushed the api-permissions branch 4 times, most recently from d6e7eff to 13f35ba Compare July 28, 2021 16:23
@andrewazores andrewazores marked this pull request as draft July 28, 2021 20:00
@andrewazores andrewazores marked this pull request as ready for review August 3, 2021 15:57
@andrewazores
Copy link
Member Author

@ebaron ready for review

pom.xml Outdated Show resolved Hide resolved
@andrewazores andrewazores force-pushed the api-permissions branch 2 times, most recently from bf2f081 to dc5b7e7 Compare August 4, 2021 20:30
Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I still have some reservations about tying the permissions model to the operator's Kubernetes client. For example, create flightrecorders isn't currently a valid action for a user to take. This also ends up creating a hard dependency on the operator. I think we can revisit and fine-tune the permissions later on though.

src/main/java/io/cryostat/net/OpenShiftAuthManager.java Outdated Show resolved Hide resolved
@andrewazores
Copy link
Member Author

I've added a tokenreviews query to check the user's supplied token, using the service account token to perform the check, if there are no specific Kubernetes resource permissions required for a given authenticated API request. This means the operator clusterrole (or whatever serviceaccount Cryostat runs as) needs create on tokenreviews in group authentication.k8s.io.

Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I tested it locally with the corresponding changes in the operator. I attempted to log in with a fake token to see what would happen and there was no visual indicator in the UI, but there was a 401 Unauthorized in the log. I'm not sure if this differs at all from the current behaviour.

The operator changes are pretty much done, just need to add some more tests. I think these PRs should be merged at the same time to prevent breakage.

@andrewazores
Copy link
Member Author

I attempted to log in with a fake token to see what would happen and there was no visual indicator in the UI, but there was a 401 Unauthorized in the log. I'm not sure if this differs at all from the current behaviour.

I believe that's normal, although now that you mention it, there probably should be some indication that the request was made and rejected.

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

Successfully merging this pull request may close these issues.

Add kubernetes-server-mock test dependency Use OAuth token for Kubernetes client
2 participants