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

Reduce privileges for application managers #1090

Closed
wants to merge 1 commit into from

Conversation

zaro0508
Copy link
Contributor

@zaro0508 zaro0508 commented Feb 7, 2024

I think the application manager role is a bit over privileged. They should probably not have unilateral access to the secrets manger in the entire AWS account.

I think the application manager role is a bit over privileged.
They should probably not have unilateral access to the
secrets manger in the entire AWS account.
@zaro0508 zaro0508 requested a review from a team as a code owner February 7, 2024 16:43
Copy link
Contributor

@brucehoff brucehoff left a comment

Choose a reason for hiding this comment

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

This change looks like it would remove the ability for application managers to manage the secrets for their applications, doesn't it?

@zaro0508
Copy link
Contributor Author

zaro0508 commented Feb 8, 2024

This change looks like it would remove the ability for application managers to manage the secrets for their applications, doesn't it?

yes, that is correct @brucehoff. There are lots of app managers now including people who are not app developers. My concern is that it could be deleted/changed by non developers. if you feel that this is fine then we can just close this PR.

@zaro0508 zaro0508 requested a review from brucehoff February 8, 2024 02:04
@brucehoff
Copy link
Contributor

This change looks like it would remove the ability for application managers to manage the secrets for their applications, doesn't it?

yes, that is correct @brucehoff. There are lots of app managers now including people who are not app developers. My concern is that it could be deleted/changed by non developers. if you feel that this is fine then we can just close this PR.

No, I do NOT feel it is fine to give people excessive access. We have to give people the right level of access, meaning that non-developers should not have access to the secret store but neither should we remove access to the secret store from the application mangers who require it.

@zaro0508
Copy link
Contributor Author

zaro0508 commented Feb 8, 2024

No, I do NOT feel it is fine to give people excessive access. We have to give people the right level of access, meaning that non-developers should not have access to the secret store but neither should we remove access to the secret store from the application mangers who require it.

@brucehoff for further context, here's @milen-sage request https://sagebionetworks.slack.com/archives/CEFQD0KU1/p1706290847735899

@zaro0508
Copy link
Contributor Author

zaro0508 commented Feb 15, 2024

No need to do it this way. We accomplished least privilege with an alternative approach. We give Application Manager role to a few developers while we give everyone else access to the Viewer Plus role.

@zaro0508 zaro0508 closed this Feb 15, 2024
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.

2 participants