-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat(auth): Optionally skip authentication for image tagging #3795
feat(auth): Optionally skip authentication for image tagging #3795
Conversation
261d38f
to
56a0a67
Compare
Not a huge fan of doing it in this spot ... I'd probably lean towards something like the Maybe It strikes me as heavy handed to pull this into the |
Adds a new feature flag operations.security.allowUnauthenticatedImageTaggingInAccounts that takes a list of account names, and will skip the authentication check for tagging cloud images in AWS or GCE if the account name matches. The motivation is that if the bakery puts images in a protected account, any tagging stage will fail if the user or service account that triggered the pipeline does not have permissions to write to the account that contains the image, even if the same user just created the image. The feature defaults to disabled to not change the current behaviour.
Also make the feature more granular by taking a list of accounts instead of just a boolean.
56a0a67
to
83e0fbc
Compare
@ajordens Thank you for the review :) I was vacationing last week, so sorry for the slow feedback. I've updated the PR now, would you mind taking another look? I skipped the converter-suggestion, but I can move the functionality there if you like that better. |
@@ -68,4 +100,22 @@ class DescriptionAuthorizerSpec extends Specification { | |||
this.names = names | |||
} | |||
} | |||
|
|||
class TestImageTaggingDescription implements AccountNameable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't access the actual implementations because clouddriver-core
doesn't depend on the aws or google modules. Do you prefer this test class implementation, or should I depend on the aws and google modules using gradle testImplementation
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer this rather than adding the dependency, thanks!
Any update on this? 🙂 |
@spinnaker/reviewers PTAL :) |
@@ -68,4 +100,22 @@ class DescriptionAuthorizerSpec extends Specification { | |||
this.names = names | |||
} | |||
} | |||
|
|||
class TestImageTaggingDescription implements AccountNameable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer this rather than adding the dependency, thanks!
Adds a new feature flag
operations.security.allowUnauthenticatedImageTaggingInAccounts
that takes a list of account names, and will skip the authentication check for tagging cloud images in AWS or GCE if the account name matches. The motivation is that if the bakery puts images in a protected account, any tagging stage will fail if the user or service account that triggered the pipeline does not have permissions to write to the account that contains the image, even if the same user just created the image. The feature defaults to disabled to not change the current behaviour.