-
Notifications
You must be signed in to change notification settings - Fork 473
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
Enable Authenticating with Azure with Certificates using Azure SDK for Go's Generic NewDefaultAzureCredential #1694
Open
bryan-cox
wants to merge
6
commits into
openshift:master
Choose a base branch
from
bryan-cox:HOSTEDCP-1994
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
ed18676
Initial enhancement
bryan-cox facc698
Add note about SNI
bryan-cox a5ef2d8
Commit Ben's suggestion
bryan-cox bef1d14
Add specific env passed to storage and network operators
bryan-cox 667e461
Removed some variables being passed to the CSO
bryan-cox c1e875c
Update to include more specifics about Secret Store CSI driver
bryan-cox File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,160 @@ | ||
--- | ||
title: msi-enablement-for-aro-hcp | ||
authors: | ||
- "@bryan-cox" | ||
reviewers: # Include a comment about what domain expertise a reviewer is expected to bring and what area of the enhancement you expect them to focus on. For example: - "@networkguru, for networking aspects, please look at IP bootstrapping aspect" | ||
- "@enxebre" #HCP | ||
- "@csrwng" #HCP | ||
- "@kyrtapz" #CNCC | ||
- "@flavianmissi" #Image Registry | ||
- "@jsafrane" #Storage | ||
- "@Miciah" #Ingress | ||
- "@bennerv" #ARO HCP | ||
approvers: # A single approver is preferred, the role of the approver is to raise important questions, help ensure the enhancement receives reviews from all applicable areas/SMEs, and determine when consensus is achieved such that the EP can move forward to implementation. Having multiple approvers makes it difficult to determine who is responsible for the actual approval. | ||
- "@enxebre" | ||
api-approvers: # In case of new or modified APIs or API extensions (CRDs, aggregated apiservers, webhooks, finalizers). If there is no API change, use "None" | ||
- "None" | ||
creation-date: | ||
- "2024-10-07" | ||
last-updated: | ||
- "2024-10-07" | ||
tracking-link: # link to the tracking ticket (for example: Jira Feature or Epic ticket) that corresponds to this enhancement | ||
- "https://issues.redhat.com/browse/HOSTEDCP-1994" | ||
see-also: | ||
- "ARO HCP Authentication Strategy at HCP and Data Plane - https://docs.google.com/document/d/1Z7N2LAnRlgSgrFjjl2absOnkGFsI2TMcbwaW_CA1qek/edit#heading=h.bupciudrwmna" | ||
- "MSI Connector Design - https://docs.google.com/document/d/1xFJSXi71bl-fpAJBr2MM1iFdUqeQnlcneAjlH8ogQxQ/edit#heading=h.8e4x3inip35u" | ||
replaces: | ||
- "https://github.com/openshift/enhancements/pull/1659" | ||
superseded-by: | ||
--- | ||
|
||
# Enable Authenticating with Azure with Certificates using Azure SDK for Go's Generic NewDefaultAzureCredential | ||
|
||
## Summary | ||
|
||
This enhancement proposes enabling image registry, ingress, cloud network config, and storage operators(azure-file and | ||
azure-disk) to accept authenticating with Azure with certificates using Azure SDK for Go's generic function | ||
[NewDefaultAzureCredential](https://github.com/Azure/azure-sdk-for-go/blob/4ebe2fa68c8f9f0a0737d4569810525b4ac45834/sdk/azidentity/default_azure_credential.go#L63). | ||
|
||
## Motivation | ||
|
||
In production, Azure Red Hat OpenShift (ARO) Hosted Control Plane (HCP), operators running in the control plane need to | ||
authenticate using Azure managed identities, backed by certificates, to communicate with cloud services. In the | ||
meantime, ARO HCP will use Service Principal, backed by certificates, for development and testing. | ||
|
||
In contrast, the same operators running on the data plane/guest cluster side use workload identity authentication. | ||
|
||
### User Stories | ||
|
||
* [Explore enable getting AzureCreds via cert using generic NewDefaultAzureCredential](https://issues.redhat.com/browse/HOSTEDCP-1994) | ||
|
||
### Goals | ||
|
||
* Agreement from ingress, image registry, network, and storage representatives on a standard approach to authenticate with Azure for ARO HCP. | ||
|
||
### Non-Goals | ||
|
||
N/A | ||
|
||
## Proposal | ||
|
||
We propose updating the Azure API authentication methods in image registry, ingress, cloud network config, and storage | ||
operators to use the using Azure SDK for Go's generic function [NewDefaultAzureCredential](https://github.com/Azure/azure-sdk-for-go/blob/4ebe2fa68c8f9f0a0737d4569810525b4ac45834/sdk/azidentity/default_azure_credential.go#L63). | ||
This function walks through a chain of Azure authentication types, using environment variables, Instance Metadata Service (IMDS), or a file on the local filesystem to authenticate with the Azure API. | ||
|
||
HyperShift would pass the following environment variables - AZURE_CLIENT_ID, AZURE_TENANT_ID, and | ||
AZURE_CLIENT_CERTIFICATE_PATH - to its deployments of image registry, ingress, cloud network config, and storage | ||
operators (azure-file and azure-disk) on the hosted control plane. Each of these components would then pass these | ||
variables along to NewDefaultAzureCredential. | ||
|
||
For each component, we should also set this environment variable to true, AZURE_CLIENT_SEND_CERTIFICATE_CHAIN. This | ||
enables a Microsoft internal feature called SNI (Subject Name and Issuer authentication). It essentially allows one to | ||
authenticate without pinning a certificate to a service principal if the certificate passed is issued & trusted by a | ||
specific CA. | ||
|
||
Unfortunately, the SDK today does not support hot-reloading the credential. While Microsoft works on getting that into | ||
the SDK, we could either use a library to create a generator for the credential and allow reloading in-process, or use | ||
the common OpenShift fsnotify +os.Exit() to restart the pod on config change. | ||
|
||
### Workflow Description | ||
|
||
* HostedCluster control plane operator will set AZURE_CLIENT_ID, AZURE_TENANT_ID, and AZURE_CLIENT_CERTIFICATE_PATH on deployment of image registry, ingress, cluster network operator (which will pass the value to cloud network config), and storage operators (which will pass the values to azure-file and azure disk) | ||
* When each operator is configuring the Azure authentication type, it will call Azure SDK for Go's generic function NewDefaultAzureCredential | ||
|
||
### API Extensions | ||
|
||
N/A | ||
|
||
### Topology Considerations | ||
|
||
#### Hypershift / Hosted Control Planes | ||
|
||
Specified above | ||
|
||
#### Standalone Clusters | ||
|
||
N/A | ||
|
||
#### Single-node Deployments or MicroShift | ||
|
||
N/A | ||
|
||
### Implementation Details/Notes/Constraints | ||
|
||
TBD | ||
|
||
### Risks and Mitigations | ||
|
||
TBD | ||
|
||
### Drawbacks | ||
|
||
TBD | ||
|
||
## Open Questions [optional] | ||
|
||
TBD | ||
|
||
## Test Plan | ||
|
||
TBD | ||
|
||
## Graduation Criteria | ||
|
||
TBD | ||
|
||
### Dev Preview -> Tech Preview | ||
|
||
TBD | ||
|
||
### Tech Preview -> GA | ||
|
||
TBD | ||
|
||
### Removing a deprecated feature | ||
|
||
N/A | ||
|
||
## Upgrade / Downgrade Strategy | ||
|
||
N/A | ||
|
||
## Version Skew Strategy | ||
|
||
N/A | ||
|
||
## Operational Aspects of API Extensions | ||
|
||
N/A | ||
|
||
## Support Procedures | ||
|
||
N/A | ||
|
||
## Alternatives | ||
|
||
N/A | ||
|
||
## Infrastructure Needed [optional] | ||
|
||
N/A |
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.
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.
When configured with workload identity, the image registry operator will configure the image registry in a similar fashion to what's described in this EP (source).
Do we need to consider this when changing the operator code to use these env vars? In other words, do we also want the image registry (operand!) to authenticate using this proposed mechanism?
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.
Yeah probably so. On ARO HCP, on the HCP side image registry will authenticate with the cert method, while the operand would use workload identity.