docs: Azure authentication refactoring design #1940
Conversation
Signed-off-by: Shahram Kalantari <[email protected]>
shahramk64
force-pushed
the
skalantari/refactor-azure-authentication-design-doc
branch
from
d1f0005 to
e8a9fae
Compare
| 2. **Infer authentication type** automatically based on the environment, reducing user configuration overhead. | ||
| 3. **Unify implementations** for workload identity and managed identity in ORAS auth providers. | ||
| 4. Implement a **chained authentication process**: | ||
| - Workload Identity → Managed Identity → Azure CLI. |
There was a problem hiding this comment.
In terms of Azure CLI, does that mean Ratify CLI will also support auth to Azure?
There was a problem hiding this comment.
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.
shahramk64
changed the title
shahramk64
changed the title
|
|
||
| ### **Goals** | ||
| 1. Design a **common package** for Azure authentication logic. | ||
| 2. **Infer authentication type** automatically based on the environment, reducing user configuration overhead. |
There was a problem hiding this comment.
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.
That's a good point. As you suggested, we can provide this ability by accepting the override from user input.
| } | ||
| ``` | ||
| #### **2. Refactor ORAS Auth Providers** | ||
| - Combine `azureidentity.go` and `azureworkloadidentity.go` into a single file. |
There was a problem hiding this comment.
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.
I think we can ensure this by providing the override ability, as you pointed above.
| } | ||
| ``` | ||
| #### **2. Refactor ORAS Auth Providers** | ||
| - Combine `azureidentity.go` and `azureworkloadidentity.go` into a single file. |
There was a problem hiding this comment.
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.
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.
| - Authentication type will be inferred based on environment variables. | ||
|
|
||
| #### **3. Refactor Key Management and Certificate Providers** | ||
| - Update the providers to leverage the new `pkg/common/cloudauthproviders/azure` package for authentication. |
There was a problem hiding this comment.
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.
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)
shahramk64
requested review from
jimmyraywv,
luisdlp,
susanshi and
toddysm
as code owners
|
|
||
| --- | ||
|
|
||
| ### **Proposed Tasks** |
There was a problem hiding this comment.
If we plan to deprecate any existing auth provider/config, we should add this to the V2 tracking issue that @binbin-li made.
| - Consolidated authentication logic minimizes duplication and enhances clarity. | ||
|
|
||
| 2. **Enhanced User Experience**: | ||
| - Automatic detection of authentication type eliminates the need for explicit configuration in most environments. |
There was a problem hiding this comment.
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
Signed-off-by: Shahram Kalantari <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅ |
|
Ready for review, maintainers please take another look @binbin-li , @akashsinghal , @susanshi |
| 3. Ability to provide required parameters: When using the default azure credential option, we can only rely on the environment variables, meaning that we cannot provide the client_id, tenant_id, or any other parameter explicitly. This is particularly problematic when Ratify is provided with multiple auth providers. With the chanined token credentials, each credential type in the chain can be provided with the required parameters explicitly if needed. | ||
|
|
||
| #### **2. Refactor ORAS Auth Providers** | ||
| - Combine `azureidentity.go` and `azureworkloadidentity.go` into a single file. |
There was a problem hiding this comment.
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.
| # **Azure Authentication Refactoring in Ratify** | ||
|
|
||
| ## **Introduction** | ||
| Authentication is a critical process in Ratify, ensuring secure access to artifatcs in container registries, and to keys, secrets and certificates from cloud key vaults, and other resources. Azure offers two primary SDKs for authentication in Go: |
There was a problem hiding this comment.
| Authentication is a critical process in Ratify, ensuring secure access to artifatcs in container registries, and to keys, secrets and certificates from cloud key vaults, and other resources. Azure offers two primary SDKs for authentication in Go: | |
| Authentication is a critical process in Ratify, ensuring secure access to artifacts in container registries, and to keys, secrets and certificates from cloud key vaults, and other resources. Azure offers two primary SDKs for authentication in Go: |
| There is another option that can be used which is the default azure credential: `azidentity.NewDefaultAzureCredential`. It's an opinionated, preconfigured chain of credentials and is designed to support many environments, along with the most common authentication flows and developer tools. In graphical form, the underlying chain looks like this: | ||
|  | ||
|
|
||
| Howecer, this option is not recommended for the following reasons: |
There was a problem hiding this comment.
| Howecer, this option is not recommended for the following reasons: | |
| However, this option is not recommended for the following reasons: |
| "github.com/Azure/azure-sdk-for-go/sdk/azidentity" | ||
| ) | ||
|
|
||
| func NewChainedCredential() (*azidentity.ChainedTokenCredential, error) { |
There was a problem hiding this comment.
nit: I assume this proposed implementation is going to change now to take into account different client ids not specified via env variables?
Description
What this PR does / why we need it:
This PR proposes a design doc for refactoring Azure authentication modules in Ratify. It describes the existing Azure authentication modules and it's pain points and provide a solution to refactor the code to solve those pain points.
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Provide a design doc to #1944
Type of change
Please delete options that are not relevant.
mainbranch)Checklist:
Post Merge Requirements
Helm Chart Change