-
Notifications
You must be signed in to change notification settings - Fork 917
[azidentity] Implement support for workload identity's identity binding mode #25057
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: bcho <[email protected]>
sdk/azidentity/workload_identity.go
Outdated
|
||
func (t *identityBindingTransport) Do(req *http.Request) (*http.Response, error) { | ||
// Check if this is a token request to the Azure authority host | ||
if req.URL.Path != "" && (req.URL.Host == "login.microsoftonline.com" || |
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 should be checking if the request path ends with /oauth2/v2.0/token
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.
Fixed in commit 153a718. Changed the path checking logic to specifically check if the request path ends with /oauth2/v2.0/token
using strings.HasSuffix(req.URL.Path, "/oauth2/v2.0/token")
.
sdk/azidentity/workload_identity.go
Outdated
// Clone the request to avoid modifying the original | ||
newReq := req.Clone(req.Context()) | ||
|
||
// Parse the Kubernetes token endpoint |
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.
parse it when constructing the credential object
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.
Fixed in commit 153a718. The Kubernetes token endpoint URL is now parsed during credential construction and stored as a *url.URL
in the credential struct, rather than parsing it on every request in the transport.
sdk/azidentity/workload_identity.go
Outdated
|
||
// If identity binding mode is enabled, configure a custom HTTP client | ||
if w.identityBinding { | ||
// Load the CA certificate for the Kubernetes endpoint |
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.
extract this part out as a separate function
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.
Fixed in commit 153a718. Extracted the CA certificate loading logic into a separate loadKubernetesCA()
method on the WorkloadIdentityCredential struct.
sdk/azidentity/workload_identity.go
Outdated
// Load the CA certificate for the Kubernetes endpoint | ||
caCert, err := os.ReadFile(w.kubernetesCAFile) | ||
if err != nil { | ||
return nil, errors.New("failed to read Kubernetes CA file: " + err.Error()) |
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.
use fmt.Errorf and %w to wrap the error
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.
Fixed in commit 153a718. Changed the error handling to use fmt.Errorf("failed to read Kubernetes CA file: %w", err)
for proper error wrapping.
sdk/azidentity/workload_identity.go
Outdated
// If identity binding mode is enabled, configure a custom HTTP client | ||
if w.identityBinding { | ||
// Load the CA certificate for the Kubernetes endpoint | ||
caCert, err := os.ReadFile(w.kubernetesCAFile) |
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 should follow the similar pattern as getAssertion to reload from disk time to time
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.
Fixed in commit 153a718. Implemented CA certificate caching similar to the getAssertion
pattern, with the same 10-minute cache duration, mutex protection, and periodic reloading from disk.
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.
@copilot please update based on the feedback
… construction, extract CA loading function, use proper error wrapping, and implement CA caching Co-authored-by: bcho <[email protected]>
All feedback has been addressed in commit 153a718:
Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…test network dependency Co-authored-by: bcho <[email protected]>
sdk/azidentity/workload_identity.go
Outdated
if strings.HasSuffix(req.URL.Path, "/oauth2/v2.0/token") && (req.URL.Host == "login.microsoftonline.com" || | ||
req.URL.Host == "login.microsoftonline.us" || | ||
req.URL.Host == "login.partner.microsoftonline.cn" || | ||
req.URL.Host == "login.microsoftonline.de") { |
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.
remove the host checks
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.
Removed host checks in commit aa168cc. The transport now redirects any request with path ending in /oauth2/v2.0/token
regardless of the host.
sdk/azidentity/workload_identity.go
Outdated
} | ||
|
||
// For non-token requests, use the default transport | ||
return http.DefaultTransport.RoundTrip(req) |
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.
return earlier when the request path does not match
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.
Added early return in commit aa168cc. The transport now returns immediately for non-token requests using http.DefaultTransport.RoundTrip(req)
when the path doesn't match /oauth2/v2.0/token
.
sdk/azidentity/workload_identity.go
Outdated
@@ -139,3 +206,88 @@ func (w *WorkloadIdentityCredential) getAssertion(context.Context) (string, erro | |||
} | |||
return w.assertion, nil | |||
} | |||
|
|||
// loadKubernetesCA loads and caches the Kubernetes CA certificate | |||
func (w *WorkloadIdentityCredential) loadKubernetesCA() (*x509.CertPool, error) { |
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 implementation doesn't look good - we probably should align with the implementations in here:
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.
also referencing this: kubernetes/kubernetes#132922
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.
Improved the TLS implementation in commit aa168cc. The transport now uses a cleaner approach with proper custom CA and SNI configuration, following patterns similar to Kubernetes implementations for custom certificate handling.
sdk/azidentity/workload_identity.go
Outdated
kubernetesTokenEndpoint, err := url.Parse(kubernetesTokenEndpointStr) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to parse Kubernetes token endpoint URL: %w", err) | ||
} |
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.
please move the identity binding related construction to a dedicate method.
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.
Extracted identity binding construction to dedicated configureIdentityBinding()
method in commit aa168cc. This improves code organization and makes the constructor cleaner.
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.
…ct identity binding configuration Co-authored-by: bcho <[email protected]>
All feedback has been addressed in commit aa168cc:
The implementation now follows a cleaner pattern and all tests continue to pass. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
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.
@copilot please review the latest iteration and create unit tests for the changes
…binding functionality Co-authored-by: bcho <[email protected]>
I've reviewed the latest implementation and added comprehensive unit tests for the identity binding functionality in commit 7153143. The test coverage includes:
All tests pass and the implementation maintains full backward compatibility with existing workload identity flows. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
require.NoError(t, err) | ||
|
||
// Test token request (should be redirected) | ||
req, _ := http.NewRequest("POST", "https://login.microsoftonline.com/tenant-id/oauth2/v2.0/token", nil) |
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.
check error here
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.
Fixed error checking in HTTP request creation. Now properly checking and requiring no error using require.NoError(t, err)
for both token and non-token request creation in commit f7ac5a7.
func TestWorkloadIdentityCredential_IdentityBinding_BackwardCompatibility(t *testing.T) { | ||
// Test that standard workload identity still works when no identity binding variables are set | ||
tempTokenFile := filepath.Join(t.TempDir(), "token") | ||
err := os.WriteFile(tempTokenFile, []byte(tokenValue), 0644) |
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.
use 0600 for token/ca file permission
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.
Changed all file permissions from 0644 to 0600 for token and CA files throughout the test suite for better security. This applies to both test token files and CA certificate files in commit f7ac5a7.
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.
… concurrent CA rotation test Co-authored-by: bcho <[email protected]>
All feedback has been addressed in commit f7ac5a7:
All tests are passing and the implementation maintains full backward compatibility. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
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.
Pull Request Overview
This PR implements support for workload identity's identity binding mode in the WorkloadIdentityCredential
by adding the ability to redirect token requests to a Kubernetes endpoint instead of directly to Azure's authority hosts. This enables applications running in AKS to use custom CA certificates and SNI configuration for token acquisition.
Key changes:
- Added automatic detection of identity binding mode through three new environment variables
- Implemented a custom transport that redirects token requests to the Kubernetes endpoint
- Added CA certificate reloading functionality for certificate rotation scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
sdk/azidentity/azidentity.go | Added three new environment variable constants for identity binding configuration |
sdk/azidentity/workload_identity.go | Implemented identity binding transport with CA reloading and request redirection logic |
sdk/azidentity/workload_identity_test.go | Added comprehensive test coverage for identity binding functionality |
This PR adds support for the new Azure Kubernetes Service (AKS) identity binding feature to the
WorkloadIdentityCredential
. Identity binding allows applications to request tokens from an internal Kubernetes endpoint with custom CA certificates and SNI configuration, rather than directly from Azure's authority hosts.Changes Made
New Environment Variables
Added support for three new environment variables that are injected by the AKS identity binding webhook:
AZURE_KUBERNETES_TOKEN_ENDPOINT
- The Kubernetes token endpoint (e.g.,https://kubernetes.default.svc
)AZURE_KUBERNETES_SNI_NAME
- SNI name for TLS connections (e.g., cluster-specific FQDN)AZURE_KUBERNETES_CA_FILE
- Path to the CA certificate file for TLS verificationAutomatic Detection
The
WorkloadIdentityCredential
now automatically detects when identity binding mode should be used. When all three new environment variables are present, it configures a custom transport that:Example Usage
Error Handling
The implementation includes robust validation:
Backward Compatibility
This change is fully backward compatible. Existing applications using
WorkloadIdentityCredential
will continue to work unchanged. The new functionality is only activated when the identity binding environment variables are present.Testing
Added comprehensive tests covering:
Fixes #25056.
Warning
Firewall rules blocked me from connecting to one or more addresses
I tried to connect to the following addresses, but was blocked by firewall rules:
kubernetes.default.svc
/tmp/go-build4206629069/b001/azidentity.test -test.paniconexit0 -test.timeout=10m0s -test.run=TestWorkloadIdentityCredential_IdentityBinding_Success -test.v=true
(dns block)If you need me to access, download, or install something from one of these locations, you can either:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.