Skip to content

CCO-324,CCO-325: add support for workload identity #245

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 3 commits into from
Jun 19, 2023
Merged

CCO-324,CCO-325: add support for workload identity #245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a3d17c4
prevent using empty strings from env
RomanBednar May 23, 2023
cb3a63f
add support for workload identity
RomanBednar Jun 5, 2023
892fe8a
add unit tests for workload identity
RomanBednar Jun 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions cmd/azure-config-credentials-injector/credentials_injector_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
88 changes: 79 additions & 9 deletions cmd/azure-config-credentials-injector/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -29,6 +35,7 @@ var (
injectorOpts struct {
cloudConfigFilePath string
outputFilePath string
enableWorkloadIdentity string
disableIdentityExtensionAuth bool
}
)
Expand All @@ -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() {
Expand All @@ -48,26 +56,47 @@ 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
}

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)
if !found {
return fmt.Errorf("%s env variable should be set up", clientSecretEnvKey)
azureClientSecret, secretFound = mustLookupEnvValue(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)
if err != nil {
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)
}
Expand All @@ -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)
Expand All @@ -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
Copy link
Contributor

@elmiko elmiko May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to check len(clientSecret) > 0 ?

Copy link
Contributor Author

@RomanBednar RomanBednar May 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'm not sure if we want to pass through "" - I don't think CCO should let that through. But it does make sense to check for this. But if we do that with secret we should do that with all other values to be consistent. See my latest commit.

}

marshalled, err := json.Marshal(cloudConfig)
if err != nil {
return nil, err
Expand All @@ -120,3 +158,35 @@ 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 {
return "", false
}
return value, true
}