From 137fc0039dbe905bf5b04da98707fd6a225b1cd2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Jul 2026 21:03:08 +0000 Subject: [PATCH 1/8] Initial plan From 772189991721bd211199ec82c6ff7fd1b38aaf9f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Jul 2026 21:11:57 +0000 Subject: [PATCH 2/8] Support service principal certificate files --- docs/usages/configuration.md | 5 +- docs/usages/joining-nodes.md | 2 +- pkg/aksmachine/client_armapi.go | 13 ++++ pkg/aksmachine/client_armapi_test.go | 26 +++++++ pkg/config/adapter.go | 17 +++-- pkg/config/adapter_test.go | 31 ++++++++ pkg/config/config.go | 65 ++++++++++++++-- pkg/config/config_test.go | 107 ++++++++++++++++++++++++++- pkg/daemon/install.go | 54 ++++++++++++++ pkg/daemon/install_test.go | 49 ++++++++++++ pkg/daemon/start.go | 8 ++ 11 files changed, 359 insertions(+), 18 deletions(-) create mode 100644 pkg/daemon/install_test.go diff --git a/docs/usages/configuration.md b/docs/usages/configuration.md index a34143f2..1ce89602 100644 --- a/docs/usages/configuration.md +++ b/docs/usages/configuration.md @@ -81,8 +81,9 @@ At least one join or Azure authentication method must be configured. `azure.boot |------|------|-------------|--------------| | `azure.servicePrincipal.tenantId` | string | Microsoft Entra tenant ID for the service principal. | `70a036f6-8e4d-4615-bad6-149c02e7720d` | | `azure.servicePrincipal.clientId` | string | Application client ID. | `00000000-0000-0000-0000-000000000000` | -| `azure.servicePrincipal.clientSecret` | string | Application client secret. Mutually exclusive with `clientSecretFile`. | `` | -| `azure.servicePrincipal.clientSecretFile` | string | Path to a protected regular file (no group/other access; e.g., 0600) containing the application client secret. Mutually exclusive with `clientSecret`. | `/run/credentials/aks-flex-node-sp` | +| `azure.servicePrincipal.clientSecret` | string | Application client secret. Mutually exclusive with the credential file options. | `` | +| `azure.servicePrincipal.clientSecretFile` | string | Path to a protected regular file (no group/other access; e.g., 0600) containing the application client secret. Mutually exclusive with the other credential options. | `/run/credentials/aks-flex-node-sp` | +| `azure.servicePrincipal.clientCertificateFile` | string | Path to a protected PEM or unencrypted PFX file containing the application certificate and private key. Mutually exclusive with the other credential options. | `/run/credentials/aks-flex-node-sp.pem` | ## Agent diff --git a/docs/usages/joining-nodes.md b/docs/usages/joining-nodes.md index e41bc1e6..f477e00e 100644 --- a/docs/usages/joining-nodes.md +++ b/docs/usages/joining-nodes.md @@ -107,7 +107,7 @@ Minimal config shape: } ``` -The credential file contains only the client secret. The agent requires this to be a non-empty regular file with no group/world access (for example, mode 0600). Inline `clientSecret` remains supported, but cannot be configured together with `clientSecretFile`. +The secret credential file contains only the client secret. Alternatively, set `clientCertificateFile` to a PEM or unencrypted PFX file containing the application certificate and private key. The agent requires credential files to be non-empty regular files with no group/world access (for example, mode 0600). Only one of `clientSecret`, `clientSecretFile`, or `clientCertificateFile` can be configured. ## Authentication Mode Selection diff --git a/pkg/aksmachine/client_armapi.go b/pkg/aksmachine/client_armapi.go index 31ede750..f4ad0a9a 100644 --- a/pkg/aksmachine/client_armapi.go +++ b/pkg/aksmachine/client_armapi.go @@ -165,6 +165,19 @@ func getCredential(cfg *config.Config, logger *slog.Logger, clientOpts azcore.Cl "tenantID", cfg.Azure.ServicePrincipal.TenantID, "clientID", cfg.Azure.ServicePrincipal.ClientID, ) + if cfg.Azure.ServicePrincipal.ClientCertificateFile != "" { + certificates, privateKey, err := cfg.Azure.ServicePrincipal.LoadClientCertificate() + if err != nil { + return nil, fmt.Errorf("load service principal client certificate: %w", err) + } + return azidentity.NewClientCertificateCredential( + cfg.Azure.ServicePrincipal.TenantID, + cfg.Azure.ServicePrincipal.ClientID, + certificates, + privateKey, + &azidentity.ClientCertificateCredentialOptions{ClientOptions: clientOpts}, + ) + } return azidentity.NewClientSecretCredential( cfg.Azure.ServicePrincipal.TenantID, cfg.Azure.ServicePrincipal.ClientID, diff --git a/pkg/aksmachine/client_armapi_test.go b/pkg/aksmachine/client_armapi_test.go index 67b31297..0e8333f6 100644 --- a/pkg/aksmachine/client_armapi_test.go +++ b/pkg/aksmachine/client_armapi_test.go @@ -1,7 +1,10 @@ package aksmachine import ( + "io" + "log/slog" "math" + "path/filepath" "strings" "testing" @@ -150,6 +153,7 @@ func TestAzureClientOptionsFromConfig(t *testing.T) { if !ok { t.Fatal("ResourceManager cloud service is missing") } + if service.Endpoint != "https://management.example.test" { t.Fatalf("ResourceManager endpoint = %q, want https://management.example.test", service.Endpoint) } @@ -161,6 +165,28 @@ func TestAzureClientOptionsFromConfig(t *testing.T) { } } +func TestGetCredentialClientCertificateLoadError(t *testing.T) { + t.Parallel() + + cfg := testARMConfig(testClusterResourceID, "flex-node-1", "1.34.0") + cfg.Azure.ServicePrincipal = &config.ServicePrincipalConfig{ + TenantID: "tenant", + ClientID: "client", + ClientCertificateFile: filepath.Join(t.TempDir(), "missing"), + } + credential, err := getCredential( + cfg, + slog.New(slog.NewTextHandler(io.Discard, nil)), + azureClientOptionsFromConfig(cfg), + ) + if err == nil || !strings.Contains(err.Error(), "load service principal client certificate") { + t.Fatalf("getCredential() error = %v, want client certificate load error", err) + } + if credential != nil { + t.Fatalf("getCredential() = %T, want nil", credential) + } +} + func TestBuildK8sProfile(t *testing.T) { t.Parallel() diff --git a/pkg/config/adapter.go b/pkg/config/adapter.go index 15fe310b..336bef26 100644 --- a/pkg/config/adapter.go +++ b/pkg/config/adapter.go @@ -66,12 +66,17 @@ func ToAgentConfig(cfg *Config, machineName string) *agentconfig.AgentConfig { ac.Kubelet.Auth.BootstrapToken = cfg.Azure.BootstrapToken.Token case cfg.IsSPConfigured(): - ac.Kubelet.Auth.ExecCredential = buildExecCredential(map[string]string{ - "AAD_LOGIN_METHOD": "spn", - "AAD_SERVICE_PRINCIPAL_CLIENT_ID": cfg.Azure.ServicePrincipal.ClientID, - "AAD_SERVICE_PRINCIPAL_CLIENT_SECRET": cfg.Azure.ServicePrincipal.ClientSecret, - "AZURE_TENANT_ID": cfg.Azure.ServicePrincipal.TenantID, - }) + env := map[string]string{ + "AAD_LOGIN_METHOD": "spn", + "AAD_SERVICE_PRINCIPAL_CLIENT_ID": cfg.Azure.ServicePrincipal.ClientID, + "AZURE_TENANT_ID": cfg.Azure.ServicePrincipal.TenantID, + } + if cfg.Azure.ServicePrincipal.ClientCertificateFile != "" { + env["AAD_SERVICE_PRINCIPAL_CLIENT_CERTIFICATE"] = cfg.Azure.ServicePrincipal.ClientCertificateFile + } else { + env["AAD_SERVICE_PRINCIPAL_CLIENT_SECRET"] = cfg.Azure.ServicePrincipal.ClientSecret + } + ac.Kubelet.Auth.ExecCredential = buildExecCredential(env) case cfg.IsMIConfigured(): env := map[string]string{ diff --git a/pkg/config/adapter_test.go b/pkg/config/adapter_test.go index 5e153c79..020040d0 100644 --- a/pkg/config/adapter_test.go +++ b/pkg/config/adapter_test.go @@ -139,6 +139,7 @@ func TestToAgentConfig_ServicePrincipalClientSecretFile(t *testing.T) { if err := os.WriteFile(clientSecretFile, []byte("file-secret\n"), 0o600); err != nil { t.Fatalf("os.WriteFile: %v", err) } + cfg := &Config{ Azure: AzureConfig{ ServicePrincipal: &ServicePrincipalConfig{ @@ -165,6 +166,36 @@ func TestToAgentConfig_ServicePrincipalClientSecretFile(t *testing.T) { } } +func TestToAgentConfig_ServicePrincipalClientCertificateFile(t *testing.T) { + t.Parallel() + + certificateFile := "/run/credentials/aks-flex-node-sp.pem" + cfg := &Config{ + Azure: AzureConfig{ + ServicePrincipal: &ServicePrincipalConfig{ + TenantID: "tenant-123", + ClientID: "client-456", + ClientCertificateFile: certificateFile, + }, + }, + } + + exec := ToAgentConfig(cfg, "kube1").Kubelet.Auth.ExecCredential + if exec == nil { + t.Fatal("ExecCredential should be set for SP auth") + } + envMap := make(map[string]string) + for _, e := range exec.Env { + envMap[e.Name] = e.Value + } + if envMap["AAD_SERVICE_PRINCIPAL_CLIENT_CERTIFICATE"] != certificateFile { + t.Fatalf("AAD_SERVICE_PRINCIPAL_CLIENT_CERTIFICATE=%q, want %q", envMap["AAD_SERVICE_PRINCIPAL_CLIENT_CERTIFICATE"], certificateFile) + } + if _, ok := envMap["AAD_SERVICE_PRINCIPAL_CLIENT_SECRET"]; ok { + t.Fatal("AAD_SERVICE_PRINCIPAL_CLIENT_SECRET should not be set for certificate auth") + } +} + func TestToAgentConfig_ManagedIdentity(t *testing.T) { t.Parallel() diff --git a/pkg/config/config.go b/pkg/config/config.go index 06b685d2..09fa6a0b 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -1,6 +1,8 @@ package config import ( + "crypto" + "crypto/x509" "encoding/json" "fmt" "net/url" @@ -10,6 +12,8 @@ import ( "strings" "time" + "github.com/Azure/azure-sdk-for-go/sdk/azidentity" + "github.com/Azure/AKSFlexNode/pkg/logger" agentconfig "github.com/Azure/unbounded/pkg/agent/config" "k8s.io/apimachinery/pkg/util/validation" @@ -75,10 +79,11 @@ type AzureConfig struct { // ServicePrincipalConfig holds Azure service principal authentication configuration. // When provided, service principal authentication will be used instead of Azure CLI. type ServicePrincipalConfig struct { - TenantID string `json:"tenantId"` // Azure AD tenant ID - ClientID string `json:"clientId"` // Azure AD application (client) ID - ClientSecret string `json:"clientSecret,omitempty"` // Azure AD application client secret - ClientSecretFile string `json:"clientSecretFile,omitempty"` // File containing the Azure AD application client secret + TenantID string `json:"tenantId"` // Azure AD tenant ID + ClientID string `json:"clientId"` // Azure AD application (client) ID + ClientSecret string `json:"clientSecret,omitempty"` // Azure AD application client secret + ClientSecretFile string `json:"clientSecretFile,omitempty"` // File containing the Azure AD application client secret + ClientCertificateFile string `json:"clientCertificateFile,omitempty"` // File containing an Azure AD application certificate and private key } // ManagedIdentityConfig holds managed identity authentication configuration. @@ -579,11 +584,17 @@ func (c *ServicePrincipalConfig) validate() error { if c.ClientID == "" { return fmt.Errorf("azure.servicePrincipal.clientId is required when service principal is configured") } - if c.ClientSecret != "" && c.ClientSecretFile != "" { - return fmt.Errorf("only one of azure.servicePrincipal.clientSecret or azure.servicePrincipal.clientSecretFile can be configured") + configuredCredentials := 0 + for _, credential := range []string{c.ClientSecret, c.ClientSecretFile, c.ClientCertificateFile} { + if credential != "" { + configuredCredentials++ + } + } + if configuredCredentials > 1 { + return fmt.Errorf("only one of azure.servicePrincipal.clientSecret, azure.servicePrincipal.clientSecretFile, or azure.servicePrincipal.clientCertificateFile can be configured") } - if c.ClientSecret == "" && c.ClientSecretFile == "" { - return fmt.Errorf("azure.servicePrincipal.clientSecret or azure.servicePrincipal.clientSecretFile is required when service principal is configured") + if configuredCredentials == 0 { + return fmt.Errorf("one of azure.servicePrincipal.clientSecret, azure.servicePrincipal.clientSecretFile, or azure.servicePrincipal.clientCertificateFile is required when service principal is configured") } if c.ClientSecretFile != "" { clientSecret, err := loadServicePrincipalClientSecret(c.ClientSecretFile) @@ -593,6 +604,16 @@ func (c *ServicePrincipalConfig) validate() error { c.ClientSecret = clientSecret c.ClientSecretFile = "" } + if c.ClientCertificateFile != "" { + absolutePath, err := filepath.Abs(c.ClientCertificateFile) + if err != nil { + return fmt.Errorf("invalid azure.servicePrincipal.clientCertificateFile: resolve absolute path: %w", err) + } + c.ClientCertificateFile = absolutePath + if _, _, err := c.LoadClientCertificate(); err != nil { + return fmt.Errorf("invalid azure.servicePrincipal.clientCertificateFile: %w", err) + } + } return nil } @@ -625,6 +646,34 @@ func loadServicePrincipalClientSecret(path string) (string, error) { return secret, nil } +// LoadClientCertificate loads the service principal certificate and private key +// from ClientCertificateFile. +func (c *ServicePrincipalConfig) LoadClientCertificate() ([]*x509.Certificate, crypto.PrivateKey, error) { + cleanPath := filepath.Clean(c.ClientCertificateFile) + info, err := os.Lstat(cleanPath) + if err != nil { + return nil, nil, fmt.Errorf("stat service principal client certificate file: %w", err) + } + if info.Mode()&os.ModeSymlink != 0 { + return nil, nil, fmt.Errorf("service principal client certificate file must not be a symlink") + } + if !info.Mode().IsRegular() { + return nil, nil, fmt.Errorf("service principal client certificate file must be a regular file") + } + if info.Mode().Perm()&0o077 != 0 { + return nil, nil, fmt.Errorf("service principal client certificate file must not be accessible by group or other users") + } + data, err := os.ReadFile(cleanPath) + if err != nil { + return nil, nil, fmt.Errorf("read service principal client certificate file: %w", err) + } + certificates, privateKey, err := azidentity.ParseCertificates(data, nil) + if err != nil { + return nil, nil, fmt.Errorf("parse service principal client certificate file: %w", err) + } + return certificates, privateKey, nil +} + func (c *ManagedIdentityConfig) validate() error { return nil } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 09115d5b..ad28924c 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -1,8 +1,14 @@ package config import ( + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "crypto/x509/pkix" "encoding/json" + "encoding/pem" "fmt" + "math/big" "os" "path/filepath" "sort" @@ -1747,7 +1753,7 @@ func TestAuthenticationMethodValidation(t *testing.T) { }, }, wantErr: true, - errMsg: "azure.servicePrincipal.clientSecret or azure.servicePrincipal.clientSecretFile is required when service principal is configured", + errMsg: "one of azure.servicePrincipal.clientSecret, azure.servicePrincipal.clientSecretFile, or azure.servicePrincipal.clientCertificateFile is required when service principal is configured", }, { name: "managed identity authentication enabled", @@ -2062,6 +2068,7 @@ func TestServicePrincipalClientSecretFile(t *testing.T) { if err := os.WriteFile(validFile, []byte("file-secret\r\n"), 0o600); err != nil { t.Fatalf("os.WriteFile: %v", err) } + emptyFile := filepath.Join(dir, "empty") if err := os.WriteFile(emptyFile, nil, 0o600); err != nil { t.Fatalf("os.WriteFile: %v", err) @@ -2128,6 +2135,104 @@ func TestServicePrincipalClientSecretFile(t *testing.T) { } } +func TestServicePrincipalClientCertificateFile(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + validFile := filepath.Join(dir, "client-certificate.pem") + writeTestClientCertificate(t, validFile) + invalidFile := filepath.Join(dir, "invalid") + if err := os.WriteFile(invalidFile, []byte("not a certificate"), 0o600); err != nil { + t.Fatalf("os.WriteFile: %v", err) + } + insecureFile := filepath.Join(dir, "insecure") + writeTestClientCertificate(t, insecureFile) + if err := os.Chmod(insecureFile, 0o644); err != nil { + t.Fatalf("os.Chmod: %v", err) + } + + tests := []struct { + name string + config *ServicePrincipalConfig + wantErr string + }{ + { + name: "loads certificate", + config: &ServicePrincipalConfig{TenantID: "tenant", ClientID: "client", ClientCertificateFile: validFile}, + }, + { + name: "rejects secret and certificate", + config: &ServicePrincipalConfig{TenantID: "tenant", ClientID: "client", ClientSecret: "secret", ClientCertificateFile: validFile}, + wantErr: "only one of", + }, + { + name: "rejects invalid certificate", + config: &ServicePrincipalConfig{TenantID: "tenant", ClientID: "client", ClientCertificateFile: invalidFile}, + wantErr: "parse service principal client certificate file", + }, + { + name: "rejects insecure permissions", + config: &ServicePrincipalConfig{TenantID: "tenant", ClientID: "client", ClientCertificateFile: insecureFile}, + wantErr: "must not be accessible by group or other users", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := tt.config.validate() + if tt.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("ServicePrincipalConfig.validate() error = %v, want %q", err, tt.wantErr) + } + return + } + if err != nil { + t.Fatalf("ServicePrincipalConfig.validate() error = %v", err) + } + certificates, privateKey, err := tt.config.LoadClientCertificate() + if err != nil { + t.Fatalf("ServicePrincipalConfig.LoadClientCertificate() error = %v", err) + } + if len(certificates) != 1 || privateKey == nil { + t.Fatalf("LoadClientCertificate() returned %d certificates and key %T", len(certificates), privateKey) + } + }) + } +} + +func writeTestClientCertificate(t *testing.T, path string) { + t.Helper() + + privateKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("rsa.GenerateKey: %v", err) + } + template := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "test-client"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + } + certificate, err := x509.CreateCertificate(rand.Reader, template, template, &privateKey.PublicKey, privateKey) + if err != nil { + t.Fatalf("x509.CreateCertificate: %v", err) + } + privateKeyData, err := x509.MarshalPKCS8PrivateKey(privateKey) + if err != nil { + t.Fatalf("x509.MarshalPKCS8PrivateKey: %v", err) + } + data := append( + pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certificate}), + pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: privateKeyData})..., + ) + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatalf("os.WriteFile: %v", err) + } +} + func TestValidateBootstrapTokenAPIServerURL(t *testing.T) { t.Parallel() diff --git a/pkg/daemon/install.go b/pkg/daemon/install.go index d7dbea24..21920500 100644 --- a/pkg/daemon/install.go +++ b/pkg/daemon/install.go @@ -14,6 +14,11 @@ type installBinaryTask struct { machineDir string } +type installClientCertificateTask struct { + sourcePath string + machineDir string +} + // InstallBinary returns a task that copies the current process binary into // the nspawn rootfs at /usr/local/bin/aks-flex-node. func InstallBinary(machineDir string) phases.Task { @@ -51,3 +56,52 @@ func (t *installBinaryTask) Do(_ context.Context) error { return nil } + +// InstallClientCertificate returns a task that copies a configured service +// principal certificate to the same absolute path inside the nspawn rootfs. +func InstallClientCertificate(sourcePath, machineDir string) phases.Task { + return &installClientCertificateTask{sourcePath: sourcePath, machineDir: machineDir} +} + +func (t *installClientCertificateTask) Name() string { return "install-client-certificate-in-rootfs" } + +func (t *installClientCertificateTask) Do(_ context.Context) error { + if t.sourcePath == "" { + return nil + } + if !filepath.IsAbs(t.sourcePath) { + return fmt.Errorf("client certificate path must be absolute") + } + + source, err := os.Open(filepath.Clean(t.sourcePath)) //nolint:gosec // validated config path + if err != nil { + return fmt.Errorf("open client certificate: %w", err) + } + defer func() { _ = source.Close() }() + + relativePath, err := filepath.Rel(string(filepath.Separator), t.sourcePath) + if err != nil { + return fmt.Errorf("resolve client certificate path in rootfs: %w", err) + } + machineRoot, err := os.OpenRoot(t.machineDir) + if err != nil { + return fmt.Errorf("open machine rootfs: %w", err) + } + defer func() { _ = machineRoot.Close() }() + if err := machineRoot.MkdirAll(filepath.Dir(relativePath), 0o750); err != nil { + return fmt.Errorf("create client certificate directory: %w", err) + } + destination, err := machineRoot.OpenFile(relativePath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) + if err != nil { + return fmt.Errorf("create client certificate: %w", err) + } + defer func() { _ = destination.Close() }() + if err := destination.Chmod(0o600); err != nil { + return fmt.Errorf("set client certificate permissions: %w", err) + } + + if _, err := io.Copy(destination, source); err != nil { + return fmt.Errorf("copy client certificate: %w", err) + } + return nil +} diff --git a/pkg/daemon/install_test.go b/pkg/daemon/install_test.go new file mode 100644 index 00000000..6f93fba5 --- /dev/null +++ b/pkg/daemon/install_test.go @@ -0,0 +1,49 @@ +package daemon + +import ( + "os" + "path/filepath" + "testing" +) + +func TestInstallClientCertificate(t *testing.T) { + t.Parallel() + + sourceDir := t.TempDir() + sourcePath := filepath.Join(sourceDir, "credentials", "client.pem") + if err := os.MkdirAll(filepath.Dir(sourcePath), 0o750); err != nil { + t.Fatalf("os.MkdirAll: %v", err) + } + const certificate = "test certificate" + if err := os.WriteFile(sourcePath, []byte(certificate), 0o600); err != nil { + t.Fatalf("os.WriteFile: %v", err) + } + + machineDir := t.TempDir() + destinationPath := filepath.Join(machineDir, sourcePath) + if err := os.MkdirAll(filepath.Dir(destinationPath), 0o750); err != nil { + t.Fatalf("os.MkdirAll: %v", err) + } + if err := os.WriteFile(destinationPath, []byte("old"), 0o644); err != nil { + t.Fatalf("os.WriteFile: %v", err) + } + + task := InstallClientCertificate(sourcePath, machineDir) + if err := task.Do(t.Context()); err != nil { + t.Fatalf("InstallClientCertificate().Do() error = %v", err) + } + data, err := os.ReadFile(destinationPath) + if err != nil { + t.Fatalf("os.ReadFile: %v", err) + } + if string(data) != certificate { + t.Fatalf("installed certificate = %q, want %q", data, certificate) + } + info, err := os.Stat(destinationPath) + if err != nil { + t.Fatalf("os.Stat: %v", err) + } + if info.Mode().Perm() != 0o600 { + t.Fatalf("installed certificate permissions = %o, want 600", info.Mode().Perm()) + } +} diff --git a/pkg/daemon/start.go b/pkg/daemon/start.go index 7557612b..28b47889 100644 --- a/pkg/daemon/start.go +++ b/pkg/daemon/start.go @@ -52,6 +52,7 @@ func StartNode( phases.Parallel(log, npd.Download(log, cfg, gs.RootFS.MachineDir), InstallBinary(gs.RootFS.MachineDir), + InstallClientCertificate(servicePrincipalClientCertificateFile(cfg), gs.RootFS.MachineDir), ), nodestart.StartNode(log, gs.NodeStart), nodestart.WaitForKubelet(log, machineName), @@ -59,3 +60,10 @@ func StartNode( saveState(store, state), ) } + +func servicePrincipalClientCertificateFile(cfg *config.Config) string { + if cfg.Azure.ServicePrincipal == nil { + return "" + } + return cfg.Azure.ServicePrincipal.ClientCertificateFile +} From 613770a483905bfb6a1513266bb7e138144204d7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Jul 2026 21:24:01 +0000 Subject: [PATCH 3/8] Normalize certificates for kubelet auth --- pkg/cmd/token/kubelogin/kubelogin.go | 32 ++++++++++++++ pkg/cmd/token/kubelogin/kubelogin_test.go | 37 ++++++++++++++++ pkg/config/adapter.go | 5 ++- pkg/config/adapter_test.go | 10 +++-- pkg/config/config.go | 34 +++++++++++++- pkg/config/config_test.go | 4 ++ pkg/daemon/install.go | 54 ----------------------- pkg/daemon/install_test.go | 49 -------------------- pkg/daemon/start.go | 8 ---- 9 files changed, 117 insertions(+), 116 deletions(-) create mode 100644 pkg/cmd/token/kubelogin/kubelogin_test.go delete mode 100644 pkg/daemon/install_test.go diff --git a/pkg/cmd/token/kubelogin/kubelogin.go b/pkg/cmd/token/kubelogin/kubelogin.go index 68bbb02d..9de58e10 100644 --- a/pkg/cmd/token/kubelogin/kubelogin.go +++ b/pkg/cmd/token/kubelogin/kubelogin.go @@ -2,6 +2,7 @@ package kubelogin import ( "context" + "encoding/base64" "encoding/json" "fmt" "io" @@ -20,6 +21,7 @@ import ( ) const aksAADServerID = "6dae42f8-4368-4678-94ff-3960e28e3630" +const clientCertificateDataEnv = "AKS_FLEX_NODE_CLIENT_CERTIFICATE_DATA" var flagServerID string var flagPopEnabled bool @@ -63,6 +65,14 @@ func run(ctx context.Context, out io.Writer) error { tokOpts.ServerID = flagServerID tokOpts.IsPoPTokenEnabled = flagPopEnabled tokOpts.PoPTokenClaims = flagPopClaims + if certificateData := os.Getenv(clientCertificateDataEnv); certificateData != "" { + certificatePath, cleanup, err := prepareClientCertificate(certificateData) + if err != nil { + return err + } + defer cleanup() + tokOpts.ClientCert = certificatePath + } // TODO: logging to show login details provider, err := token.GetTokenProvider(tokOpts) if err != nil { @@ -76,6 +86,28 @@ func run(ctx context.Context, out io.Writer) error { return outputToken(out, ec, accessToken) } +func prepareClientCertificate(encodedData string) (string, func(), error) { + data, err := base64.StdEncoding.DecodeString(encodedData) + if err != nil { + return "", nil, fmt.Errorf("decode client certificate data: %w", err) + } + file, err := os.CreateTemp("", "aks-flex-node-client-certificate-*.pem") + if err != nil { + return "", nil, fmt.Errorf("create temporary client certificate: %w", err) + } + cleanup := func() { _ = os.Remove(file.Name()) } + if _, err := file.Write(data); err != nil { + _ = file.Close() + cleanup() + return "", nil, fmt.Errorf("write temporary client certificate: %w", err) + } + if err := file.Close(); err != nil { + cleanup() + return "", nil, fmt.Errorf("close temporary client certificate: %w", err) + } + return file.Name(), cleanup, nil +} + const execInfoEnv = "KUBERNETES_EXEC_INFO" var scheme = runtime.NewScheme() diff --git a/pkg/cmd/token/kubelogin/kubelogin_test.go b/pkg/cmd/token/kubelogin/kubelogin_test.go new file mode 100644 index 00000000..494e882e --- /dev/null +++ b/pkg/cmd/token/kubelogin/kubelogin_test.go @@ -0,0 +1,37 @@ +package kubelogin + +import ( + "encoding/base64" + "os" + "testing" +) + +func TestPrepareClientCertificate(t *testing.T) { + t.Parallel() + + const certificate = "certificate data" + path, cleanup, err := prepareClientCertificate(base64.StdEncoding.EncodeToString([]byte(certificate))) + if err != nil { + t.Fatalf("prepareClientCertificate() error = %v", err) + } + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("os.ReadFile: %v", err) + } + if string(data) != certificate { + t.Fatalf("temporary certificate = %q, want %q", data, certificate) + } + info, err := os.Stat(path) + if err != nil { + t.Fatalf("os.Stat: %v", err) + } + if info.Mode().Perm() != 0o600 { + t.Fatalf("temporary certificate permissions = %o, want 600", info.Mode().Perm()) + } + + cleanup() + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatalf("temporary certificate still exists after cleanup: %v", err) + } +} diff --git a/pkg/config/adapter.go b/pkg/config/adapter.go index 336bef26..c4465af3 100644 --- a/pkg/config/adapter.go +++ b/pkg/config/adapter.go @@ -1,6 +1,7 @@ package config import ( + "encoding/base64" "fmt" "log/slog" @@ -17,6 +18,8 @@ const ( // aksAADServerID is the Azure AD server application ID for AKS. aksAADServerID = "6dae42f8-4368-4678-94ff-3960e28e3630" + + clientCertificateDataEnv = "AKS_FLEX_NODE_CLIENT_CERTIFICATE_DATA" ) // ToAgentConfig converts a FlexNode Config to the shared agent library's @@ -72,7 +75,7 @@ func ToAgentConfig(cfg *Config, machineName string) *agentconfig.AgentConfig { "AZURE_TENANT_ID": cfg.Azure.ServicePrincipal.TenantID, } if cfg.Azure.ServicePrincipal.ClientCertificateFile != "" { - env["AAD_SERVICE_PRINCIPAL_CLIENT_CERTIFICATE"] = cfg.Azure.ServicePrincipal.ClientCertificateFile + env[clientCertificateDataEnv] = base64.StdEncoding.EncodeToString([]byte(cfg.Azure.ServicePrincipal.clientCertificateData())) } else { env["AAD_SERVICE_PRINCIPAL_CLIENT_SECRET"] = cfg.Azure.ServicePrincipal.ClientSecret } diff --git a/pkg/config/adapter_test.go b/pkg/config/adapter_test.go index 020040d0..33840d00 100644 --- a/pkg/config/adapter_test.go +++ b/pkg/config/adapter_test.go @@ -169,7 +169,8 @@ func TestToAgentConfig_ServicePrincipalClientSecretFile(t *testing.T) { func TestToAgentConfig_ServicePrincipalClientCertificateFile(t *testing.T) { t.Parallel() - certificateFile := "/run/credentials/aks-flex-node-sp.pem" + certificateFile := filepath.Join(t.TempDir(), "client-certificate.pem") + writeTestClientCertificate(t, certificateFile) cfg := &Config{ Azure: AzureConfig{ ServicePrincipal: &ServicePrincipalConfig{ @@ -179,6 +180,9 @@ func TestToAgentConfig_ServicePrincipalClientCertificateFile(t *testing.T) { }, }, } + if err := cfg.Azure.ServicePrincipal.validate(); err != nil { + t.Fatalf("ServicePrincipalConfig.validate() error = %v", err) + } exec := ToAgentConfig(cfg, "kube1").Kubelet.Auth.ExecCredential if exec == nil { @@ -188,8 +192,8 @@ func TestToAgentConfig_ServicePrincipalClientCertificateFile(t *testing.T) { for _, e := range exec.Env { envMap[e.Name] = e.Value } - if envMap["AAD_SERVICE_PRINCIPAL_CLIENT_CERTIFICATE"] != certificateFile { - t.Fatalf("AAD_SERVICE_PRINCIPAL_CLIENT_CERTIFICATE=%q, want %q", envMap["AAD_SERVICE_PRINCIPAL_CLIENT_CERTIFICATE"], certificateFile) + if envMap[clientCertificateDataEnv] == "" { + t.Fatalf("%s should contain the certificate data", clientCertificateDataEnv) } if _, ok := envMap["AAD_SERVICE_PRINCIPAL_CLIENT_SECRET"]; ok { t.Fatal("AAD_SERVICE_PRINCIPAL_CLIENT_SECRET should not be set for certificate auth") diff --git a/pkg/config/config.go b/pkg/config/config.go index 09fa6a0b..820bc29a 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -2,8 +2,10 @@ package config import ( "crypto" + "crypto/rsa" "crypto/x509" "encoding/json" + "encoding/pem" "fmt" "net/url" "os" @@ -84,6 +86,7 @@ type ServicePrincipalConfig struct { ClientSecret string `json:"clientSecret,omitempty"` // Azure AD application client secret ClientSecretFile string `json:"clientSecretFile,omitempty"` // File containing the Azure AD application client secret ClientCertificateFile string `json:"clientCertificateFile,omitempty"` // File containing an Azure AD application certificate and private key + clientCertificatePEM string } // ManagedIdentityConfig holds managed identity authentication configuration. @@ -335,6 +338,9 @@ func (cfg *Config) DeepCopy() *Config { if err := json.Unmarshal(data, &out); err != nil { return nil } + if cfg.Azure.ServicePrincipal != nil && out.Azure.ServicePrincipal != nil { + out.Azure.ServicePrincipal.clientCertificatePEM = cfg.Azure.ServicePrincipal.clientCertificatePEM + } return &out } @@ -610,9 +616,15 @@ func (c *ServicePrincipalConfig) validate() error { return fmt.Errorf("invalid azure.servicePrincipal.clientCertificateFile: resolve absolute path: %w", err) } c.ClientCertificateFile = absolutePath - if _, _, err := c.LoadClientCertificate(); err != nil { + certificates, privateKey, err := c.LoadClientCertificate() + if err != nil { return fmt.Errorf("invalid azure.servicePrincipal.clientCertificateFile: %w", err) } + clientCertificatePEM, err := marshalClientCertificatePEM(certificates, privateKey) + if err != nil { + return fmt.Errorf("invalid azure.servicePrincipal.clientCertificateFile: %w", err) + } + c.clientCertificatePEM = clientCertificatePEM } return nil } @@ -674,6 +686,26 @@ func (c *ServicePrincipalConfig) LoadClientCertificate() ([]*x509.Certificate, c return certificates, privateKey, nil } +func marshalClientCertificatePEM(certificates []*x509.Certificate, privateKey crypto.PrivateKey) (string, error) { + if _, ok := privateKey.(*rsa.PrivateKey); !ok { + return "", fmt.Errorf("service principal client certificate private key must be RSA") + } + privateKeyData, err := x509.MarshalPKCS8PrivateKey(privateKey) + if err != nil { + return "", fmt.Errorf("marshal service principal client certificate private key: %w", err) + } + var data []byte + for _, certificate := range certificates { + data = append(data, pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certificate.Raw})...) + } + data = append(data, pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: privateKeyData})...) + return string(data), nil +} + +func (c *ServicePrincipalConfig) clientCertificateData() string { + return c.clientCertificatePEM +} + func (c *ManagedIdentityConfig) validate() error { return nil } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index ad28924c..d040f77e 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -2198,6 +2198,10 @@ func TestServicePrincipalClientCertificateFile(t *testing.T) { if len(certificates) != 1 || privateKey == nil { t.Fatalf("LoadClientCertificate() returned %d certificates and key %T", len(certificates), privateKey) } + copied := (&Config{Azure: AzureConfig{ServicePrincipal: tt.config}}).DeepCopy() + if copied == nil || copied.Azure.ServicePrincipal.clientCertificateData() == "" { + t.Fatal("DeepCopy() did not preserve normalized client certificate data") + } }) } } diff --git a/pkg/daemon/install.go b/pkg/daemon/install.go index 21920500..d7dbea24 100644 --- a/pkg/daemon/install.go +++ b/pkg/daemon/install.go @@ -14,11 +14,6 @@ type installBinaryTask struct { machineDir string } -type installClientCertificateTask struct { - sourcePath string - machineDir string -} - // InstallBinary returns a task that copies the current process binary into // the nspawn rootfs at /usr/local/bin/aks-flex-node. func InstallBinary(machineDir string) phases.Task { @@ -56,52 +51,3 @@ func (t *installBinaryTask) Do(_ context.Context) error { return nil } - -// InstallClientCertificate returns a task that copies a configured service -// principal certificate to the same absolute path inside the nspawn rootfs. -func InstallClientCertificate(sourcePath, machineDir string) phases.Task { - return &installClientCertificateTask{sourcePath: sourcePath, machineDir: machineDir} -} - -func (t *installClientCertificateTask) Name() string { return "install-client-certificate-in-rootfs" } - -func (t *installClientCertificateTask) Do(_ context.Context) error { - if t.sourcePath == "" { - return nil - } - if !filepath.IsAbs(t.sourcePath) { - return fmt.Errorf("client certificate path must be absolute") - } - - source, err := os.Open(filepath.Clean(t.sourcePath)) //nolint:gosec // validated config path - if err != nil { - return fmt.Errorf("open client certificate: %w", err) - } - defer func() { _ = source.Close() }() - - relativePath, err := filepath.Rel(string(filepath.Separator), t.sourcePath) - if err != nil { - return fmt.Errorf("resolve client certificate path in rootfs: %w", err) - } - machineRoot, err := os.OpenRoot(t.machineDir) - if err != nil { - return fmt.Errorf("open machine rootfs: %w", err) - } - defer func() { _ = machineRoot.Close() }() - if err := machineRoot.MkdirAll(filepath.Dir(relativePath), 0o750); err != nil { - return fmt.Errorf("create client certificate directory: %w", err) - } - destination, err := machineRoot.OpenFile(relativePath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) - if err != nil { - return fmt.Errorf("create client certificate: %w", err) - } - defer func() { _ = destination.Close() }() - if err := destination.Chmod(0o600); err != nil { - return fmt.Errorf("set client certificate permissions: %w", err) - } - - if _, err := io.Copy(destination, source); err != nil { - return fmt.Errorf("copy client certificate: %w", err) - } - return nil -} diff --git a/pkg/daemon/install_test.go b/pkg/daemon/install_test.go deleted file mode 100644 index 6f93fba5..00000000 --- a/pkg/daemon/install_test.go +++ /dev/null @@ -1,49 +0,0 @@ -package daemon - -import ( - "os" - "path/filepath" - "testing" -) - -func TestInstallClientCertificate(t *testing.T) { - t.Parallel() - - sourceDir := t.TempDir() - sourcePath := filepath.Join(sourceDir, "credentials", "client.pem") - if err := os.MkdirAll(filepath.Dir(sourcePath), 0o750); err != nil { - t.Fatalf("os.MkdirAll: %v", err) - } - const certificate = "test certificate" - if err := os.WriteFile(sourcePath, []byte(certificate), 0o600); err != nil { - t.Fatalf("os.WriteFile: %v", err) - } - - machineDir := t.TempDir() - destinationPath := filepath.Join(machineDir, sourcePath) - if err := os.MkdirAll(filepath.Dir(destinationPath), 0o750); err != nil { - t.Fatalf("os.MkdirAll: %v", err) - } - if err := os.WriteFile(destinationPath, []byte("old"), 0o644); err != nil { - t.Fatalf("os.WriteFile: %v", err) - } - - task := InstallClientCertificate(sourcePath, machineDir) - if err := task.Do(t.Context()); err != nil { - t.Fatalf("InstallClientCertificate().Do() error = %v", err) - } - data, err := os.ReadFile(destinationPath) - if err != nil { - t.Fatalf("os.ReadFile: %v", err) - } - if string(data) != certificate { - t.Fatalf("installed certificate = %q, want %q", data, certificate) - } - info, err := os.Stat(destinationPath) - if err != nil { - t.Fatalf("os.Stat: %v", err) - } - if info.Mode().Perm() != 0o600 { - t.Fatalf("installed certificate permissions = %o, want 600", info.Mode().Perm()) - } -} diff --git a/pkg/daemon/start.go b/pkg/daemon/start.go index 28b47889..7557612b 100644 --- a/pkg/daemon/start.go +++ b/pkg/daemon/start.go @@ -52,7 +52,6 @@ func StartNode( phases.Parallel(log, npd.Download(log, cfg, gs.RootFS.MachineDir), InstallBinary(gs.RootFS.MachineDir), - InstallClientCertificate(servicePrincipalClientCertificateFile(cfg), gs.RootFS.MachineDir), ), nodestart.StartNode(log, gs.NodeStart), nodestart.WaitForKubelet(log, machineName), @@ -60,10 +59,3 @@ func StartNode( saveState(store, state), ) } - -func servicePrincipalClientCertificateFile(cfg *config.Config) string { - if cfg.Azure.ServicePrincipal == nil { - return "" - } - return cfg.Azure.ServicePrincipal.ClientCertificateFile -} From ae4bd15df4dcee0b910e6ccbf701229795ce15f6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Jul 2026 21:40:17 +0000 Subject: [PATCH 4/8] Reuse secret file for certificates --- docs/usages/configuration.md | 5 +- docs/usages/joining-nodes.md | 2 +- pkg/aksmachine/client_armapi.go | 2 +- pkg/aksmachine/client_armapi_test.go | 6 +- pkg/config/adapter.go | 2 +- pkg/config/adapter_test.go | 8 +- pkg/config/config.go | 107 +++++++++++---------------- pkg/config/config_test.go | 23 ++---- 8 files changed, 61 insertions(+), 94 deletions(-) diff --git a/docs/usages/configuration.md b/docs/usages/configuration.md index 1ce89602..c7594fe7 100644 --- a/docs/usages/configuration.md +++ b/docs/usages/configuration.md @@ -81,9 +81,8 @@ At least one join or Azure authentication method must be configured. `azure.boot |------|------|-------------|--------------| | `azure.servicePrincipal.tenantId` | string | Microsoft Entra tenant ID for the service principal. | `70a036f6-8e4d-4615-bad6-149c02e7720d` | | `azure.servicePrincipal.clientId` | string | Application client ID. | `00000000-0000-0000-0000-000000000000` | -| `azure.servicePrincipal.clientSecret` | string | Application client secret. Mutually exclusive with the credential file options. | `` | -| `azure.servicePrincipal.clientSecretFile` | string | Path to a protected regular file (no group/other access; e.g., 0600) containing the application client secret. Mutually exclusive with the other credential options. | `/run/credentials/aks-flex-node-sp` | -| `azure.servicePrincipal.clientCertificateFile` | string | Path to a protected PEM or unencrypted PFX file containing the application certificate and private key. Mutually exclusive with the other credential options. | `/run/credentials/aks-flex-node-sp.pem` | +| `azure.servicePrincipal.clientSecret` | string | Application client secret. Mutually exclusive with `clientSecretFile`. | `` | +| `azure.servicePrincipal.clientSecretFile` | string | Path to a protected regular file (no group/other access; e.g., 0600) containing either the application client secret or a PEM/unencrypted PFX certificate and private key. | `/run/credentials/aks-flex-node-sp` | ## Agent diff --git a/docs/usages/joining-nodes.md b/docs/usages/joining-nodes.md index f477e00e..5ff58a4c 100644 --- a/docs/usages/joining-nodes.md +++ b/docs/usages/joining-nodes.md @@ -107,7 +107,7 @@ Minimal config shape: } ``` -The secret credential file contains only the client secret. Alternatively, set `clientCertificateFile` to a PEM or unencrypted PFX file containing the application certificate and private key. The agent requires credential files to be non-empty regular files with no group/world access (for example, mode 0600). Only one of `clientSecret`, `clientSecretFile`, or `clientCertificateFile` can be configured. +The credential file contains either the client secret or a PEM/unencrypted PFX application certificate and private key; the agent detects the credential type from its contents. The file must be a non-empty regular file with no group/world access (for example, mode 0600). Only one of `clientSecret` or `clientSecretFile` can be configured. ## Authentication Mode Selection diff --git a/pkg/aksmachine/client_armapi.go b/pkg/aksmachine/client_armapi.go index f4ad0a9a..7114abde 100644 --- a/pkg/aksmachine/client_armapi.go +++ b/pkg/aksmachine/client_armapi.go @@ -165,7 +165,7 @@ func getCredential(cfg *config.Config, logger *slog.Logger, clientOpts azcore.Cl "tenantID", cfg.Azure.ServicePrincipal.TenantID, "clientID", cfg.Azure.ServicePrincipal.ClientID, ) - if cfg.Azure.ServicePrincipal.ClientCertificateFile != "" { + if cfg.Azure.ServicePrincipal.ClientSecretFile != "" { certificates, privateKey, err := cfg.Azure.ServicePrincipal.LoadClientCertificate() if err != nil { return nil, fmt.Errorf("load service principal client certificate: %w", err) diff --git a/pkg/aksmachine/client_armapi_test.go b/pkg/aksmachine/client_armapi_test.go index 0e8333f6..63429b91 100644 --- a/pkg/aksmachine/client_armapi_test.go +++ b/pkg/aksmachine/client_armapi_test.go @@ -170,9 +170,9 @@ func TestGetCredentialClientCertificateLoadError(t *testing.T) { cfg := testARMConfig(testClusterResourceID, "flex-node-1", "1.34.0") cfg.Azure.ServicePrincipal = &config.ServicePrincipalConfig{ - TenantID: "tenant", - ClientID: "client", - ClientCertificateFile: filepath.Join(t.TempDir(), "missing"), + TenantID: "tenant", + ClientID: "client", + ClientSecretFile: filepath.Join(t.TempDir(), "missing"), } credential, err := getCredential( cfg, diff --git a/pkg/config/adapter.go b/pkg/config/adapter.go index c4465af3..d9a02f66 100644 --- a/pkg/config/adapter.go +++ b/pkg/config/adapter.go @@ -74,7 +74,7 @@ func ToAgentConfig(cfg *Config, machineName string) *agentconfig.AgentConfig { "AAD_SERVICE_PRINCIPAL_CLIENT_ID": cfg.Azure.ServicePrincipal.ClientID, "AZURE_TENANT_ID": cfg.Azure.ServicePrincipal.TenantID, } - if cfg.Azure.ServicePrincipal.ClientCertificateFile != "" { + if cfg.Azure.ServicePrincipal.clientCertificateData() != "" { env[clientCertificateDataEnv] = base64.StdEncoding.EncodeToString([]byte(cfg.Azure.ServicePrincipal.clientCertificateData())) } else { env["AAD_SERVICE_PRINCIPAL_CLIENT_SECRET"] = cfg.Azure.ServicePrincipal.ClientSecret diff --git a/pkg/config/adapter_test.go b/pkg/config/adapter_test.go index 33840d00..2a7773f2 100644 --- a/pkg/config/adapter_test.go +++ b/pkg/config/adapter_test.go @@ -166,7 +166,7 @@ func TestToAgentConfig_ServicePrincipalClientSecretFile(t *testing.T) { } } -func TestToAgentConfig_ServicePrincipalClientCertificateFile(t *testing.T) { +func TestToAgentConfig_ServicePrincipalCertificateFile(t *testing.T) { t.Parallel() certificateFile := filepath.Join(t.TempDir(), "client-certificate.pem") @@ -174,9 +174,9 @@ func TestToAgentConfig_ServicePrincipalClientCertificateFile(t *testing.T) { cfg := &Config{ Azure: AzureConfig{ ServicePrincipal: &ServicePrincipalConfig{ - TenantID: "tenant-123", - ClientID: "client-456", - ClientCertificateFile: certificateFile, + TenantID: "tenant-123", + ClientID: "client-456", + ClientSecretFile: certificateFile, }, }, } diff --git a/pkg/config/config.go b/pkg/config/config.go index 820bc29a..98bb18fc 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -81,12 +81,11 @@ type AzureConfig struct { // ServicePrincipalConfig holds Azure service principal authentication configuration. // When provided, service principal authentication will be used instead of Azure CLI. type ServicePrincipalConfig struct { - TenantID string `json:"tenantId"` // Azure AD tenant ID - ClientID string `json:"clientId"` // Azure AD application (client) ID - ClientSecret string `json:"clientSecret,omitempty"` // Azure AD application client secret - ClientSecretFile string `json:"clientSecretFile,omitempty"` // File containing the Azure AD application client secret - ClientCertificateFile string `json:"clientCertificateFile,omitempty"` // File containing an Azure AD application certificate and private key - clientCertificatePEM string + TenantID string `json:"tenantId"` // Azure AD tenant ID + ClientID string `json:"clientId"` // Azure AD application (client) ID + ClientSecret string `json:"clientSecret,omitempty"` // Azure AD application client secret + ClientSecretFile string `json:"clientSecretFile,omitempty"` // File containing an Azure AD application client secret or certificate + clientCertificatePEM string } // ManagedIdentityConfig holds managed identity authentication configuration. @@ -590,94 +589,72 @@ func (c *ServicePrincipalConfig) validate() error { if c.ClientID == "" { return fmt.Errorf("azure.servicePrincipal.clientId is required when service principal is configured") } - configuredCredentials := 0 - for _, credential := range []string{c.ClientSecret, c.ClientSecretFile, c.ClientCertificateFile} { - if credential != "" { - configuredCredentials++ - } - } - if configuredCredentials > 1 { - return fmt.Errorf("only one of azure.servicePrincipal.clientSecret, azure.servicePrincipal.clientSecretFile, or azure.servicePrincipal.clientCertificateFile can be configured") + if c.ClientSecret != "" && c.ClientSecretFile != "" { + return fmt.Errorf("only one of azure.servicePrincipal.clientSecret or azure.servicePrincipal.clientSecretFile can be configured") } - if configuredCredentials == 0 { - return fmt.Errorf("one of azure.servicePrincipal.clientSecret, azure.servicePrincipal.clientSecretFile, or azure.servicePrincipal.clientCertificateFile is required when service principal is configured") + if c.ClientSecret == "" && c.ClientSecretFile == "" { + return fmt.Errorf("one of azure.servicePrincipal.clientSecret or azure.servicePrincipal.clientSecretFile is required when service principal is configured") } if c.ClientSecretFile != "" { - clientSecret, err := loadServicePrincipalClientSecret(c.ClientSecretFile) - if err != nil { - return fmt.Errorf("invalid azure.servicePrincipal.clientSecretFile: %w", err) - } - c.ClientSecret = clientSecret - c.ClientSecretFile = "" - } - if c.ClientCertificateFile != "" { - absolutePath, err := filepath.Abs(c.ClientCertificateFile) + absolutePath, err := filepath.Abs(c.ClientSecretFile) if err != nil { - return fmt.Errorf("invalid azure.servicePrincipal.clientCertificateFile: resolve absolute path: %w", err) + return fmt.Errorf("invalid azure.servicePrincipal.clientSecretFile: resolve absolute path: %w", err) } - c.ClientCertificateFile = absolutePath - certificates, privateKey, err := c.LoadClientCertificate() + c.ClientSecretFile = absolutePath + data, err := loadServicePrincipalCredentialFile(c.ClientSecretFile) if err != nil { - return fmt.Errorf("invalid azure.servicePrincipal.clientCertificateFile: %w", err) + return fmt.Errorf("invalid azure.servicePrincipal.clientSecretFile: %w", err) } - clientCertificatePEM, err := marshalClientCertificatePEM(certificates, privateKey) - if err != nil { - return fmt.Errorf("invalid azure.servicePrincipal.clientCertificateFile: %w", err) + certificates, privateKey, certificateErr := azidentity.ParseCertificates(data, nil) + if certificateErr == nil { + clientCertificatePEM, err := marshalClientCertificatePEM(certificates, privateKey) + if err != nil { + return fmt.Errorf("invalid azure.servicePrincipal.clientSecretFile: %w", err) + } + c.clientCertificatePEM = clientCertificatePEM + } else { + c.ClientSecret = strings.TrimRight(string(data), "\r\n") + if c.ClientSecret == "" { + return fmt.Errorf("invalid azure.servicePrincipal.clientSecretFile: service principal credential file is empty") + } + c.ClientSecretFile = "" } - c.clientCertificatePEM = clientCertificatePEM } return nil } -// loadServicePrincipalClientSecret reads a service principal secret from a -// protected file. A trailing line ending is ignored to support standard secret -// file creation tools. -func loadServicePrincipalClientSecret(path string) (string, error) { +func loadServicePrincipalCredentialFile(path string) ([]byte, error) { cleanPath := filepath.Clean(path) info, err := os.Lstat(cleanPath) if err != nil { - return "", fmt.Errorf("stat service principal client secret file: %w", err) + return nil, fmt.Errorf("stat service principal credential file: %w", err) } if info.Mode()&os.ModeSymlink != 0 { - return "", fmt.Errorf("service principal client secret file must not be a symlink") + return nil, fmt.Errorf("service principal credential file must not be a symlink") } if !info.Mode().IsRegular() { - return "", fmt.Errorf("service principal client secret file must be a regular file") + return nil, fmt.Errorf("service principal credential file must be a regular file") } if info.Mode().Perm()&0o077 != 0 { - return "", fmt.Errorf("service principal client secret file must not be accessible by group or other users") + return nil, fmt.Errorf("service principal credential file must not be accessible by group or other users") } data, err := os.ReadFile(cleanPath) if err != nil { - return "", fmt.Errorf("read service principal client secret file: %w", err) - } - secret := strings.TrimRight(string(data), "\r\n") - if secret == "" { - return "", fmt.Errorf("service principal client secret file is empty") + return nil, fmt.Errorf("read service principal credential file: %w", err) } - return secret, nil + return data, nil } // LoadClientCertificate loads the service principal certificate and private key -// from ClientCertificateFile. +// from ClientSecretFile. func (c *ServicePrincipalConfig) LoadClientCertificate() ([]*x509.Certificate, crypto.PrivateKey, error) { - cleanPath := filepath.Clean(c.ClientCertificateFile) - info, err := os.Lstat(cleanPath) - if err != nil { - return nil, nil, fmt.Errorf("stat service principal client certificate file: %w", err) - } - if info.Mode()&os.ModeSymlink != 0 { - return nil, nil, fmt.Errorf("service principal client certificate file must not be a symlink") - } - if !info.Mode().IsRegular() { - return nil, nil, fmt.Errorf("service principal client certificate file must be a regular file") - } - if info.Mode().Perm()&0o077 != 0 { - return nil, nil, fmt.Errorf("service principal client certificate file must not be accessible by group or other users") - } - data, err := os.ReadFile(cleanPath) - if err != nil { - return nil, nil, fmt.Errorf("read service principal client certificate file: %w", err) + data := []byte(c.clientCertificatePEM) + if len(data) == 0 { + var err error + data, err = loadServicePrincipalCredentialFile(c.ClientSecretFile) + if err != nil { + return nil, nil, err + } } certificates, privateKey, err := azidentity.ParseCertificates(data, nil) if err != nil { diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index d040f77e..9a6d2389 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -1753,7 +1753,7 @@ func TestAuthenticationMethodValidation(t *testing.T) { }, }, wantErr: true, - errMsg: "one of azure.servicePrincipal.clientSecret, azure.servicePrincipal.clientSecretFile, or azure.servicePrincipal.clientCertificateFile is required when service principal is configured", + errMsg: "one of azure.servicePrincipal.clientSecret or azure.servicePrincipal.clientSecretFile is required when service principal is configured", }, { name: "managed identity authentication enabled", @@ -2097,12 +2097,12 @@ func TestServicePrincipalClientSecretFile(t *testing.T) { { name: "rejects missing file", config: &ServicePrincipalConfig{TenantID: "tenant", ClientID: "client", ClientSecretFile: filepath.Join(dir, "missing")}, - wantErr: "stat service principal client secret file", + wantErr: "stat service principal credential file", }, { name: "rejects empty file", config: &ServicePrincipalConfig{TenantID: "tenant", ClientID: "client", ClientSecretFile: emptyFile}, - wantErr: "service principal client secret file is empty", + wantErr: "service principal credential file is empty", }, { name: "rejects insecure permissions", @@ -2135,16 +2135,12 @@ func TestServicePrincipalClientSecretFile(t *testing.T) { } } -func TestServicePrincipalClientCertificateFile(t *testing.T) { +func TestServicePrincipalCertificateFile(t *testing.T) { t.Parallel() dir := t.TempDir() validFile := filepath.Join(dir, "client-certificate.pem") writeTestClientCertificate(t, validFile) - invalidFile := filepath.Join(dir, "invalid") - if err := os.WriteFile(invalidFile, []byte("not a certificate"), 0o600); err != nil { - t.Fatalf("os.WriteFile: %v", err) - } insecureFile := filepath.Join(dir, "insecure") writeTestClientCertificate(t, insecureFile) if err := os.Chmod(insecureFile, 0o644); err != nil { @@ -2158,21 +2154,16 @@ func TestServicePrincipalClientCertificateFile(t *testing.T) { }{ { name: "loads certificate", - config: &ServicePrincipalConfig{TenantID: "tenant", ClientID: "client", ClientCertificateFile: validFile}, + config: &ServicePrincipalConfig{TenantID: "tenant", ClientID: "client", ClientSecretFile: validFile}, }, { name: "rejects secret and certificate", - config: &ServicePrincipalConfig{TenantID: "tenant", ClientID: "client", ClientSecret: "secret", ClientCertificateFile: validFile}, + config: &ServicePrincipalConfig{TenantID: "tenant", ClientID: "client", ClientSecret: "secret", ClientSecretFile: validFile}, wantErr: "only one of", }, - { - name: "rejects invalid certificate", - config: &ServicePrincipalConfig{TenantID: "tenant", ClientID: "client", ClientCertificateFile: invalidFile}, - wantErr: "parse service principal client certificate file", - }, { name: "rejects insecure permissions", - config: &ServicePrincipalConfig{TenantID: "tenant", ClientID: "client", ClientCertificateFile: insecureFile}, + config: &ServicePrincipalConfig{TenantID: "tenant", ClientID: "client", ClientSecretFile: insecureFile}, wantErr: "must not be accessible by group or other users", }, } From b983420626de2c3476f8099811c891c630c79018 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Jul 2026 21:48:56 +0000 Subject: [PATCH 5/8] Reject malformed certificate files --- pkg/config/config.go | 12 ++++++++++++ pkg/config/config_test.go | 9 +++++++++ 2 files changed, 21 insertions(+) diff --git a/pkg/config/config.go b/pkg/config/config.go index 98bb18fc..93046cc6 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -1,6 +1,7 @@ package config import ( + "bytes" "crypto" "crypto/rsa" "crypto/x509" @@ -13,6 +14,7 @@ import ( "regexp" "strings" "time" + "unicode/utf8" "github.com/Azure/azure-sdk-for-go/sdk/azidentity" @@ -613,6 +615,9 @@ func (c *ServicePrincipalConfig) validate() error { } c.clientCertificatePEM = clientCertificatePEM } else { + if credentialLooksLikeCertificate(data) { + return fmt.Errorf("invalid azure.servicePrincipal.clientSecretFile: parse service principal client certificate file: %w", certificateErr) + } c.ClientSecret = strings.TrimRight(string(data), "\r\n") if c.ClientSecret == "" { return fmt.Errorf("invalid azure.servicePrincipal.clientSecretFile: service principal credential file is empty") @@ -645,6 +650,13 @@ func loadServicePrincipalCredentialFile(path string) ([]byte, error) { return data, nil } +func credentialLooksLikeCertificate(data []byte) bool { + return bytes.Contains(data, []byte("-----BEGIN CERTIFICATE-----")) || + bytes.Contains(data, []byte("-----BEGIN PRIVATE KEY-----")) || + bytes.Contains(data, []byte("-----BEGIN RSA PRIVATE KEY-----")) || + !utf8.Valid(data) +} + // LoadClientCertificate loads the service principal certificate and private key // from ClientSecretFile. func (c *ServicePrincipalConfig) LoadClientCertificate() ([]*x509.Certificate, crypto.PrivateKey, error) { diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 9a6d2389..05038990 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -2141,6 +2141,10 @@ func TestServicePrincipalCertificateFile(t *testing.T) { dir := t.TempDir() validFile := filepath.Join(dir, "client-certificate.pem") writeTestClientCertificate(t, validFile) + invalidFile := filepath.Join(dir, "invalid-certificate.pem") + if err := os.WriteFile(invalidFile, []byte("-----BEGIN CERTIFICATE-----\ninvalid\n-----END CERTIFICATE-----\n"), 0o600); err != nil { + t.Fatalf("os.WriteFile: %v", err) + } insecureFile := filepath.Join(dir, "insecure") writeTestClientCertificate(t, insecureFile) if err := os.Chmod(insecureFile, 0o644); err != nil { @@ -2161,6 +2165,11 @@ func TestServicePrincipalCertificateFile(t *testing.T) { config: &ServicePrincipalConfig{TenantID: "tenant", ClientID: "client", ClientSecret: "secret", ClientSecretFile: validFile}, wantErr: "only one of", }, + { + name: "rejects malformed certificate", + config: &ServicePrincipalConfig{TenantID: "tenant", ClientID: "client", ClientSecretFile: invalidFile}, + wantErr: "parse service principal client certificate file", + }, { name: "rejects insecure permissions", config: &ServicePrincipalConfig{TenantID: "tenant", ClientID: "client", ClientSecretFile: insecureFile}, From acf0055e698d5c37a6f199ad8ebef31655f227f0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Jul 2026 22:19:36 +0000 Subject: [PATCH 6/8] Pass certificate file path to kubelogin --- pkg/cmd/token/kubelogin/kubelogin.go | 29 +++++--- pkg/cmd/token/kubelogin/kubelogin_test.go | 80 +++++++++++++++++++++-- pkg/config/adapter.go | 16 +++-- pkg/config/adapter_test.go | 5 +- pkg/config/config.go | 14 ++-- 5 files changed, 117 insertions(+), 27 deletions(-) diff --git a/pkg/cmd/token/kubelogin/kubelogin.go b/pkg/cmd/token/kubelogin/kubelogin.go index 9de58e10..0cb405f2 100644 --- a/pkg/cmd/token/kubelogin/kubelogin.go +++ b/pkg/cmd/token/kubelogin/kubelogin.go @@ -2,12 +2,13 @@ package kubelogin import ( "context" - "encoding/base64" "encoding/json" "fmt" "io" "os" + "github.com/Azure/AKSFlexNode/pkg/config" + "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/Azure/kubelogin/pkg/token" "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -21,11 +22,11 @@ import ( ) const aksAADServerID = "6dae42f8-4368-4678-94ff-3960e28e3630" -const clientCertificateDataEnv = "AKS_FLEX_NODE_CLIENT_CERTIFICATE_DATA" var flagServerID string var flagPopEnabled bool var flagPopClaims string +var flagClientCertificateFile string var Command = &cobra.Command{ Use: "kubelogin", @@ -49,6 +50,10 @@ func init() { &flagPopClaims, "pop-claims", "", "Comma-separated list of key=value claims to include in the PoP token (e.g., 'u=cluster-resource-id').", ) + Command.Flags().StringVar( + &flagClientCertificateFile, "client-certificate-file", "", + "Path to the service principal client certificate file.", + ) } func run(ctx context.Context, out io.Writer) error { @@ -65,8 +70,8 @@ func run(ctx context.Context, out io.Writer) error { tokOpts.ServerID = flagServerID tokOpts.IsPoPTokenEnabled = flagPopEnabled tokOpts.PoPTokenClaims = flagPopClaims - if certificateData := os.Getenv(clientCertificateDataEnv); certificateData != "" { - certificatePath, cleanup, err := prepareClientCertificate(certificateData) + if flagClientCertificateFile != "" { + certificatePath, cleanup, err := prepareClientCertificate(flagClientCertificateFile) if err != nil { return err } @@ -86,17 +91,25 @@ func run(ctx context.Context, out io.Writer) error { return outputToken(out, ec, accessToken) } -func prepareClientCertificate(encodedData string) (string, func(), error) { - data, err := base64.StdEncoding.DecodeString(encodedData) +func prepareClientCertificate(certificateFile string) (string, func(), error) { + data, err := config.LoadServicePrincipalCredentialFile(certificateFile) + if err != nil { + return "", nil, fmt.Errorf("load client certificate file: %w", err) + } + certificates, privateKey, err := azidentity.ParseCertificates(data, nil) + if err != nil { + return "", nil, fmt.Errorf("parse client certificate file: %w", err) + } + normalizedPEM, err := config.MarshalClientCertificatePEM(certificates, privateKey) if err != nil { - return "", nil, fmt.Errorf("decode client certificate data: %w", err) + return "", nil, fmt.Errorf("normalize client certificate file: %w", err) } file, err := os.CreateTemp("", "aks-flex-node-client-certificate-*.pem") if err != nil { return "", nil, fmt.Errorf("create temporary client certificate: %w", err) } cleanup := func() { _ = os.Remove(file.Name()) } - if _, err := file.Write(data); err != nil { + if _, err := file.WriteString(normalizedPEM); err != nil { _ = file.Close() cleanup() return "", nil, fmt.Errorf("write temporary client certificate: %w", err) diff --git a/pkg/cmd/token/kubelogin/kubelogin_test.go b/pkg/cmd/token/kubelogin/kubelogin_test.go index 494e882e..021ddf27 100644 --- a/pkg/cmd/token/kubelogin/kubelogin_test.go +++ b/pkg/cmd/token/kubelogin/kubelogin_test.go @@ -1,16 +1,24 @@ package kubelogin import ( - "encoding/base64" + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" "os" + "path/filepath" "testing" + "time" ) func TestPrepareClientCertificate(t *testing.T) { t.Parallel() - const certificate = "certificate data" - path, cleanup, err := prepareClientCertificate(base64.StdEncoding.EncodeToString([]byte(certificate))) + certificateFile := filepath.Join(t.TempDir(), "client-certificate.pem") + writeTestClientCertificate(t, certificateFile) + path, cleanup, err := prepareClientCertificate(certificateFile) if err != nil { t.Fatalf("prepareClientCertificate() error = %v", err) } @@ -19,8 +27,11 @@ func TestPrepareClientCertificate(t *testing.T) { if err != nil { t.Fatalf("os.ReadFile: %v", err) } - if string(data) != certificate { - t.Fatalf("temporary certificate = %q, want %q", data, certificate) + if len(data) == 0 { + t.Fatal("temporary certificate should not be empty") + } + if string(data) == "" || !containsPEMCertificate(data) || !containsPEMPrivateKey(data) { + t.Fatalf("temporary certificate should contain normalized PEM certificate and private key, got %q", data) } info, err := os.Stat(path) if err != nil { @@ -35,3 +46,62 @@ func TestPrepareClientCertificate(t *testing.T) { t.Fatalf("temporary certificate still exists after cleanup: %v", err) } } + +func containsPEMCertificate(data []byte) bool { + for len(data) > 0 { + var block *pem.Block + block, data = pem.Decode(data) + if block == nil { + return false + } + if block.Type == "CERTIFICATE" { + return true + } + } + return false +} + +func containsPEMPrivateKey(data []byte) bool { + for len(data) > 0 { + var block *pem.Block + block, data = pem.Decode(data) + if block == nil { + return false + } + if block.Type == "PRIVATE KEY" { + return true + } + } + return false +} + +func writeTestClientCertificate(t *testing.T, path string) { + t.Helper() + + privateKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("rsa.GenerateKey: %v", err) + } + template := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "test-client"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + } + certificate, err := x509.CreateCertificate(rand.Reader, template, template, &privateKey.PublicKey, privateKey) + if err != nil { + t.Fatalf("x509.CreateCertificate: %v", err) + } + privateKeyData, err := x509.MarshalPKCS8PrivateKey(privateKey) + if err != nil { + t.Fatalf("x509.MarshalPKCS8PrivateKey: %v", err) + } + data := append( + pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certificate}), + pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: privateKeyData})..., + ) + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatalf("os.WriteFile: %v", err) + } +} diff --git a/pkg/config/adapter.go b/pkg/config/adapter.go index f94fec44..dfd302b3 100644 --- a/pkg/config/adapter.go +++ b/pkg/config/adapter.go @@ -2,7 +2,6 @@ package config import ( "context" - "encoding/base64" "fmt" "log/slog" @@ -19,8 +18,6 @@ const ( // aksAADServerID is the Azure AD server application ID for AKS. aksAADServerID = "6dae42f8-4368-4678-94ff-3960e28e3630" - - clientCertificateDataEnv = "AKS_FLEX_NODE_CLIENT_CERTIFICATE_DATA" ) // ToAgentConfig converts a FlexNode Config to the shared agent library's @@ -83,11 +80,15 @@ func ToAgentConfig(cfg *Config, machineName string) *agentconfig.AgentConfig { "AZURE_TENANT_ID": cfg.Azure.ServicePrincipal.TenantID, } if cfg.Azure.ServicePrincipal.clientCertificateData() != "" { - env[clientCertificateDataEnv] = base64.StdEncoding.EncodeToString([]byte(cfg.Azure.ServicePrincipal.clientCertificateData())) + ac.Kubelet.Auth.ExecCredential = buildExecCredential( + env, + "--client-certificate-file", + cfg.Azure.ServicePrincipal.ClientSecretFile, + ) } else { env["AAD_SERVICE_PRINCIPAL_CLIENT_SECRET"] = cfg.Azure.ServicePrincipal.ClientSecret + ac.Kubelet.Auth.ExecCredential = buildExecCredential(env) } - ac.Kubelet.Auth.ExecCredential = buildExecCredential(env) case cfg.IsMIConfigured(): env := map[string]string{ @@ -122,16 +123,17 @@ func ResolveMachineGoalState(ctx context.Context, log *slog.Logger, cfg *Config, // buildExecCredential creates an ExecConfig that invokes the aks-flex-node // binary as a credential plugin. The binary's `token kubelogin` subcommand // uses kubelogin to obtain an Azure AD token for the AKS API server. -func buildExecCredential(env map[string]string) *clientcmdapi.ExecConfig { +func buildExecCredential(env map[string]string, args ...string) *clientcmdapi.ExecConfig { execEnv := make([]clientcmdapi.ExecEnvVar, 0, len(env)) for k, v := range env { execEnv = append(execEnv, clientcmdapi.ExecEnvVar{Name: k, Value: v}) } + execArgs := append([]string{"token", "kubelogin", "--server-id", aksAADServerID}, args...) return &clientcmdapi.ExecConfig{ APIVersion: "client.authentication.k8s.io/v1", Command: flexNodeBinaryPath, - Args: []string{"token", "kubelogin", "--server-id", aksAADServerID}, + Args: execArgs, Env: execEnv, InteractiveMode: clientcmdapi.NeverExecInteractiveMode, ProvideClusterInfo: false, diff --git a/pkg/config/adapter_test.go b/pkg/config/adapter_test.go index 127b3595..510af5e4 100644 --- a/pkg/config/adapter_test.go +++ b/pkg/config/adapter_test.go @@ -3,6 +3,7 @@ package config import ( "os" "path/filepath" + "slices" "testing" "github.com/Azure/unbounded/pkg/agent/goalstates" @@ -194,8 +195,8 @@ func TestToAgentConfig_ServicePrincipalCertificateFile(t *testing.T) { for _, e := range exec.Env { envMap[e.Name] = e.Value } - if envMap[clientCertificateDataEnv] == "" { - t.Fatalf("%s should contain the certificate data", clientCertificateDataEnv) + if !slices.Equal(exec.Args, []string{"token", "kubelogin", "--server-id", aksAADServerID, "--client-certificate-file", certificateFile}) { + t.Fatalf("Args=%v, want kubelogin client-certificate-file args", exec.Args) } if _, ok := envMap["AAD_SERVICE_PRINCIPAL_CLIENT_SECRET"]; ok { t.Fatal("AAD_SERVICE_PRINCIPAL_CLIENT_SECRET should not be set for certificate auth") diff --git a/pkg/config/config.go b/pkg/config/config.go index b9be066c..7958426b 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -619,13 +619,13 @@ func (c *ServicePrincipalConfig) validate() error { return fmt.Errorf("invalid azure.servicePrincipal.clientSecretFile: resolve absolute path: %w", err) } c.ClientSecretFile = absolutePath - data, err := loadServicePrincipalCredentialFile(c.ClientSecretFile) + data, err := LoadServicePrincipalCredentialFile(c.ClientSecretFile) if err != nil { return fmt.Errorf("invalid azure.servicePrincipal.clientSecretFile: %w", err) } certificates, privateKey, certificateErr := azidentity.ParseCertificates(data, nil) if certificateErr == nil { - clientCertificatePEM, err := marshalClientCertificatePEM(certificates, privateKey) + clientCertificatePEM, err := MarshalClientCertificatePEM(certificates, privateKey) if err != nil { return fmt.Errorf("invalid azure.servicePrincipal.clientSecretFile: %w", err) } @@ -644,7 +644,9 @@ func (c *ServicePrincipalConfig) validate() error { return nil } -func loadServicePrincipalCredentialFile(path string) ([]byte, error) { +// LoadServicePrincipalCredentialFile reads a service principal credential file +// after verifying it is a protected regular file. +func LoadServicePrincipalCredentialFile(path string) ([]byte, error) { cleanPath := filepath.Clean(path) info, err := os.Lstat(cleanPath) if err != nil { @@ -679,7 +681,7 @@ func (c *ServicePrincipalConfig) LoadClientCertificate() ([]*x509.Certificate, c data := []byte(c.clientCertificatePEM) if len(data) == 0 { var err error - data, err = loadServicePrincipalCredentialFile(c.ClientSecretFile) + data, err = LoadServicePrincipalCredentialFile(c.ClientSecretFile) if err != nil { return nil, nil, err } @@ -691,7 +693,9 @@ func (c *ServicePrincipalConfig) LoadClientCertificate() ([]*x509.Certificate, c return certificates, privateKey, nil } -func marshalClientCertificatePEM(certificates []*x509.Certificate, privateKey crypto.PrivateKey) (string, error) { +// MarshalClientCertificatePEM converts a client certificate chain and private +// key into PKCS#8 PEM form for consumers that require PEM files. +func MarshalClientCertificatePEM(certificates []*x509.Certificate, privateKey crypto.PrivateKey) (string, error) { if _, ok := privateKey.(*rsa.PrivateKey); !ok { return "", fmt.Errorf("service principal client certificate private key must be RSA") } From 6502b9a85c2a5ea3f78ed0e0618c15fc3dedee64 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Jul 2026 22:50:25 +0000 Subject: [PATCH 7/8] Pass certificate path directly to kubelogin --- docs/usages/configuration.md | 2 +- docs/usages/joining-nodes.md | 2 +- pkg/cmd/token/kubelogin/kubelogin.go | 38 ++-------- pkg/cmd/token/kubelogin/kubelogin_test.go | 86 +++++++++-------------- 4 files changed, 42 insertions(+), 86 deletions(-) diff --git a/docs/usages/configuration.md b/docs/usages/configuration.md index 9067e74c..4509c32f 100644 --- a/docs/usages/configuration.md +++ b/docs/usages/configuration.md @@ -82,7 +82,7 @@ At least one join or Azure authentication method must be configured. `azure.boot | `azure.servicePrincipal.tenantId` | string | Microsoft Entra tenant ID for the service principal. | `70a036f6-8e4d-4615-bad6-149c02e7720d` | | `azure.servicePrincipal.clientId` | string | Application client ID. | `00000000-0000-0000-0000-000000000000` | | `azure.servicePrincipal.clientSecret` | string | Application client secret. Mutually exclusive with `clientSecretFile`. | `` | -| `azure.servicePrincipal.clientSecretFile` | string | Path to a protected regular file (no group/other access; e.g., 0600) containing either the application client secret or a PEM/unencrypted PFX certificate and private key. | `/run/credentials/aks-flex-node-sp` | +| `azure.servicePrincipal.clientSecretFile` | string | Path to a protected regular file (no group/other access; e.g., 0600) containing either the application client secret or a PEM/unencrypted PFX certificate and private key. PFX certificate files must use a `.pfx` suffix. | `/run/credentials/aks-flex-node-sp` | ## Agent diff --git a/docs/usages/joining-nodes.md b/docs/usages/joining-nodes.md index 5ff58a4c..f0781c5b 100644 --- a/docs/usages/joining-nodes.md +++ b/docs/usages/joining-nodes.md @@ -107,7 +107,7 @@ Minimal config shape: } ``` -The credential file contains either the client secret or a PEM/unencrypted PFX application certificate and private key; the agent detects the credential type from its contents. The file must be a non-empty regular file with no group/world access (for example, mode 0600). Only one of `clientSecret` or `clientSecretFile` can be configured. +The credential file contains either the client secret or a PEM/unencrypted PFX application certificate and private key; the agent detects the credential type from its contents. PFX certificate files must use a `.pfx` suffix. The file must be a non-empty regular file with no group/world access (for example, mode 0600). Only one of `clientSecret` or `clientSecretFile` can be configured. ## Authentication Mode Selection diff --git a/pkg/cmd/token/kubelogin/kubelogin.go b/pkg/cmd/token/kubelogin/kubelogin.go index 0cb405f2..d64502c7 100644 --- a/pkg/cmd/token/kubelogin/kubelogin.go +++ b/pkg/cmd/token/kubelogin/kubelogin.go @@ -8,7 +8,6 @@ import ( "os" "github.com/Azure/AKSFlexNode/pkg/config" - "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/Azure/kubelogin/pkg/token" "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -71,12 +70,10 @@ func run(ctx context.Context, out io.Writer) error { tokOpts.IsPoPTokenEnabled = flagPopEnabled tokOpts.PoPTokenClaims = flagPopClaims if flagClientCertificateFile != "" { - certificatePath, cleanup, err := prepareClientCertificate(flagClientCertificateFile) - if err != nil { + if err := validateClientCertificateFile(flagClientCertificateFile); err != nil { return err } - defer cleanup() - tokOpts.ClientCert = certificatePath + tokOpts.ClientCert = flagClientCertificateFile } // TODO: logging to show login details provider, err := token.GetTokenProvider(tokOpts) @@ -91,34 +88,11 @@ func run(ctx context.Context, out io.Writer) error { return outputToken(out, ec, accessToken) } -func prepareClientCertificate(certificateFile string) (string, func(), error) { - data, err := config.LoadServicePrincipalCredentialFile(certificateFile) - if err != nil { - return "", nil, fmt.Errorf("load client certificate file: %w", err) - } - certificates, privateKey, err := azidentity.ParseCertificates(data, nil) - if err != nil { - return "", nil, fmt.Errorf("parse client certificate file: %w", err) - } - normalizedPEM, err := config.MarshalClientCertificatePEM(certificates, privateKey) - if err != nil { - return "", nil, fmt.Errorf("normalize client certificate file: %w", err) - } - file, err := os.CreateTemp("", "aks-flex-node-client-certificate-*.pem") - if err != nil { - return "", nil, fmt.Errorf("create temporary client certificate: %w", err) - } - cleanup := func() { _ = os.Remove(file.Name()) } - if _, err := file.WriteString(normalizedPEM); err != nil { - _ = file.Close() - cleanup() - return "", nil, fmt.Errorf("write temporary client certificate: %w", err) - } - if err := file.Close(); err != nil { - cleanup() - return "", nil, fmt.Errorf("close temporary client certificate: %w", err) +func validateClientCertificateFile(certificateFile string) error { + if _, err := config.LoadServicePrincipalCredentialFile(certificateFile); err != nil { + return fmt.Errorf("load client certificate file: %w", err) } - return file.Name(), cleanup, nil + return nil } const execInfoEnv = "KUBERNETES_EXEC_INFO" diff --git a/pkg/cmd/token/kubelogin/kubelogin_test.go b/pkg/cmd/token/kubelogin/kubelogin_test.go index 021ddf27..8edaf77f 100644 --- a/pkg/cmd/token/kubelogin/kubelogin_test.go +++ b/pkg/cmd/token/kubelogin/kubelogin_test.go @@ -9,70 +9,52 @@ import ( "math/big" "os" "path/filepath" + "strings" "testing" "time" ) -func TestPrepareClientCertificate(t *testing.T) { +func TestValidateClientCertificateFile(t *testing.T) { t.Parallel() - certificateFile := filepath.Join(t.TempDir(), "client-certificate.pem") - writeTestClientCertificate(t, certificateFile) - path, cleanup, err := prepareClientCertificate(certificateFile) - if err != nil { - t.Fatalf("prepareClientCertificate() error = %v", err) - } - - data, err := os.ReadFile(path) - if err != nil { - t.Fatalf("os.ReadFile: %v", err) - } - if len(data) == 0 { - t.Fatal("temporary certificate should not be empty") - } - if string(data) == "" || !containsPEMCertificate(data) || !containsPEMPrivateKey(data) { - t.Fatalf("temporary certificate should contain normalized PEM certificate and private key, got %q", data) - } - info, err := os.Stat(path) - if err != nil { - t.Fatalf("os.Stat: %v", err) - } - if info.Mode().Perm() != 0o600 { - t.Fatalf("temporary certificate permissions = %o, want 600", info.Mode().Perm()) + dir := t.TempDir() + validFile := filepath.Join(dir, "client-certificate.pem") + writeTestClientCertificate(t, validFile) + insecureFile := filepath.Join(dir, "client-certificate-insecure.pem") + writeTestClientCertificate(t, insecureFile) + if err := os.Chmod(insecureFile, 0o644); err != nil { + t.Fatalf("os.Chmod: %v", err) } - cleanup() - if _, err := os.Stat(path); !os.IsNotExist(err) { - t.Fatalf("temporary certificate still exists after cleanup: %v", err) + tests := []struct { + name string + path string + wantErr string + }{ + {name: "valid protected certificate file", path: validFile}, + { + name: "rejects insecure permissions", + path: insecureFile, + wantErr: "must not be accessible by group or other users", + }, } -} -func containsPEMCertificate(data []byte) bool { - for len(data) > 0 { - var block *pem.Block - block, data = pem.Decode(data) - if block == nil { - return false - } - if block.Type == "CERTIFICATE" { - return true - } - } - return false -} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() -func containsPEMPrivateKey(data []byte) bool { - for len(data) > 0 { - var block *pem.Block - block, data = pem.Decode(data) - if block == nil { - return false - } - if block.Type == "PRIVATE KEY" { - return true - } + err := validateClientCertificateFile(tt.path) + if tt.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("validateClientCertificateFile() error = %v, want %q", err, tt.wantErr) + } + return + } + if err != nil { + t.Fatalf("validateClientCertificateFile() error = %v", err) + } + }) } - return false } func writeTestClientCertificate(t *testing.T, path string) { From 4e995e134ebbb1b03d1e3581d3f2373080b9944a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Jul 2026 23:05:13 +0000 Subject: [PATCH 8/8] fix cert file validation for kubelogin and config --- pkg/cmd/token/kubelogin/kubelogin.go | 4 +- pkg/cmd/token/kubelogin/kubelogin_test.go | 9 +++++ pkg/config/config.go | 33 +++++++++++++++++ pkg/config/config_test.go | 45 +++++++++++++++++++++++ 4 files changed, 89 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/token/kubelogin/kubelogin.go b/pkg/cmd/token/kubelogin/kubelogin.go index d64502c7..9495a88b 100644 --- a/pkg/cmd/token/kubelogin/kubelogin.go +++ b/pkg/cmd/token/kubelogin/kubelogin.go @@ -89,8 +89,8 @@ func run(ctx context.Context, out io.Writer) error { } func validateClientCertificateFile(certificateFile string) error { - if _, err := config.LoadServicePrincipalCredentialFile(certificateFile); err != nil { - return fmt.Errorf("load client certificate file: %w", err) + if err := config.ValidateServicePrincipalCertificateFile(certificateFile); err != nil { + return fmt.Errorf("validate client certificate file: %w", err) } return nil } diff --git a/pkg/cmd/token/kubelogin/kubelogin_test.go b/pkg/cmd/token/kubelogin/kubelogin_test.go index 8edaf77f..8d531e4a 100644 --- a/pkg/cmd/token/kubelogin/kubelogin_test.go +++ b/pkg/cmd/token/kubelogin/kubelogin_test.go @@ -25,6 +25,10 @@ func TestValidateClientCertificateFile(t *testing.T) { if err := os.Chmod(insecureFile, 0o644); err != nil { t.Fatalf("os.Chmod: %v", err) } + invalidFile := filepath.Join(dir, "client-certificate-invalid.pem") + if err := os.WriteFile(invalidFile, []byte("not-a-certificate"), 0o600); err != nil { + t.Fatalf("os.WriteFile: %v", err) + } tests := []struct { name string @@ -37,6 +41,11 @@ func TestValidateClientCertificateFile(t *testing.T) { path: insecureFile, wantErr: "must not be accessible by group or other users", }, + { + name: "rejects malformed certificate file contents", + path: invalidFile, + wantErr: "parse service principal client certificate file", + }, } for _, tt := range tests { diff --git a/pkg/config/config.go b/pkg/config/config.go index 7958426b..24d3574b 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -625,6 +625,9 @@ func (c *ServicePrincipalConfig) validate() error { } certificates, privateKey, certificateErr := azidentity.ParseCertificates(data, nil) if certificateErr == nil { + if err := validatePKCS12FileSuffix(c.ClientSecretFile, data); err != nil { + return fmt.Errorf("invalid azure.servicePrincipal.clientSecretFile: %w", err) + } clientCertificatePEM, err := MarshalClientCertificatePEM(certificates, privateKey) if err != nil { return fmt.Errorf("invalid azure.servicePrincipal.clientSecretFile: %w", err) @@ -644,6 +647,26 @@ func (c *ServicePrincipalConfig) validate() error { return nil } +// ValidateServicePrincipalCertificateFile verifies that a certificate credential +// file is protected, well-formed, and compatible with service principal auth. +func ValidateServicePrincipalCertificateFile(path string) error { + data, err := LoadServicePrincipalCredentialFile(path) + if err != nil { + return err + } + if err := validatePKCS12FileSuffix(path, data); err != nil { + return err + } + certificates, privateKey, err := azidentity.ParseCertificates(data, nil) + if err != nil { + return fmt.Errorf("parse service principal client certificate file: %w", err) + } + if _, err := MarshalClientCertificatePEM(certificates, privateKey); err != nil { + return err + } + return nil +} + // LoadServicePrincipalCredentialFile reads a service principal credential file // after verifying it is a protected regular file. func LoadServicePrincipalCredentialFile(path string) ([]byte, error) { @@ -668,6 +691,16 @@ func LoadServicePrincipalCredentialFile(path string) ([]byte, error) { return data, nil } +func validatePKCS12FileSuffix(path string, data []byte) error { + if utf8.Valid(data) { + return nil + } + if !strings.EqualFold(filepath.Ext(path), ".pfx") { + return fmt.Errorf("PFX service principal credential files must use a .pfx suffix") + } + return nil +} + func credentialLooksLikeCertificate(data []byte) bool { return bytes.Contains(data, []byte("-----BEGIN CERTIFICATE-----")) || bytes.Contains(data, []byte("-----BEGIN PRIVATE KEY-----")) || diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index fa4f6a7b..7d92dd8f 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -2271,6 +2271,51 @@ func TestServicePrincipalCertificateFile(t *testing.T) { } } +func TestValidatePKCS12FileSuffix(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + path string + data []byte + wantErr string + }{ + { + name: "allows utf8 data with non-pfx suffix", + path: "client-secret", + data: []byte("client-secret"), + }, + { + name: "allows binary data with pfx suffix", + path: "client-certificate.pfx", + data: []byte{0xff, 0x00, 0x01}, + }, + { + name: "rejects binary data without pfx suffix", + path: "client-certificate", + data: []byte{0xff, 0x00, 0x01}, + wantErr: "must use a .pfx suffix", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := validatePKCS12FileSuffix(tt.path, tt.data) + if tt.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("validatePKCS12FileSuffix() error = %v, want %q", err, tt.wantErr) + } + return + } + if err != nil { + t.Fatalf("validatePKCS12FileSuffix() error = %v", err) + } + }) + } +} + func writeTestClientCertificate(t *testing.T, path string) { t.Helper()