Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
docs: Azure authentication refactoring design #1940
base: dev
Are you sure you want to change the base?
docs: Azure authentication refactoring design #1940
Changes from 1 commit
e8a9fae
c2e8b78
56195df
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This is a great add for almost all scenarios but there are scenarios where an override from the uesr to specify exactly the cred type might be required. Notation CLI encountered this too. We should consider exposing override capability which will not use the chained credential.
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.
That's a good point. As you suggested, we can provide this ability by accepting the override from user input.
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.
In terms of Azure CLI, does that mean Ratify CLI will also support auth to Azure?
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.
Yes, one of the goals of this work is to support Azure authentication in the CLI scenario and the azidentity SDK seems to be able to facilitate this through a number of credential types like AzureCLICredential, AzureDeveloperCLICredential, DefaultAzureCredential, and ChainedTokenCredential.
ChainedTokenCredential seems to be the right choice for ratify to consolidate all scenarios in one single place.
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.
nit: I assume this proposed implementation is going to change now to take into account different client ids not specified via env variables?
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.
That's correct. This is just a simple example of how it can be done. ClientId can be explicitly passed and used when provided, instead of always expecting it from env variables.
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.
How will maintain backwards compatability and ensure no breaking changes? We'll need to ensure we can support existing workload identity managed identity providers when user specifies.
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 think we can ensure this by providing the override ability, as you pointed above.
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.
Does this mean we will introduce a new auth provider or just refactor existing one and add new fields necessary?
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 think we can combine the existing two Oras auth providers into one auth provider. If chained authentication is used, there is no need to have both. We can override the chained credential process based on the user input, to explicitly use workload or managed identity.
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.
Since we are keeping the existing authProviders, and introducing new azure auth provider. Should we just keep current implementation as is ( reduce risk , and introduce a new implementation/new file for the new auth provider. This is simliar to how we deprecated CertProvider and introduce KMP CR.
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.
+1
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.
Sounds good to me too.
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.
From a user perspective, will there be any change in how the credentials are configured? Is KMP AKV setup with client id etc. decoupled still from ORAS azure auth providers?
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.
Good point. This requires more thinking. There are a few alternatives here:
1- Decoupled scenario: both can provide their own configurations. I wonder how the chained credential should work in this case. For example if both are defining a client id variable, which one is set in the ENV variable that the chained credential uses.
2- Coupled scenario: the configuration that is common to both is extracted and placed into a separate resource to represent both. This means that both will use the same credential type and the same credential config, (unless overridden explicitly?)
I think a decision needs to be made whether to support different types of credentials for Oras and KMP at the same time or not (for example, workload identity for KMP and managed identity for Oras), and also when using the same credential type for both, to support different identities for them (for example, a different client id for Oras and KMP)
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.
Could you please elaborate on which auth configuration will be simplified after refactoring? Will Ratify detect whether users use a Azure Workload Identity or Azure Managed Identity? Maybe we could reference this doc to clarify which configuration could be removed https://ratify.dev/docs/quickstarts/ratify-on-azure#create-a-custom-resource-for-accessing-acr
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.
If we plan to deprecate any existing auth provider/config, we should add this to the V2 tracking issue that @binbin-li made.