From a3d17c46d49f28b31f0629e1ec6aa4bfc6ad4e31 Mon Sep 17 00:00:00 2001 From: Roman Bednar Date: Tue, 23 May 2023 10:39:36 +0200 Subject: [PATCH 1/3] prevent using empty strings from env --- cmd/azure-config-credentials-injector/main.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/cmd/azure-config-credentials-injector/main.go b/cmd/azure-config-credentials-injector/main.go index 20371b5f3..e303bf363 100644 --- a/cmd/azure-config-credentials-injector/main.go +++ b/cmd/azure-config-credentials-injector/main.go @@ -52,12 +52,12 @@ func mergeCloudConfig(_ *cobra.Command, args []string) error { return err } - azureClientId, found := os.LookupEnv(clientIDEnvKey) + azureClientId, found := mustLookupEnvValue(clientIDEnvKey) if !found { return fmt.Errorf("%s env variable should be set up", clientIDEnvKey) } - azureClientSecret, found := os.LookupEnv(clientSecretEnvKey) + azureClientSecret, secretFound = mustLookupEnvValue(clientSecretEnvKey) if !found { return fmt.Errorf("%s env variable should be set up", clientSecretEnvKey) } @@ -120,3 +120,11 @@ func writeCloudConfig(path string, preparedConfig []byte) error { } return nil } + +func mustLookupEnvValue(key string) (string, bool) { + value, found := os.LookupEnv(key) + if !found || len(value) == 0 { + return "", false + } + return value, true +} From cb3a63fa46a853ee83625e06e11439f7d9674062 Mon Sep 17 00:00:00 2001 From: Roman Bednar Date: Mon, 5 Jun 2023 12:15:15 +0200 Subject: [PATCH 2/3] add support for workload identity --- cmd/azure-config-credentials-injector/main.go | 76 +++++++++++++++++-- 1 file changed, 69 insertions(+), 7 deletions(-) diff --git a/cmd/azure-config-credentials-injector/main.go b/cmd/azure-config-credentials-injector/main.go index e303bf363..c23c8d0e6 100644 --- a/cmd/azure-config-credentials-injector/main.go +++ b/cmd/azure-config-credentials-injector/main.go @@ -11,12 +11,18 @@ import ( ) const ( - clientIDEnvKey = "AZURE_CLIENT_ID" - clientSecretEnvKey = "AZURE_CLIENT_SECRET" + clientIDEnvKey = "AZURE_CLIENT_ID" + clientSecretEnvKey = "AZURE_CLIENT_SECRET" + tenantIDEnvKey = "AZURE_TENANT_ID" + federatedTokenEnvKey = "AZURE_FEDERATED_TOKEN_FILE" clientIDCloudConfigKey = "aadClientId" clientSecretCloudConfigKey = "aadClientSecret" useManagedIdentityExtensionConfigKey = "useManagedIdentityExtension" + + tenantIdConfigKey = "tenantId" + aadFederatedTokenFileConfigKey = "aadFederatedTokenFile" + useFederatedWorkloadIdentityExtensionConfigKey = "useFederatedWorkloadIdentityExtension" ) var ( @@ -29,6 +35,7 @@ var ( injectorOpts struct { cloudConfigFilePath string outputFilePath string + enableWorkloadIdentity string disableIdentityExtensionAuth bool } ) @@ -39,6 +46,7 @@ func init() { injectorCmd.PersistentFlags().StringVar(&injectorOpts.cloudConfigFilePath, "cloud-config-file-path", "/tmp/cloud-config/cloud.conf", "Location of the original cloud config file.") injectorCmd.PersistentFlags().StringVar(&injectorOpts.outputFilePath, "output-file-path", "/tmp/merged-cloud-config/cloud.conf", "Location of the generated cloud config file with injected credentials.") injectorCmd.PersistentFlags().BoolVar(&injectorOpts.disableIdentityExtensionAuth, "disable-identity-extension-auth", false, "Disable managed identity authentication, if it's set in cloudConfig.") + injectorCmd.PersistentFlags().StringVar(&injectorOpts.enableWorkloadIdentity, "enable-azure-workload-identity", "false", "Enable workload identity authentication.") } func main() { @@ -48,6 +56,15 @@ func main() { } func mergeCloudConfig(_ *cobra.Command, args []string) error { + var ( + azureClientId string + azureClientSecret string + tenantId string + federatedTokenFile string + secretFound bool + err error + ) + if _, err := os.Stat(injectorOpts.cloudConfigFilePath); os.IsNotExist(err) { return err } @@ -58,8 +75,20 @@ func mergeCloudConfig(_ *cobra.Command, args []string) error { } azureClientSecret, secretFound = mustLookupEnvValue(clientSecretEnvKey) - if !found { - return fmt.Errorf("%s env variable should be set up", clientSecretEnvKey) + + // Ensure there is only one authentication method. + if injectorOpts.enableWorkloadIdentity == "true" { + tenantId, federatedTokenFile, err = lookupWorkloadIdentityEnv(tenantIDEnvKey, federatedTokenEnvKey) + if err != nil { + return fmt.Errorf("workload identity method failed: %v", err) + } + if secretFound { + return fmt.Errorf("%s env variable is set while workload identity is enabled, this should never happen.\nPlease consider reporting a bug: https://issues.redhat.com", clientSecretEnvKey) + } + } else { + if !secretFound { + return fmt.Errorf("%s env variable should be set up", clientSecretEnvKey) + } } cloudConfig, err := readCloudConfig(injectorOpts.cloudConfigFilePath) @@ -67,7 +96,7 @@ func mergeCloudConfig(_ *cobra.Command, args []string) error { return fmt.Errorf("couldn't read cloud config from file: %w", err) } - preparedCloudConfig, err := prepareCloudConfig(cloudConfig, azureClientId, azureClientSecret) + preparedCloudConfig, err := prepareCloudConfig(cloudConfig, azureClientId, azureClientSecret, tenantId, federatedTokenFile) if err != nil { return fmt.Errorf("couldn't prepare cloud config: %w", err) } @@ -92,9 +121,9 @@ func readCloudConfig(path string) (map[string]interface{}, error) { return data, nil } -func prepareCloudConfig(cloudConfig map[string]interface{}, clientId string, clientSecret string) ([]byte, error) { +func prepareCloudConfig(cloudConfig map[string]interface{}, clientId, clientSecret, tenantId, federatedTokenFile string) ([]byte, error) { cloudConfig[clientIDCloudConfigKey] = clientId - cloudConfig[clientSecretCloudConfigKey] = clientSecret + if value, found := cloudConfig[useManagedIdentityExtensionConfigKey]; found { if injectorOpts.disableIdentityExtensionAuth { klog.Infof("%s cleared\n", useManagedIdentityExtensionConfigKey) @@ -106,6 +135,15 @@ func prepareCloudConfig(cloudConfig map[string]interface{}, clientId string, cli } } + if len(tenantId) != 0 && len(federatedTokenFile) != 0 { + cloudConfig[tenantIdConfigKey] = tenantId + cloudConfig[aadFederatedTokenFileConfigKey] = federatedTokenFile + cloudConfig[useFederatedWorkloadIdentityExtensionConfigKey] = true + } else { + klog.V(4).Info("%s env variable is set, client secret authentication will be used", clientSecretEnvKey) + cloudConfig[clientSecretCloudConfigKey] = clientSecret + } + marshalled, err := json.Marshal(cloudConfig) if err != nil { return nil, err @@ -121,6 +159,30 @@ func writeCloudConfig(path string, preparedConfig []byte) error { return nil } +// lookupWorkloadIdentityEnv loads tenantID and federatedTokenFile values from environment, which are both required for +// workload identity. Return error if any or both values are missing. +func lookupWorkloadIdentityEnv(tenantEnvKey, tokenEnvKey string) (tenantId, federatedTokenFile string, err error) { + tenantId, tenantIdFound := mustLookupEnvValue(tenantEnvKey) + federatedTokenFile, federatedTokenFileFound := mustLookupEnvValue(tokenEnvKey) + klog.V(4).Infof("env vars required for workload identity auth are set to: %v=%v and %v=%v", tenantEnvKey, tenantId, tokenEnvKey, federatedTokenFile) + + if !tenantIdFound && !federatedTokenFileFound { + err = fmt.Errorf("%v and %v environment variables not found or empty", tenantEnvKey, tokenEnvKey) + return + } + + if !tenantIdFound { + err = fmt.Errorf("%v environment variable not found or empty", tenantEnvKey) + return + } + + if !federatedTokenFileFound { + err = fmt.Errorf("%v environment variable not found or empty", tokenEnvKey) + } + + return +} + func mustLookupEnvValue(key string) (string, bool) { value, found := os.LookupEnv(key) if !found || len(value) == 0 { From 892fe8a21f45eadd3f11cb3234aa54285c31af71 Mon Sep 17 00:00:00 2001 From: Roman Bednar Date: Mon, 5 Jun 2023 12:15:31 +0200 Subject: [PATCH 3/3] add unit tests for workload identity --- .../credentials_injector_test.go | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/cmd/azure-config-credentials-injector/credentials_injector_test.go b/cmd/azure-config-credentials-injector/credentials_injector_test.go index 5f7f3856f..a53114e0e 100644 --- a/cmd/azure-config-credentials-injector/credentials_injector_test.go +++ b/cmd/azure-config-credentials-injector/credentials_injector_test.go @@ -143,6 +143,38 @@ func Test_mergeCloudConfig(t *testing.T) { fileContent: "{\"aadClientSecret\":\"fizz\",\"aadClientId\":\"baz\",\"useManagedIdentityExtension\":\"true\"}", expectedContent: "{\"aadClientId\":\"foo\",\"aadClientSecret\":\"bar\",\"useManagedIdentityExtension\":false}", }, + { + name: "all ok, use workload identity while client secret is not present", + args: []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name(), "--disable-identity-extension-auth", "--enable-azure-workload-identity=true"}, + envVars: map[string]string{"AZURE_TENANT_ID": "bar", "AZURE_CLIENT_ID": "buzz", "AZURE_FEDERATED_TOKEN_FILE": "baz"}, + fileContent: "{\"tenantId\":\"foo\",\"aadClientId\":\"fizz\"}", + expectedContent: "{\"aadClientId\":\"buzz\",\"aadFederatedTokenFile\":\"baz\",\"tenantId\":\"bar\",\"useFederatedWorkloadIdentityExtension\":true}", + }, + { + name: "all ok, use workload identity while managed identity is not explicitly disabled", + args: []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name(), "--enable-azure-workload-identity=true"}, + envVars: map[string]string{"AZURE_TENANT_ID": "bar", "AZURE_CLIENT_ID": "buzz", "AZURE_FEDERATED_TOKEN_FILE": "baz"}, + fileContent: "{\"tenantId\":\"foo\",\"aadClientId\":\"fizz\"}", + expectedContent: "{\"aadClientId\":\"buzz\",\"aadFederatedTokenFile\":\"baz\",\"tenantId\":\"bar\",\"useFederatedWorkloadIdentityExtension\":true}", + }, + { + name: "should fail, client secret is present while workload identity is enabled", + args: []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name(), "--disable-identity-extension-auth", "--enable-azure-workload-identity=true"}, + envVars: map[string]string{"AZURE_TENANT_ID": "baz", "AZURE_CLIENT_ID": "foo", "AZURE_CLIENT_SECRET": "bar", "AZURE_FEDERATED_TOKEN_FILE": "baz"}, + expectedErrMsg: "AZURE_CLIENT_SECRET env variable is set while workload identity is enabled, this should never happen.\nPlease consider reporting a bug: https://issues.redhat.com", + }, + { + name: "should fail, workload identity can't be enabled because tenant id missing", + args: []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name(), "--disable-identity-extension-auth", "--enable-azure-workload-identity=true"}, + envVars: map[string]string{"AZURE_CLIENT_ID": "buzz", "AZURE_FEDERATED_TOKEN_FILE": "baz"}, + expectedErrMsg: "workload identity method failed: AZURE_TENANT_ID environment variable not found or empty", + }, + { + name: "should fail, workload identity can't be enabled because federated token missing", + args: []string{"--cloud-config-file-path", inputFile.Name(), "--output-file-path", outputFile.Name(), "--disable-identity-extension-auth", "--enable-azure-workload-identity=true"}, + envVars: map[string]string{"AZURE_TENANT_ID": "bar", "AZURE_CLIENT_ID": "buzz"}, + expectedErrMsg: "workload identity method failed: AZURE_FEDERATED_TOKEN_FILE environment variable not found or empty", + }, } for _, tc := range testCases {