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

Only allow access request deletion through static roles' permissions #9540

Merged
merged 2 commits into from
Jan 3, 2022

Conversation

espadolini
Copy link
Contributor

Access request deletion is not meant to be used on a regular basis, and in fact it's not an operation that tsh supports; it's however supported via gRPC and the web interface. Letting a temporarily privileged user delete access requests can lead to reduced visibility over the action of the user, and can cause confusion if the cert authority needs to reference that access request to issue more certificates for other purposes.

This partially mitigates TEL-Q321-8.

@espadolini
Copy link
Contributor Author

@russjones after this change the web UI will still allow the user to click the "delete" button on the access request if an elevated role would allow it, displaying an error message after confirmation.

The code that ultimately decides what operations are shown as available to the user (lib/web/apiserver.go:(*Handler.getUserContext)) is currently checking the roles stored in the user certificate, and I don't think it would be a good idea for it to fetch the static roles from the backend to do a more accurate check every time.

@espadolini espadolini force-pushed the espadolini/access-request-deletion branch from 8e1ce99 to 55d4347 Compare December 22, 2021 15:48
@espadolini espadolini removed the request for review from smallinsky December 22, 2021 15:56
@russjones
Copy link
Contributor

@espadolini Please add test coverage to this PR.

@espadolini espadolini force-pushed the espadolini/access-request-deletion branch 2 times, most recently from e92771a to 3a254df Compare December 30, 2021 10:19
@espadolini espadolini force-pushed the espadolini/access-request-deletion branch from 3a254df to d18f9d1 Compare December 31, 2021 10:36
@espadolini espadolini force-pushed the espadolini/access-request-deletion branch from d18f9d1 to 877b815 Compare January 3, 2022 14:28
@espadolini espadolini enabled auto-merge (squash) January 3, 2022 14:32
@espadolini espadolini force-pushed the espadolini/access-request-deletion branch from 877b815 to d8a482c Compare January 3, 2022 16:14
@espadolini espadolini merged commit 8db6aa5 into master Jan 3, 2022
@espadolini espadolini deleted the espadolini/access-request-deletion branch January 3, 2022 18:43
@espadolini
Copy link
Contributor Author

@russjones which backports, if any?

espadolini added a commit that referenced this pull request Mar 17, 2022
espadolini added a commit that referenced this pull request Mar 17, 2022
espadolini added a commit that referenced this pull request Mar 17, 2022
espadolini added a commit that referenced this pull request Mar 17, 2022
@webvictim webvictim mentioned this pull request Apr 19, 2022
@webvictim webvictim mentioned this pull request Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants