From a6957bb23e8a750d7b6bee901c2a80051dad7d07 Mon Sep 17 00:00:00 2001 From: Keegan Witt Date: Tue, 28 Nov 2023 18:40:43 -0500 Subject: [PATCH 01/17] Remove KMS requiring metadata files (closes #4375) Signed-off-by: Keegan Witt --- doc/plugin_server_keymanager_aws_kms.md | 15 ++++++++------- ...lugin_server_keymanager_azure_key_vault.md | 19 ++++++++++--------- doc/plugin_server_keymanager_gcp_kms.md | 13 +++++++------ pkg/server/plugin/keymanager/awskms/awskms.go | 18 +++++++++++------- .../plugin/keymanager/awskms/awskms_test.go | 4 ++-- .../azurekeyvault/azure_key_vault.go | 16 ++++++++++------ .../azurekeyvault/azure_key_vault_test.go | 4 ++-- pkg/server/plugin/keymanager/gcpkms/gcpkms.go | 19 +++++++++++++------ .../plugin/keymanager/gcpkms/gcpkms_test.go | 2 +- 9 files changed, 64 insertions(+), 46 deletions(-) diff --git a/doc/plugin_server_keymanager_aws_kms.md b/doc/plugin_server_keymanager_aws_kms.md index d76086e432..6b44024a45 100644 --- a/doc/plugin_server_keymanager_aws_kms.md +++ b/doc/plugin_server_keymanager_aws_kms.md @@ -6,13 +6,14 @@ The `aws_kms` key manager plugin leverages the AWS Key Management Service (KMS) The plugin accepts the following configuration options: -| Key | Type | Required | Description | Default | -|-------------------|--------|---------------------------------------|-------------------------------------------------------------------------------|---------------------------------------------------------| -| access_key_id | string | see [AWS KMS Access](#aws-kms-access) | The Access Key Id used to authenticate to KMS | Value of the AWS_ACCESS_KEY_ID environment variable | -| secret_access_key | string | see [AWS KMS Access](#aws-kms-access) | The Secret Access Key used to authenticate to KMS | Value of the AWS_SECRET_ACCESS_KEY environment variable | -| region | string | yes | The region where the keys will be stored | | -| key_metadata_file | string | yes | A file path location where information about generated keys will be persisted | | -| key_policy_file | string | no | A file path location to a custom key policy in JSON format | "" | +| Key | Type | Required | Description | Default | +|-------------------|--------|---------------------------------------|-----------------------------------------------------------------------------------------|---------------------------------------------------------| +| access_key_id | string | see [AWS KMS Access](#aws-kms-access) | The Access Key Id used to authenticate to KMS | Value of the AWS_ACCESS_KEY_ID environment variable | +| secret_access_key | string | see [AWS KMS Access](#aws-kms-access) | The Secret Access Key used to authenticate to KMS | Value of the AWS_SECRET_ACCESS_KEY environment variable | +| region | string | yes | The region where the keys will be stored | | +| key_metadata_file | string | yes | A file path location where information about generated keys will be persisted | | +| key_metadata | string | yes | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | | +| key_policy_file | string | no | A file path location to a custom key policy in JSON format | "" | ### Alias and Key Management diff --git a/doc/plugin_server_keymanager_azure_key_vault.md b/doc/plugin_server_keymanager_azure_key_vault.md index 464e03b488..23be12fe1e 100644 --- a/doc/plugin_server_keymanager_azure_key_vault.md +++ b/doc/plugin_server_keymanager_azure_key_vault.md @@ -10,15 +10,16 @@ SPIRE. The plugin accepts the following configuration options: -| Key | Type | Required | Description | Default | -|-------------------|---------|---------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------|---------| -| key_metadata_file | string | yes | A file path location where key metadata used by the plugin will be persisted. See "[Management of keys](#management-of-keys)" for more information. | "" | -| key_vault_uri | string | Yes | The Key Vault URI where the keys managed by this plugin reside. | "" | -| use_msi | boolean | [Deprecated](#authenticating-to-azure) | Whether or not to use MSI to authenticate to Azure Key Vault. | false | -| subscription_id | string | [Optional](#authenticating-to-azure) | The subscription id. | "" | -| app_id | string | [Optional](#authenticating-to-azure) | The application id. | "" | -| app_secret | string | [Optional](#authenticating-to-azure) | The application secret. | "" | -| tenant_id | string | [Optional](#authenticating-to-azure) | The tenant id. | "" | +| Key | Type | Required | Description | Default | +|-------------------|---------|----------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------|---------| +| key_metadata_file | string | yes | A file path location where key metadata used by the plugin will be persisted. See "[Management of keys](#management-of-keys)" for more information. | "" | +| key_metadata | string | yes | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | "" | +| key_vault_uri | string | Yes | The Key Vault URI where the keys managed by this plugin reside. | "" | +| use_msi | boolean | [Deprecated](#authenticating-to-azure) | Whether or not to use MSI to authenticate to Azure Key Vault. | false | +| subscription_id | string | [Optional](#authenticating-to-azure) | The subscription id. | "" | +| app_id | string | [Optional](#authenticating-to-azure) | The application id. | "" | +| app_secret | string | [Optional](#authenticating-to-azure) | The application secret. | "" | +| tenant_id | string | [Optional](#authenticating-to-azure) | The tenant id. | "" | ### Authenticating to Azure diff --git a/doc/plugin_server_keymanager_gcp_kms.md b/doc/plugin_server_keymanager_gcp_kms.md index da2e9fa673..fd64c2be7f 100644 --- a/doc/plugin_server_keymanager_gcp_kms.md +++ b/doc/plugin_server_keymanager_gcp_kms.md @@ -10,12 +10,13 @@ SPIRE. The plugin accepts the following configuration options: -| Key | Type | Required | Description | Default | -| --- | ---- | -------- | ----------- | ------- | -| key_policy_file | string | no | A file path location to a custom [IAM Policy (v3)](https://cloud.google.com/pubsub/docs/reference/rpc/google.iam.v1#google.iam.v1.Policy) in JSON format to be attached to created CryptoKeys. | "" | -| key_metadata_file | string | yes | A file path location where key metadata used by the plugin will be persisted. See "[Management of keys](#management-of-keys)" for more information. | "" | -| key_ring | string | yes | Resource ID of the key ring where the keys managed by this plugin reside, in the format projects/\*/locations/\*/keyRings/\* | "" | -| service_account_file | string | no | Path to the service account file used to authenticate with the Cloud KMS API. | Value of `GOOGLE_APPLICATION_CREDENTIALS` environment variable. | +| Key | Type | Required | Description | Default | +|----------------------|--------|----------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-----------------------------------------------------------------| +| key_policy_file | string | no | A file path location to a custom [IAM Policy (v3)](https://cloud.google.com/pubsub/docs/reference/rpc/google.iam.v1#google.iam.v1.Policy) in JSON format to be attached to created CryptoKeys. | "" | +| key_metadata_file | string | yes | A file path location where key metadata used by the plugin will be persisted. See "[Management of keys](#management-of-keys)" for more information. | "" | +| key_metadata | string | yes | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | "" | +| key_ring | string | yes | Resource ID of the key ring where the keys managed by this plugin reside, in the format projects/\*/locations/\*/keyRings/\* | "" | +| service_account_file | string | no | Path to the service account file used to authenticate with the Cloud KMS API. | Value of `GOOGLE_APPLICATION_CREDENTIALS` environment variable. | ### Authenticating with the Cloud KMS API diff --git a/pkg/server/plugin/keymanager/awskms/awskms.go b/pkg/server/plugin/keymanager/awskms/awskms.go index b10484b4df..8755ad94cb 100644 --- a/pkg/server/plugin/keymanager/awskms/awskms.go +++ b/pkg/server/plugin/keymanager/awskms/awskms.go @@ -96,6 +96,7 @@ type Config struct { SecretAccessKey string `hcl:"secret_access_key" json:"secret_access_key"` Region string `hcl:"region" json:"region"` KeyMetadataFile string `hcl:"key_metadata_file" json:"key_metadata_file"` + KeyMetadata string `hcl:"key_metadata" json:"key_metadata"` KeyPolicyFile string `hcl:"key_policy_file" json:"key_policy_file"` } @@ -131,11 +132,14 @@ func (p *Plugin) Configure(ctx context.Context, req *configv1.ConfigureRequest) return nil, err } - serverID, err := loadServerID(config.KeyMetadataFile) - if err != nil { - return nil, err + var serverID = config.KeyMetadata + if config.KeyMetadata == "" { + serverID, err = getOrCreateServerID(config.KeyMetadataFile) + if err != nil { + return nil, err + } + p.log.Debug("Loaded server id", "server_id", serverID) } - p.log.Debug("Loaded server id", "server_id", serverID) if config.KeyPolicyFile != "" { policyBytes, err := os.ReadFile(config.KeyPolicyFile) @@ -832,8 +836,8 @@ func parseAndValidateConfig(c string) (*Config, error) { return nil, status.Error(codes.InvalidArgument, "configuration is missing a region") } - if config.KeyMetadataFile == "" { - return nil, status.Error(codes.InvalidArgument, "configuration is missing server id file path") + if config.KeyMetadataFile == "" && config.KeyMetadata == "" { + return nil, status.Error(codes.InvalidArgument, "configuration requires server id or server id file path") } return config, nil @@ -923,7 +927,7 @@ func min(x, y time.Duration) time.Duration { return y } -func loadServerID(idPath string) (string, error) { +func getOrCreateServerID(idPath string) (string, error) { // get id from path data, err := os.ReadFile(idPath) switch { diff --git a/pkg/server/plugin/keymanager/awskms/awskms_test.go b/pkg/server/plugin/keymanager/awskms/awskms_test.go index a74bf818a0..2e55fbdeeb 100644 --- a/pkg/server/plugin/keymanager/awskms/awskms_test.go +++ b/pkg/server/plugin/keymanager/awskms/awskms_test.go @@ -91,7 +91,7 @@ func TestKeyManagerContract(t *testing.T) { func(aws.Config) (stsClient, error) { return fakeSTSClient, nil }, ) km := new(keymanager.V1) - keyMetadataFile := filepath.Join(dir, "metadata.json") + keyMetadataFile := filepath.Join(dir, "metadata") if isWindows { keyMetadataFile = filepath.ToSlash(keyMetadataFile) } @@ -226,7 +226,7 @@ func TestConfigure(t *testing.T) { { name: "missing server id file path", configureRequest: configureRequestWithVars("access_key_id", "secret_access_key", "region", "", ""), - err: "configuration is missing server id file path", + err: "configuration requires server id or server id file path", code: codes.InvalidArgument, }, { diff --git a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go index 7c76ad3d88..f48fba9cb6 100644 --- a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go +++ b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go @@ -78,6 +78,7 @@ type pluginHooks struct { // Config provides configuration context for the plugin. type Config struct { KeyMetadataFile string `hcl:"key_metadata_file" json:"key_metadata_file"` + KeyMetadata string `hcl:"key_metadata" json:"key_metadata"` KeyVaultURI string `hcl:"key_vault_uri" json:"key_vault_uri"` TenantID string `hcl:"tenant_id" json:"tenant_id"` SubscriptionID string `hcl:"subscription_id" json:"subscription_id"` @@ -139,11 +140,14 @@ func (p *Plugin) Configure(ctx context.Context, req *configv1.ConfigureRequest) return nil, err } - serverID, err := getOrCreateServerID(config.KeyMetadataFile) - if err != nil { - return nil, err + var serverID = config.KeyMetadata + if config.KeyMetadata == "" { + serverID, err = getOrCreateServerID(config.KeyMetadataFile) + if err != nil { + return nil, err + } + p.log.Debug("Loaded server ID", "server_id", serverID) } - p.log.Debug("Loaded server ID", "server_id", serverID) var client cloudKeyManagementService @@ -685,8 +689,8 @@ func parseAndValidateConfig(c string) (*Config, error) { return nil, status.Error(codes.InvalidArgument, "configuration is missing the Key Vault URI") } - if config.KeyMetadataFile == "" { - return nil, status.Error(codes.InvalidArgument, "configuration is missing server ID file path") + if config.KeyMetadataFile == "" && config.KeyMetadata == "" { + return nil, status.Error(codes.InvalidArgument, "configuration requires server id or server id file path") } return config, nil diff --git a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go index 04f031ff66..585adcc22b 100644 --- a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go +++ b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go @@ -115,7 +115,7 @@ func TestConfigure(t *testing.T) { { name: "missing key metadata file", configureRequest: configureRequestWithVars("", validKeyVaultURI, "", "", "", validAppSecret, "true"), - err: "configuration is missing server ID file path", + err: "configuration requires server id or server id file path", code: codes.InvalidArgument, }, { @@ -149,7 +149,7 @@ func TestConfigure(t *testing.T) { { name: "missing server id file path", configureRequest: configureRequestWithVars("", validKeyVaultURI, validTenantID, validSubscriptionID, validAppID, validAppSecret, "false"), - err: "configuration is missing server ID file path", + err: "configuration requires server id or server id file path", code: codes.InvalidArgument, }, { diff --git a/pkg/server/plugin/keymanager/gcpkms/gcpkms.go b/pkg/server/plugin/keymanager/gcpkms/gcpkms.go index 69b921aeb5..8f06485d65 100644 --- a/pkg/server/plugin/keymanager/gcpkms/gcpkms.go +++ b/pkg/server/plugin/keymanager/gcpkms/gcpkms.go @@ -119,6 +119,9 @@ type Config struct { // File path location where key metadata used by the plugin is persisted. KeyMetadataFile string `hcl:"key_metadata_file" json:"key_metadata_file"` + // Key metadata used by the plugin. + KeyMetadata string `hcl:"key_metadata" json:"key_metadata"` + // File path location to a custom IAM Policy (v3) that will be set to // created CryptoKeys. KeyPolicyFile string `hcl:"key_policy_file" json:"key_policy_file"` @@ -167,11 +170,15 @@ func (p *Plugin) Configure(ctx context.Context, req *configv1.ConfigureRequest) return nil, err } - serverID, err := getOrCreateServerID(config.KeyMetadataFile) - if err != nil { - return nil, err + var serverID = config.KeyMetadata + if config.KeyMetadata == "" { + serverID, err = getOrCreateServerID(config.KeyMetadataFile) + if err != nil { + return nil, err + } + p.log.Debug("Loaded server ID", "server_id", serverID) } - p.log.Debug("Loaded server ID", "server_id", serverID) + var customPolicy *iam.Policy3 if config.KeyPolicyFile != "" { if customPolicy, err = parsePolicyFile(config.KeyPolicyFile); err != nil { @@ -1103,8 +1110,8 @@ func parseAndValidateConfig(c string) (*Config, error) { return nil, status.Error(codes.InvalidArgument, "configuration is missing the key ring") } - if config.KeyMetadataFile == "" { - return nil, status.Error(codes.InvalidArgument, "configuration is missing server ID file path") + if config.KeyMetadataFile == "" && config.KeyMetadata == "" { + return nil, status.Error(codes.InvalidArgument, "configuration requires server id or server id file path") } return config, nil diff --git a/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go b/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go index 4be5f0e5aa..1c82304ce2 100644 --- a/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go +++ b/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go @@ -236,7 +236,7 @@ func TestConfigure(t *testing.T) { config: &Config{ KeyRing: validKeyRing, }, - expectMsg: "configuration is missing server ID file path", + expectMsg: "configuration requires server id or server id file path", expectCode: codes.InvalidArgument, }, { From 58085c4665a0c885a93e5fca0d4b3cc77ce84212 Mon Sep 17 00:00:00 2001 From: Keegan Witt Date: Mon, 4 Dec 2023 16:39:50 -0500 Subject: [PATCH 02/17] PR feedback changes Signed-off-by: Keegan Witt --- doc/plugin_server_keymanager_aws_kms.md | 17 +-- ...lugin_server_keymanager_azure_key_vault.md | 23 ++-- doc/plugin_server_keymanager_gcp_kms.md | 17 +-- pkg/server/plugin/keymanager/awskms/awskms.go | 58 +++++++--- .../plugin/keymanager/awskms/awskms_test.go | 105 +++++++++++++----- .../azurekeyvault/azure_key_vault.go | 37 ++++-- .../azurekeyvault/azure_key_vault_test.go | 65 ++++++++--- pkg/server/plugin/keymanager/gcpkms/gcpkms.go | 26 ++++- .../plugin/keymanager/gcpkms/gcpkms_test.go | 100 +++++++++++++---- 9 files changed, 319 insertions(+), 129 deletions(-) diff --git a/doc/plugin_server_keymanager_aws_kms.md b/doc/plugin_server_keymanager_aws_kms.md index 6b44024a45..64ee080a4a 100644 --- a/doc/plugin_server_keymanager_aws_kms.md +++ b/doc/plugin_server_keymanager_aws_kms.md @@ -6,14 +6,15 @@ The `aws_kms` key manager plugin leverages the AWS Key Management Service (KMS) The plugin accepts the following configuration options: -| Key | Type | Required | Description | Default | -|-------------------|--------|---------------------------------------|-----------------------------------------------------------------------------------------|---------------------------------------------------------| -| access_key_id | string | see [AWS KMS Access](#aws-kms-access) | The Access Key Id used to authenticate to KMS | Value of the AWS_ACCESS_KEY_ID environment variable | -| secret_access_key | string | see [AWS KMS Access](#aws-kms-access) | The Secret Access Key used to authenticate to KMS | Value of the AWS_SECRET_ACCESS_KEY environment variable | -| region | string | yes | The region where the keys will be stored | | -| key_metadata_file | string | yes | A file path location where information about generated keys will be persisted | | -| key_metadata | string | yes | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | | -| key_policy_file | string | no | A file path location to a custom key policy in JSON format | "" | +| Key | Type | Required | Description | Default | +|----------------------|--------|---------------------------------------|--------------------------------------------------------------------------------------------|---------------------------------------------------------| +| access_key_id | string | see [AWS KMS Access](#aws-kms-access) | The Access Key Id used to authenticate to KMS | Value of the AWS_ACCESS_KEY_ID environment variable | +| secret_access_key | string | see [AWS KMS Access](#aws-kms-access) | The Secret Access Key used to authenticate to KMS | Value of the AWS_SECRET_ACCESS_KEY environment variable | +| region | string | yes | The region where the keys will be stored | | +| key_metadata_file | string | yes | A file path location where information about generated keys will be persisted (deprecated) | | +| key_identifier_file | string | yes | A file path location where information about generated keys will be persisted | | +| key_identifier_value | string | yes | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | | +| key_policy_file | string | no | A file path location to a custom key policy in JSON format | "" | ### Alias and Key Management diff --git a/doc/plugin_server_keymanager_azure_key_vault.md b/doc/plugin_server_keymanager_azure_key_vault.md index 23be12fe1e..005583f3d0 100644 --- a/doc/plugin_server_keymanager_azure_key_vault.md +++ b/doc/plugin_server_keymanager_azure_key_vault.md @@ -10,16 +10,17 @@ SPIRE. The plugin accepts the following configuration options: -| Key | Type | Required | Description | Default | -|-------------------|---------|----------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------|---------| -| key_metadata_file | string | yes | A file path location where key metadata used by the plugin will be persisted. See "[Management of keys](#management-of-keys)" for more information. | "" | -| key_metadata | string | yes | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | "" | -| key_vault_uri | string | Yes | The Key Vault URI where the keys managed by this plugin reside. | "" | -| use_msi | boolean | [Deprecated](#authenticating-to-azure) | Whether or not to use MSI to authenticate to Azure Key Vault. | false | -| subscription_id | string | [Optional](#authenticating-to-azure) | The subscription id. | "" | -| app_id | string | [Optional](#authenticating-to-azure) | The application id. | "" | -| app_secret | string | [Optional](#authenticating-to-azure) | The application secret. | "" | -| tenant_id | string | [Optional](#authenticating-to-azure) | The tenant id. | "" | +| Key | Type | Required | Description | Default | +|----------------------|---------|----------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------| +| key_metadata_file | string | yes | A file path location where key metadata used by the plugin will be persisted (deprecated). See "[Management of keys](#management-of-keys)" for more information. | "" | +| key_identifier_file | string | yes | A file path location where key metadata used by the plugin will be persisted. See "[Management of keys](#management-of-keys)" for more information. | "" | +| key_identifier_value | string | yes | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | "" | +| key_vault_uri | string | Yes | The Key Vault URI where the keys managed by this plugin reside. | "" | +| use_msi | boolean | [Deprecated](#authenticating-to-azure) | Whether or not to use MSI to authenticate to Azure Key Vault. | false | +| subscription_id | string | [Optional](#authenticating-to-azure) | The subscription id. | "" | +| app_id | string | [Optional](#authenticating-to-azure) | The application id. | "" | +| app_secret | string | [Optional](#authenticating-to-azure) | The application secret. | "" | +| tenant_id | string | [Optional](#authenticating-to-azure) | The tenant id. | "" | ### Authenticating to Azure @@ -53,7 +54,7 @@ following table is provided for informational purposes only: | Label | Description | |-----------------|----------------------------------------------------------------------------------------------------------------------------------------| -| spire-server-td | A string representing the trust domain name of the server. | +| spire-server-td | A string representing the trust domain name of the server. | | spire-server-id | Auto-generated ID that is unique to the server and is persisted in the _Key Metadata File_ (see the `key_metadata_file` configurable). | If the _Key Metadata File_ is not found during server startup, the file is diff --git a/doc/plugin_server_keymanager_gcp_kms.md b/doc/plugin_server_keymanager_gcp_kms.md index fd64c2be7f..24d16c53c6 100644 --- a/doc/plugin_server_keymanager_gcp_kms.md +++ b/doc/plugin_server_keymanager_gcp_kms.md @@ -13,8 +13,9 @@ The plugin accepts the following configuration options: | Key | Type | Required | Description | Default | |----------------------|--------|----------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-----------------------------------------------------------------| | key_policy_file | string | no | A file path location to a custom [IAM Policy (v3)](https://cloud.google.com/pubsub/docs/reference/rpc/google.iam.v1#google.iam.v1.Policy) in JSON format to be attached to created CryptoKeys. | "" | -| key_metadata_file | string | yes | A file path location where key metadata used by the plugin will be persisted. See "[Management of keys](#management-of-keys)" for more information. | "" | -| key_metadata | string | yes | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | "" | +| key_metadata_file | string | yes | A file path location where key metadata used by the plugin will be persisted (deprecated). See "[Management of keys](#management-of-keys)" for more information. | "" | +| key_identifier_file | string | yes | A file path location where key metadata used by the plugin will be persisted. See "[Management of keys](#management-of-keys)" for more information. | "" | +| key_identifier_value | string | yes | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | "" | | key_ring | string | yes | Resource ID of the key ring where the keys managed by this plugin reside, in the format projects/\*/locations/\*/keyRings/\* | "" | | service_account_file | string | no | Path to the service account file used to authenticate with the Cloud KMS API. | Value of `GOOGLE_APPLICATION_CREDENTIALS` environment variable. | @@ -50,12 +51,12 @@ the service. All the labels are named with the `spire-` prefix. Users don't need to interact with the labels managed by the plugin. The following table is provided for informational purposes only: -| Label | Description | -| ----- | ----------- | -| spire-server-td | SHA-1 checksum of the trust domain name of the server. | -| spire-server-id | Auto-generated ID that is unique to the server and is persisted in the _Key Metadata File_ (see the `key_metadata_file` configurable). | -| spire-last-update | Unix time of the last time that the plugin updated the CryptoKey to keep it active. | -| spire-active | Indicates if the CryptoKey is still in use by the plugin. | +| Label | Description | +|-------------------|----------------------------------------------------------------------------------------------------------------------------------------| +| spire-server-td | SHA-1 checksum of the trust domain name of the server. | +| spire-server-id | Auto-generated ID that is unique to the server and is persisted in the _Key Metadata File_ (see the `key_metadata_file` configurable). | +| spire-last-update | Unix time of the last time that the plugin updated the CryptoKey to keep it active. | +| spire-active | Indicates if the CryptoKey is still in use by the plugin. | If the _Key Metadata File_ is not found during server startup, the file is recreated, with a new auto-generated server ID. Consequently, if the file is diff --git a/pkg/server/plugin/keymanager/awskms/awskms.go b/pkg/server/plugin/keymanager/awskms/awskms.go index 8755ad94cb..374464e120 100644 --- a/pkg/server/plugin/keymanager/awskms/awskms.go +++ b/pkg/server/plugin/keymanager/awskms/awskms.go @@ -8,6 +8,7 @@ import ( "fmt" "os" "path" + "regexp" "strings" "sync" "time" @@ -92,12 +93,13 @@ type Plugin struct { // Config provides configuration context for the plugin type Config struct { - AccessKeyID string `hcl:"access_key_id" json:"access_key_id"` - SecretAccessKey string `hcl:"secret_access_key" json:"secret_access_key"` - Region string `hcl:"region" json:"region"` - KeyMetadataFile string `hcl:"key_metadata_file" json:"key_metadata_file"` - KeyMetadata string `hcl:"key_metadata" json:"key_metadata"` - KeyPolicyFile string `hcl:"key_policy_file" json:"key_policy_file"` + AccessKeyID string `hcl:"access_key_id" json:"access_key_id"` + SecretAccessKey string `hcl:"secret_access_key" json:"secret_access_key"` + Region string `hcl:"region" json:"region"` + KeyMetadataFile string `hcl:"key_metadata_file" json:"key_metadata_file"` + KeyIdentifierFile string `hcl:"key_identifier_file" json:"key_identifier_file"` + KeyIdentifierValue string `hcl:"key_identifier_value" json:"key_identifier_value"` + KeyPolicyFile string `hcl:"key_policy_file" json:"key_policy_file"` } // New returns an instantiated plugin @@ -132,22 +134,30 @@ func (p *Plugin) Configure(ctx context.Context, req *configv1.ConfigureRequest) return nil, err } - var serverID = config.KeyMetadata - if config.KeyMetadata == "" { + if config.KeyPolicyFile != "" { + policyBytes, err := os.ReadFile(config.KeyPolicyFile) + if err != nil { + return nil, status.Errorf(codes.Internal, "failed to read file configured in 'key_policy_file': %v", err) + } + policyStr := string(policyBytes) + p.keyPolicy = &policyStr + } + + var serverID = config.KeyIdentifierValue + if serverID == "" && config.KeyMetadataFile != "" { + p.log.Warn("`key_metadata_file` is deprecated in favor of `key_identifier_file` and will be removed in a future version") serverID, err = getOrCreateServerID(config.KeyMetadataFile) if err != nil { return nil, err } p.log.Debug("Loaded server id", "server_id", serverID) } - - if config.KeyPolicyFile != "" { - policyBytes, err := os.ReadFile(config.KeyPolicyFile) + if serverID == "" && config.KeyIdentifierFile != "" { + serverID, err = getOrCreateServerID(config.KeyIdentifierFile) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to read file configured in 'key_policy_file': %v", err) + return nil, err } - policyStr := string(policyBytes) - p.keyPolicy = &policyStr + p.log.Debug("Loaded server id", "server_id", serverID) } awsCfg, err := newAWSConfig(ctx, config) @@ -836,9 +846,27 @@ func parseAndValidateConfig(c string) (*Config, error) { return nil, status.Error(codes.InvalidArgument, "configuration is missing a region") } - if config.KeyMetadataFile == "" && config.KeyMetadata == "" { + if config.KeyIdentifierValue != "" { + re := regexp.MustCompile(".*[^A-z0-9/_-].*") + if re.MatchString(config.KeyIdentifierValue) { + return nil, status.Error(codes.InvalidArgument, "Key identifier must contain only alphanumeric characters, forward slashes (/), underscores (_), and dashes (-)") + } + if strings.HasPrefix(config.KeyIdentifierValue, "alias/aws/") { + return nil, status.Error(codes.InvalidArgument, "Key identifier must not start with alias/aws/") + } + if len(config.KeyIdentifierValue) > 256 { + return nil, status.Error(codes.InvalidArgument, "Key identifier must not be longer than 256 characters") + } + } + if config.KeyMetadataFile == "" && config.KeyIdentifierFile == "" && config.KeyIdentifierValue == "" { return nil, status.Error(codes.InvalidArgument, "configuration requires server id or server id file path") } + if (config.KeyMetadataFile != "" || config.KeyIdentifierFile != "") && config.KeyIdentifierValue != "" { + return nil, status.Error(codes.InvalidArgument, "configuration must not contain both server id and server id file path") + } + if config.KeyMetadataFile != "" && config.KeyIdentifierFile != "" { + return nil, status.Error(codes.InvalidArgument, "configuration must not contain both `key_identifier_file` and deprecated `key_metadata_file`") + } return config, nil } diff --git a/pkg/server/plugin/keymanager/awskms/awskms_test.go b/pkg/server/plugin/keymanager/awskms/awskms_test.go index 2e55fbdeeb..4556582e81 100644 --- a/pkg/server/plugin/keymanager/awskms/awskms_test.go +++ b/pkg/server/plugin/keymanager/awskms/awskms_test.go @@ -91,14 +91,14 @@ func TestKeyManagerContract(t *testing.T) { func(aws.Config) (stsClient, error) { return fakeSTSClient, nil }, ) km := new(keymanager.V1) - keyMetadataFile := filepath.Join(dir, "metadata") + keyIdentifierFile := filepath.Join(dir, "metadata") if isWindows { - keyMetadataFile = filepath.ToSlash(keyMetadataFile) + keyIdentifierFile = filepath.ToSlash(keyIdentifierFile) } plugintest.Load(t, builtin(p), km, plugintest.Configuref(` region = "fake-region" key_metadata_file = %q - `, keyMetadataFile)) + `, keyIdentifierFile)) return km } @@ -209,39 +209,77 @@ func TestConfigure(t *testing.T) { name: "pass without keys", configureRequest: configureRequestWithDefaults(t), }, + { + name: "pass with identity file", + configureRequest: configureRequestWithVars("", "secret_access_key", "region", KeyIdentifierFile, getKeyIdentifierFile(t), ""), + }, + { + name: "pass with identity value", + configureRequest: configureRequestWithVars("", "secret_access_key", "region", KeyIdentifierValue, "server-id", ""), + }, { name: "missing access key id", - configureRequest: configureRequestWithVars("", "secret_access_key", "region", getKeyMetadataFile(t), ""), + configureRequest: configureRequestWithVars("", "secret_access_key", "region", KeyMetadataFile, getKeyIdentifierFile(t), ""), }, { name: "missing secret access key", - configureRequest: configureRequestWithVars("access_key", "", "region", getKeyMetadataFile(t), ""), + configureRequest: configureRequestWithVars("access_key", "", "region", KeyMetadataFile, getKeyIdentifierFile(t), ""), }, { name: "missing region", - configureRequest: configureRequestWithVars("access_key_id", "secret_access_key", "", getKeyMetadataFile(t), ""), + configureRequest: configureRequestWithVars("access_key_id", "secret_access_key", "", KeyMetadataFile, getKeyIdentifierFile(t), ""), err: "configuration is missing a region", code: codes.InvalidArgument, }, { name: "missing server id file path", - configureRequest: configureRequestWithVars("access_key_id", "secret_access_key", "region", "", ""), + configureRequest: configureRequestWithVars("access_key_id", "secret_access_key", "region", KeyMetadataFile, "", ""), err: "configuration requires server id or server id file path", code: codes.InvalidArgument, }, + { + name: "key identifier file and key identifier value", + configureRequest: configureRequestWithString(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_identifier_file":"key_identifier_file","key_identifier_value":"key_identifier_value","key_policy_file":""}`), + err: "configuration must not contain both server id and server id file path", + code: codes.InvalidArgument, + }, + { + name: "key metadata file and key identifier file", + configureRequest: configureRequestWithString(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_metadata_file":"key_metadata_file","key_identifier_file":"key_identifier_file","key_policy_file":""}`), + err: "configuration must not contain both `key_identifier_file` and deprecated `key_metadata_file`", + code: codes.InvalidArgument, + }, + { + name: "key metadata value invalid character", + configureRequest: configureRequestWithString(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_identifier_value":"@key_identifier_value@","key_policy_file":""}`), + err: "Key identifier must contain only alphanumeric characters, forward slashes (/), underscores (_), and dashes (-)", + code: codes.InvalidArgument, + }, + { + name: "key metadata value too long", + configureRequest: configureRequestWithString(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_identifier_value":"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA","key_policy_file":""}`), + err: "Key identifier must not be longer than 256 characters", + code: codes.InvalidArgument, + }, + { + name: "key metadata value starts with illegal alias", + configureRequest: configureRequestWithString(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_identifier_value":"alias/aws/key_identifier_value","key_policy_file":""}`), + err: "Key identifier must not start with alias/aws/", + code: codes.InvalidArgument, + }, { name: "custom policy file does not exists", - configureRequest: configureRequestWithVars("access_key", "secret_access_key", "region", getEmptyKeyMetadataFile(t), "non-existent-file.json"), + configureRequest: configureRequestWithVars("access_key", "secret_access_key", "region", KeyMetadataFile, getEmptyKeyIdentifierFile(t), "non-existent-file.json"), err: fmt.Sprintf("failed to read file configured in 'key_policy_file': open non-existent-file.json: %s", spiretest.FileNotFound()), code: codes.Internal, }, { name: "use custom policy file", - configureRequest: configureRequestWithVars("access_key", "secret_access_key", "region", getEmptyKeyMetadataFile(t), getCustomPolicyFile(t)), + configureRequest: configureRequestWithVars("access_key", "secret_access_key", "region", KeyMetadataFile, getEmptyKeyIdentifierFile(t), getCustomPolicyFile(t)), }, { name: "new server id file path", - configureRequest: configureRequestWithVars("access_key_id", "secret_access_key", "region", getEmptyKeyMetadataFile(t), ""), + configureRequest: configureRequestWithVars("access_key_id", "secret_access_key", "region", KeyMetadataFile, getEmptyKeyIdentifierFile(t), ""), }, { name: "decode error", @@ -382,7 +420,7 @@ func TestGenerateKey(t *testing.T) { KeyId: spireKeyID, KeyType: keymanagerv1.KeyType_EC_P256, }, - configureReq: configureRequestWithVars("access_key_id", "secret_access_key", "region", getEmptyKeyMetadataFile(t), ""), + configureReq: configureRequestWithVars("access_key_id", "secret_access_key", "region", KeyMetadataFile, getEmptyKeyIdentifierFile(t), ""), instanceAccountID: "example-account-id", instanceRoleARN: "arn:aws:sts::example-account-id:assumed-role/example-assumed-role-name/example-instance-id", expectedKeyPolicy: &roleBasedPolicy, @@ -393,7 +431,7 @@ func TestGenerateKey(t *testing.T) { KeyId: spireKeyID, KeyType: keymanagerv1.KeyType_EC_P256, }, - configureReq: configureRequestWithVars("access_key_id", "secret_access_key", "region", getEmptyKeyMetadataFile(t), getCustomPolicyFile(t)), + configureReq: configureRequestWithVars("access_key_id", "secret_access_key", "region", KeyMetadataFile, getEmptyKeyIdentifierFile(t), getCustomPolicyFile(t)), instanceAccountID: "example-account-id", instanceRoleARN: "arn:aws:sts::example-account-id:assumed-role/example-assumed-role-name/example-instance-id", expectedKeyPolicy: &customPolicy, @@ -665,7 +703,7 @@ func TestGenerateKey(t *testing.T) { KeyId: spireKeyID, KeyType: keymanagerv1.KeyType_EC_P256, }, - configureReq: configureRequestWithVars("access_key_id", "secret_access_key", "region", getEmptyKeyMetadataFile(t), ""), + configureReq: configureRequestWithVars("access_key_id", "secret_access_key", "region", KeyMetadataFile, getEmptyKeyIdentifierFile(t), ""), getCallerIdentityErr: "something went wrong", err: "cannot get caller identity: something went wrong", code: codes.Internal, @@ -676,7 +714,7 @@ func TestGenerateKey(t *testing.T) { KeyId: spireKeyID, KeyType: keymanagerv1.KeyType_EC_P256, }, - configureReq: configureRequestWithVars("access_key_id", "secret_access_key", "region", getEmptyKeyMetadataFile(t), ""), + configureReq: configureRequestWithVars("access_key_id", "secret_access_key", "region", KeyMetadataFile, getEmptyKeyIdentifierFile(t), ""), instanceRoleARN: "arn:aws:sts::example-account-id", logs: []spiretest.LogEntry{ { @@ -691,7 +729,7 @@ func TestGenerateKey(t *testing.T) { KeyId: spireKeyID, KeyType: keymanagerv1.KeyType_EC_P256, }, - configureReq: configureRequestWithVars("access_key_id", "secret_access_key", "region", getKeyMetadataFile(t), ""), + configureReq: configureRequestWithVars("access_key_id", "secret_access_key", "region", KeyMetadataFile, getKeyIdentifierFile(t), ""), instanceRoleARN: "arn:aws:sts::example-account-id:user/development", logs: []spiretest.LogEntry{ { @@ -1932,23 +1970,33 @@ func TestDisposeKeys(t *testing.T) { func configureRequestWithString(config string) *configv1.ConfigureRequest { return &configv1.ConfigureRequest{ - HclConfiguration: config, + HclConfiguration: config, + CoreConfiguration: &configv1.CoreConfiguration{TrustDomain: "test.example.org"}, } } -func configureRequestWithVars(accessKeyID, secretAccessKey, region, keyMetadataFile, keyPolicyFile string) *configv1.ConfigureRequest { +type KeyIdentifierConfigName string + +const ( + KeyMetadataFile KeyIdentifierConfigName = "key_metadata_file" + KeyIdentifierFile KeyIdentifierConfigName = "key_identifier_file" + KeyIdentifierValue KeyIdentifierConfigName = "key_identifier_value" +) + +func configureRequestWithVars(accessKeyID, secretAccessKey, region, keyIdentifierConfigName KeyIdentifierConfigName, keyIdentifierConfigValue, keyPolicyFile string) *configv1.ConfigureRequest { return &configv1.ConfigureRequest{ HclConfiguration: fmt.Sprintf(`{ "access_key_id": "%s", "secret_access_key": "%s", "region":"%s", - "key_metadata_file":"%s", + "%s":"%s", "key_policy_file":"%s" }`, accessKeyID, secretAccessKey, region, - keyMetadataFile, + keyIdentifierConfigName, + keyIdentifierConfigValue, keyPolicyFile), CoreConfiguration: &configv1.CoreConfiguration{TrustDomain: "test.example.org"}, } @@ -1956,25 +2004,26 @@ func configureRequestWithVars(accessKeyID, secretAccessKey, region, keyMetadataF func configureRequestWithDefaults(t *testing.T) *configv1.ConfigureRequest { return &configv1.ConfigureRequest{ - HclConfiguration: serializedConfiguration(validAccessKeyID, validSecretAccessKey, validRegion, getKeyMetadataFile(t)), + HclConfiguration: serializedConfiguration(validAccessKeyID, validSecretAccessKey, validRegion, KeyMetadataFile, getKeyIdentifierFile(t)), CoreConfiguration: &configv1.CoreConfiguration{TrustDomain: "test.example.org"}, } } -func serializedConfiguration(accessKeyID, secretAccessKey, region string, keyMetadataFile string) string { +func serializedConfiguration(accessKeyID, secretAccessKey, region string, keyIdentifierConfigName KeyIdentifierConfigName, keyIdentifierConfigValue string) string { return fmt.Sprintf(`{ "access_key_id": "%s", "secret_access_key": "%s", "region":"%s", - "key_metadata_file":"%s" + "%s":"%s" }`, accessKeyID, secretAccessKey, region, - keyMetadataFile) + keyIdentifierConfigName, + keyIdentifierConfigValue) } -func getKeyMetadataFile(t *testing.T) string { +func getKeyIdentifierFile(t *testing.T) string { tempDir := t.TempDir() tempFilePath := path.Join(tempDir, validServerIDFile) err := os.WriteFile(tempFilePath, []byte(validServerID), 0o600) @@ -1987,13 +2036,13 @@ func getKeyMetadataFile(t *testing.T) string { return tempFilePath } -func getEmptyKeyMetadataFile(t *testing.T) string { +func getEmptyKeyIdentifierFile(t *testing.T) string { tempDir := t.TempDir() - keyMetadataFile := path.Join(tempDir, validServerIDFile) + keyIdentifierFile := path.Join(tempDir, validServerIDFile) if isWindows { - keyMetadataFile = filepath.ToSlash(keyMetadataFile) + keyIdentifierFile = filepath.ToSlash(keyIdentifierFile) } - return keyMetadataFile + return keyIdentifierFile } func getCustomPolicyFile(t *testing.T) string { diff --git a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go index f48fba9cb6..3ac518a671 100644 --- a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go +++ b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go @@ -77,13 +77,14 @@ type pluginHooks struct { // Config provides configuration context for the plugin. type Config struct { - KeyMetadataFile string `hcl:"key_metadata_file" json:"key_metadata_file"` - KeyMetadata string `hcl:"key_metadata" json:"key_metadata"` - KeyVaultURI string `hcl:"key_vault_uri" json:"key_vault_uri"` - TenantID string `hcl:"tenant_id" json:"tenant_id"` - SubscriptionID string `hcl:"subscription_id" json:"subscription_id"` - AppID string `hcl:"app_id" json:"app_id"` - AppSecret string `hcl:"app_secret" json:"app_secret"` + KeyMetadataFile string `hcl:"key_metadata_file" json:"key_metadata_file"` + KeyIdentifierFile string `hcl:"key_identifier_file" json:"key_identifier_file"` + KeyIdentifierValue string `hcl:"key_identifier_value" json:"key_identifier_value"` + KeyVaultURI string `hcl:"key_vault_uri" json:"key_vault_uri"` + TenantID string `hcl:"tenant_id" json:"tenant_id"` + SubscriptionID string `hcl:"subscription_id" json:"subscription_id"` + AppID string `hcl:"app_id" json:"app_id"` + AppSecret string `hcl:"app_secret" json:"app_secret"` // Deprecated: use_msi is deprecated and will be removed in a future release. // Will be used implicitly if other mechanisms to authenticate fail. @@ -140,13 +141,21 @@ func (p *Plugin) Configure(ctx context.Context, req *configv1.ConfigureRequest) return nil, err } - var serverID = config.KeyMetadata - if config.KeyMetadata == "" { + var serverID = config.KeyIdentifierValue + if serverID == "" && config.KeyMetadataFile != "" { + p.log.Warn("`key_metadata_file` is deprecated in favor of `key_identifier_file` and will be removed in a future version") serverID, err = getOrCreateServerID(config.KeyMetadataFile) if err != nil { return nil, err } - p.log.Debug("Loaded server ID", "server_id", serverID) + p.log.Debug("Loaded server id", "server_id", serverID) + } + if serverID == "" && config.KeyIdentifierFile != "" { + serverID, err = getOrCreateServerID(config.KeyIdentifierFile) + if err != nil { + return nil, err + } + p.log.Debug("Loaded server id", "server_id", serverID) } var client cloudKeyManagementService @@ -689,9 +698,15 @@ func parseAndValidateConfig(c string) (*Config, error) { return nil, status.Error(codes.InvalidArgument, "configuration is missing the Key Vault URI") } - if config.KeyMetadataFile == "" && config.KeyMetadata == "" { + if config.KeyMetadataFile == "" && config.KeyIdentifierFile == "" && config.KeyIdentifierValue == "" { return nil, status.Error(codes.InvalidArgument, "configuration requires server id or server id file path") } + if (config.KeyMetadataFile != "" || config.KeyIdentifierFile != "") && config.KeyIdentifierValue != "" { + return nil, status.Error(codes.InvalidArgument, "configuration must not contain both server id and server id file path") + } + if config.KeyMetadataFile != "" && config.KeyIdentifierFile != "" { + return nil, status.Error(codes.InvalidArgument, "configuration must not contain both `key_identifier_file` and deprecated `key_metadata_file`") + } return config, nil } diff --git a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go index 585adcc22b..2fb2de2a08 100644 --- a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go +++ b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go @@ -66,13 +66,13 @@ func TestKeyManagerContract(t *testing.T) { func(azcore.TokenCredential, string) (cloudKeyManagementService, error) { return kmsClient, nil }, ) km := new(keymanager.V1) - keyMetadataFile := createKeyMetadataFile(t) + keyIdentifierFile := createKeyIdentifierFile(t) plugintest.Load(t, builtin(p), km, plugintest.Configuref(` key_metadata_file = %q key_vault_uri = "https://spire-server.vault.azure.net/" use_msi=true - `, keyMetadataFile)) + `, keyIdentifierFile)) return km } @@ -112,55 +112,75 @@ func TestConfigure(t *testing.T) { name: "pass without keys", configureRequest: configureRequestWithDefaults(t), }, + { + name: "pass with identity file", + configureRequest: configureRequestWithVars(KeyIdentifierFile, createKeyIdentifierFile(t), validKeyVaultURI, validTenantID, validSubscriptionID, validAppID, validAppSecret, "false"), + }, + { + name: "pass with identity value", + configureRequest: configureRequestWithVars(KeyIdentifierValue, "server-id", validKeyVaultURI, validTenantID, validSubscriptionID, validAppID, validAppSecret, "false"), + }, { name: "missing key metadata file", - configureRequest: configureRequestWithVars("", validKeyVaultURI, "", "", "", validAppSecret, "true"), + configureRequest: configureRequestWithVars(KeyMetadataFile, "", validKeyVaultURI, validTenantID, validSubscriptionID, validAppID, validAppSecret, "false"), err: "configuration requires server id or server id file path", code: codes.InvalidArgument, }, + { + name: "key identifier file and key identifier value", + configureRequest: configureRequestWithString(fmt.Sprintf(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_identifier_file":"key_identifier_file","key_identifier_value":"key_identifier_value","key_policy_file":"","key_vault_uri":"%s"}`, validKeyVaultURI)), + err: "configuration must not contain both server id and server id file path", + code: codes.InvalidArgument, + }, + { + name: "key metadata file and key identifier file", + configureRequest: configureRequestWithString(fmt.Sprintf(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_metadata_file":"key_metadata_file","key_identifier_file":"key_identifier_file","key_policy_file":"","key_vault_uri":"%s"}`, validKeyVaultURI)), + err: "configuration must not contain both `key_identifier_file` and deprecated `key_metadata_file`", + code: codes.InvalidArgument, + }, { name: "missing client authentication config", - configureRequest: configureRequestWithVars(createKeyMetadataFile(t), validKeyVaultURI, "", "", "", "", "false"), + configureRequest: configureRequestWithVars(KeyMetadataFile, createKeyIdentifierFile(t), validKeyVaultURI, "", "", "", "", "false"), }, { name: "use MSI while app secret is set", - configureRequest: configureRequestWithVars(createKeyMetadataFile(t), validKeyVaultURI, "", "", "", validAppSecret, "true"), + configureRequest: configureRequestWithVars(KeyMetadataFile, createKeyIdentifierFile(t), validKeyVaultURI, "", "", "", validAppSecret, "true"), err: "invalid configuration, cannot use both MSI and app authentication", code: codes.InvalidArgument, }, { name: "missing Key Vault URI", - configureRequest: configureRequestWithVars(createKeyMetadataFile(t), "", validTenantID, validSubscriptionID, validAppID, validAppSecret, "false"), + configureRequest: configureRequestWithVars(KeyMetadataFile, createKeyIdentifierFile(t), "", validTenantID, validSubscriptionID, validAppID, validAppSecret, "false"), err: "configuration is missing the Key Vault URI", code: codes.InvalidArgument, }, { name: "missing tenant ID", - configureRequest: configureRequestWithVars(createKeyMetadataFile(t), validKeyVaultURI, "", validSubscriptionID, validAppID, validAppSecret, "false"), + configureRequest: configureRequestWithVars(KeyMetadataFile, createKeyIdentifierFile(t), validKeyVaultURI, "", validSubscriptionID, validAppID, validAppSecret, "false"), err: "invalid configuration, missing tenant id", code: codes.InvalidArgument, }, { name: "missing subscription ID ", - configureRequest: configureRequestWithVars(createKeyMetadataFile(t), validKeyVaultURI, validTenantID, "", validAppID, validAppSecret, "false"), + configureRequest: configureRequestWithVars(KeyMetadataFile, createKeyIdentifierFile(t), validKeyVaultURI, validTenantID, "", validAppID, validAppSecret, "false"), err: "invalid configuration, missing subscription id", code: codes.InvalidArgument, }, { name: "missing server id file path", - configureRequest: configureRequestWithVars("", validKeyVaultURI, validTenantID, validSubscriptionID, validAppID, validAppSecret, "false"), + configureRequest: configureRequestWithVars(KeyMetadataFile, "", validKeyVaultURI, validTenantID, validSubscriptionID, validAppID, validAppSecret, "false"), err: "configuration requires server id or server id file path", code: codes.InvalidArgument, }, { name: "missing application ID", - configureRequest: configureRequestWithVars(createKeyMetadataFile(t), validKeyVaultURI, validTenantID, validSubscriptionID, "", validAppSecret, "false"), + configureRequest: configureRequestWithVars(KeyMetadataFile, createKeyIdentifierFile(t), validKeyVaultURI, validTenantID, validSubscriptionID, "", validAppSecret, "false"), err: "invalid configuration, missing application id", code: codes.InvalidArgument, }, { name: "missing application secret", - configureRequest: configureRequestWithVars(createKeyMetadataFile(t), validKeyVaultURI, validTenantID, validSubscriptionID, validAppID, "", "false"), + configureRequest: configureRequestWithVars(KeyMetadataFile, createKeyIdentifierFile(t), validKeyVaultURI, validTenantID, validSubscriptionID, validAppID, "", "false"), err: "invalid configuration, missing app secret", code: codes.InvalidArgument, }, @@ -950,7 +970,7 @@ func setupTest(t *testing.T) *pluginTest { func configureRequestWithDefaults(t *testing.T) *configv1.ConfigureRequest { return &configv1.ConfigureRequest{ - HclConfiguration: serializedConfiguration(createKeyMetadataFile(t), validKeyVaultURI, validTenantID, validSubscriptionID, validAppID, validAppSecret, ""), + HclConfiguration: serializedConfiguration(KeyMetadataFile, createKeyIdentifierFile(t), validKeyVaultURI, validTenantID, validSubscriptionID, validAppID, validAppSecret, ""), CoreConfiguration: &configv1.CoreConfiguration{TrustDomain: trustDomain}, } } @@ -961,9 +981,17 @@ func getUUID(t *testing.T) string { return uuid.String() } -func serializedConfiguration(keyMetadataFile, keyVaultURI, tenantID, subscriptionID, appID, appSecret, useMsi string) string { +type KeyIdentifierConfigName string + +const ( + KeyMetadataFile KeyIdentifierConfigName = "key_metadata_file" + KeyIdentifierFile KeyIdentifierConfigName = "key_identifier_file" + KeyIdentifierValue KeyIdentifierConfigName = "key_identifier_value" +) + +func serializedConfiguration(keyIdentifierConfigName KeyIdentifierConfigName, keyIdentifierConfigValue, keyVaultURI, tenantID, subscriptionID, appID, appSecret, useMsi string) string { return fmt.Sprintf(`{ - "key_metadata_file":"%s", + "%s":"%s", "key_vault_uri":"%s", "tenant_id":"%s", "subscription_id":"%s", @@ -971,7 +999,8 @@ func serializedConfiguration(keyMetadataFile, keyVaultURI, tenantID, subscriptio "app_secret":"%s", "use_msi":%s }`, - keyMetadataFile, + keyIdentifierConfigName, + keyIdentifierConfigValue, keyVaultURI, tenantID, subscriptionID, @@ -980,9 +1009,9 @@ func serializedConfiguration(keyMetadataFile, keyVaultURI, tenantID, subscriptio useMsi) } -func configureRequestWithVars(keyMetadataFile, keyVaultURI, tenantID, subscriptionID, appID, appSecret, useMsi string) *configv1.ConfigureRequest { +func configureRequestWithVars(keyIdentifierConfigName KeyIdentifierConfigName, keyIdentifierConfigValue, keyVaultURI, tenantID, subscriptionID, appID, appSecret, useMsi string) *configv1.ConfigureRequest { return &configv1.ConfigureRequest{ - HclConfiguration: serializedConfiguration(keyMetadataFile, keyVaultURI, tenantID, subscriptionID, appID, appSecret, useMsi), + HclConfiguration: serializedConfiguration(keyIdentifierConfigName, keyIdentifierConfigValue, keyVaultURI, tenantID, subscriptionID, appID, appSecret, useMsi), CoreConfiguration: &configv1.CoreConfiguration{TrustDomain: trustDomain}, } } @@ -993,7 +1022,7 @@ func configureRequestWithString(config string) *configv1.ConfigureRequest { } } -func createKeyMetadataFile(t *testing.T) string { +func createKeyIdentifierFile(t *testing.T) string { tempDir := t.TempDir() tempFilePath := filepath.ToSlash(filepath.Join(tempDir, validServerIDFile)) err := os.WriteFile(tempFilePath, []byte(validServerID), 0o600) diff --git a/pkg/server/plugin/keymanager/gcpkms/gcpkms.go b/pkg/server/plugin/keymanager/gcpkms/gcpkms.go index 8f06485d65..32ea51f440 100644 --- a/pkg/server/plugin/keymanager/gcpkms/gcpkms.go +++ b/pkg/server/plugin/keymanager/gcpkms/gcpkms.go @@ -117,10 +117,11 @@ type Plugin struct { // Config provides configuration context for the plugin. type Config struct { // File path location where key metadata used by the plugin is persisted. - KeyMetadataFile string `hcl:"key_metadata_file" json:"key_metadata_file"` + KeyMetadataFile string `hcl:"key_metadata_file" json:"key_metadata_file"` + KeyIdentifierFile string `hcl:"key_identifier_file" json:"key_identifier_file"` // Key metadata used by the plugin. - KeyMetadata string `hcl:"key_metadata" json:"key_metadata"` + KeyIdentifierValue string `hcl:"key_identifier_value" json:"key_identifier_value"` // File path location to a custom IAM Policy (v3) that will be set to // created CryptoKeys. @@ -170,13 +171,20 @@ func (p *Plugin) Configure(ctx context.Context, req *configv1.ConfigureRequest) return nil, err } - var serverID = config.KeyMetadata - if config.KeyMetadata == "" { + var serverID = config.KeyIdentifierValue + if serverID == "" && config.KeyMetadataFile != "" { serverID, err = getOrCreateServerID(config.KeyMetadataFile) if err != nil { return nil, err } - p.log.Debug("Loaded server ID", "server_id", serverID) + p.log.Debug("Loaded server id", "server_id", serverID) + } + if serverID == "" && config.KeyIdentifierFile != "" { + serverID, err = getOrCreateServerID(config.KeyIdentifierFile) + if err != nil { + return nil, err + } + p.log.Debug("Loaded server id", "server_id", serverID) } var customPolicy *iam.Policy3 @@ -1110,9 +1118,15 @@ func parseAndValidateConfig(c string) (*Config, error) { return nil, status.Error(codes.InvalidArgument, "configuration is missing the key ring") } - if config.KeyMetadataFile == "" && config.KeyMetadata == "" { + if config.KeyMetadataFile == "" && config.KeyIdentifierFile == "" && config.KeyIdentifierValue == "" { return nil, status.Error(codes.InvalidArgument, "configuration requires server id or server id file path") } + if (config.KeyMetadataFile != "" || config.KeyIdentifierFile != "") && config.KeyIdentifierValue != "" { + return nil, status.Error(codes.InvalidArgument, "configuration must not contain both server id and server id file path") + } + if config.KeyMetadataFile != "" && config.KeyIdentifierFile != "" { + return nil, status.Error(codes.InvalidArgument, "configuration must not contain both `key_identifier_file` and deprecated `key_metadata_file`") + } return config, nil } diff --git a/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go b/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go index 1c82304ce2..7929514dee 100644 --- a/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go +++ b/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go @@ -133,7 +133,7 @@ func TestConfigure(t *testing.T) { { name: "pass with keys", config: &Config{ - KeyMetadataFile: createKeyMetadataFile(t, validServerID), + KeyMetadataFile: createKeyIdentifierFile(t, validServerID), KeyRing: validKeyRing, }, fakeCryptoKeys: []*fakeCryptoKey{ @@ -210,14 +210,28 @@ func TestConfigure(t *testing.T) { { name: "pass without keys", config: &Config{ - KeyMetadataFile: createKeyMetadataFile(t, validServerID), + KeyMetadataFile: createKeyIdentifierFile(t, validServerID), KeyRing: validKeyRing, }, }, + { + name: "pass with identity file", + config: &Config{ + KeyIdentifierFile: createKeyIdentifierFile(t, validServerID), + KeyRing: validKeyRing, + }, + }, + { + name: "pass with identity value", + config: &Config{ + KeyIdentifierValue: validServerID, + KeyRing: validKeyRing, + }, + }, { name: "pass without keys - using a service account file", config: &Config{ - KeyMetadataFile: createKeyMetadataFile(t, validServerID), + KeyMetadataFile: createKeyIdentifierFile(t, validServerID), KeyRing: validKeyRing, ServiceAccountFile: "service-account-file", }, @@ -226,7 +240,7 @@ func TestConfigure(t *testing.T) { { name: "missing key ring", config: &Config{ - KeyMetadataFile: createKeyMetadataFile(t, validServerID), + KeyMetadataFile: createKeyIdentifierFile(t, validServerID), }, expectMsg: "configuration is missing the key ring", expectCode: codes.InvalidArgument, @@ -239,10 +253,22 @@ func TestConfigure(t *testing.T) { expectMsg: "configuration requires server id or server id file path", expectCode: codes.InvalidArgument, }, + { + name: "key identifier file and key identifier value", + configureRequest: configureRequestWithString(fmt.Sprintf(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_identifier_file":"key_identifier_file","key_identifier_value":"key_identifier_value","key_policy_file":"","key_ring":"%s"}`, validKeyRing)), + expectMsg: "configuration must not contain both server id and server id file path", + expectCode: codes.InvalidArgument, + }, + { + name: "key metadata file and key identifier file", + configureRequest: configureRequestWithString(fmt.Sprintf(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_metadata_file":"key_metadata_file","key_identifier_file":"key_identifier_file","key_policy_file":"","key_ring":"%s"}`, validKeyRing)), + expectMsg: "configuration must not contain both `key_identifier_file` and deprecated `key_metadata_file`", + expectCode: codes.InvalidArgument, + }, { name: "custom policy file does not exist", config: &Config{ - KeyMetadataFile: createKeyMetadataFile(t, validServerID), + KeyMetadataFile: createKeyIdentifierFile(t, validServerID), KeyPolicyFile: "non-existent-file.json", KeyRing: validKeyRing, }, @@ -252,7 +278,7 @@ func TestConfigure(t *testing.T) { { name: "use custom policy file", config: &Config{ - KeyMetadataFile: createKeyMetadataFile(t, validServerID), + KeyMetadataFile: createKeyIdentifierFile(t, validServerID), KeyPolicyFile: getCustomPolicyFile(t), KeyRing: validKeyRing, }, @@ -260,14 +286,14 @@ func TestConfigure(t *testing.T) { { name: "empty key metadata file", config: &Config{ - KeyMetadataFile: createKeyMetadataFile(t, ""), + KeyMetadataFile: createKeyIdentifierFile(t, ""), KeyRing: validKeyRing, }, }, { name: "invalid server ID in metadata file", config: &Config{ - KeyMetadataFile: createKeyMetadataFile(t, "invalid-id"), + KeyMetadataFile: createKeyIdentifierFile(t, "invalid-id"), KeyRing: validKeyRing, }, expectMsg: "failed to parse server ID from path: uuid: incorrect UUID length 10 in string \"invalid-id\"", @@ -291,7 +317,7 @@ func TestConfigure(t *testing.T) { { name: "ListCryptoKeys error", config: &Config{ - KeyMetadataFile: createKeyMetadataFile(t, validServerID), + KeyMetadataFile: createKeyIdentifierFile(t, validServerID), KeyRing: validKeyRing, }, expectMsg: "failed to list SPIRE Server keys in Cloud KMS: error listing CryptoKeys", @@ -303,7 +329,7 @@ func TestConfigure(t *testing.T) { expectMsg: "failed to fetch entries: unsupported CryptoKeyVersionAlgorithm: GOOGLE_SYMMETRIC_ENCRYPTION", expectCode: codes.Internal, config: &Config{ - KeyMetadataFile: createKeyMetadataFile(t, validServerID), + KeyMetadataFile: createKeyIdentifierFile(t, validServerID), KeyRing: validKeyRing, }, fakeCryptoKeys: []*fakeCryptoKey{ @@ -331,7 +357,7 @@ func TestConfigure(t *testing.T) { expectMsg: "failed to fetch entries: error getting public key: get public key error", expectCode: codes.Internal, config: &Config{ - KeyMetadataFile: createKeyMetadataFile(t, validServerID), + KeyMetadataFile: createKeyIdentifierFile(t, validServerID), KeyRing: validKeyRing, }, fakeCryptoKeys: []*fakeCryptoKey{ @@ -684,7 +710,7 @@ func TestGenerateKey(t *testing.T) { KeyId: spireKeyID1, KeyType: keymanagerv1.KeyType_EC_P256, }, - configureReq: configureRequestWithVars(createKeyMetadataFile(t, ""), "", validKeyRing, "service_account_file"), + configureReq: configureRequestWithVars(KeyMetadataFile, createKeyIdentifierFile(t, ""), "", validKeyRing, "service_account_file"), }, { name: "success: non existing key with custom policy", @@ -692,7 +718,7 @@ func TestGenerateKey(t *testing.T) { KeyId: spireKeyID1, KeyType: keymanagerv1.KeyType_EC_P256, }, - configureReq: configureRequestWithVars(createKeyMetadataFile(t, ""), getCustomPolicyFile(t), validKeyRing, "service_account_file"), + configureReq: configureRequestWithVars(KeyMetadataFile, createKeyIdentifierFile(t, ""), getCustomPolicyFile(t), validKeyRing, "service_account_file"), }, { name: "success: replace old key", @@ -1413,7 +1439,7 @@ func TestSetIAMPolicy(t *testing.T) { if tt.useCustomPolicy { customPolicyFile := getCustomPolicyFile(t) configureReq = configureRequestFromConfig(&Config{ - KeyMetadataFile: createKeyMetadataFile(t, validServerID), + KeyMetadataFile: createKeyIdentifierFile(t, validServerID), KeyPolicyFile: customPolicyFile, KeyRing: validKeyRing, ServiceAccountFile: "service_account_file", @@ -1662,15 +1688,39 @@ func TestSignData(t *testing.T) { } } +type KeyIdentifierConfigName string + +const ( + KeyMetadataFile KeyIdentifierConfigName = "key_metadata_file" + KeyIdentifierFile KeyIdentifierConfigName = "key_identifier_file" + KeyIdentifierValue KeyIdentifierConfigName = "key_identifier_value" +) + func configureRequestFromConfig(c *Config) *configv1.ConfigureRequest { + keyMetadataFileHcl := fmt.Sprintf(`"key_metadata_file":"%s",`, c.KeyMetadataFile) + if c.KeyMetadataFile == "" { + keyMetadataFileHcl = "" + } + keyIdentifierFileHcl := fmt.Sprintf(`"key_identifier_file":"%s",`, c.KeyIdentifierFile) + if c.KeyIdentifierFile == "" { + keyIdentifierFileHcl = "" + } + keyIdentifierValueHcl := fmt.Sprintf(`"key_identifier_value":"%s",`, c.KeyIdentifierValue) + if c.KeyIdentifierValue == "" { + keyIdentifierValueHcl = "" + } return &configv1.ConfigureRequest{ HclConfiguration: fmt.Sprintf(`{ - "key_metadata_file":"%s", + %s + %s + %s "key_policy_file":"%s", "key_ring":"%s", "service_account_file":"%s" }`, - c.KeyMetadataFile, + keyMetadataFileHcl, + keyIdentifierFileHcl, + keyIdentifierValueHcl, c.KeyPolicyFile, c.KeyRing, c.ServiceAccountFile), @@ -1680,7 +1730,7 @@ func configureRequestFromConfig(c *Config) *configv1.ConfigureRequest { func configureRequestWithDefaults(t *testing.T) *configv1.ConfigureRequest { return &configv1.ConfigureRequest{ - HclConfiguration: serializedConfiguration(createKeyMetadataFile(t, validServerID), validKeyRing), + HclConfiguration: serializedConfiguration(KeyMetadataFile, createKeyIdentifierFile(t, validServerID), validKeyRing), CoreConfiguration: &configv1.CoreConfiguration{TrustDomain: "test.example.org"}, } } @@ -1691,15 +1741,16 @@ func configureRequestWithString(config string) *configv1.ConfigureRequest { } } -func configureRequestWithVars(keyMetadataFile, keyPolicyFile, keyRing, serviceAccountFile string) *configv1.ConfigureRequest { +func configureRequestWithVars(keyIdentifierConfigName KeyIdentifierConfigName, keyIdentifierConfigValue, keyPolicyFile, keyRing, serviceAccountFile string) *configv1.ConfigureRequest { return &configv1.ConfigureRequest{ HclConfiguration: fmt.Sprintf(`{ - "key_metadata_file":"%s", + "%s":"%s", "key_policy_file":"%s", "key_ring":"%s" "service_account_file":"%s" }`, - keyMetadataFile, + keyIdentifierConfigName, + keyIdentifierConfigValue, keyPolicyFile, keyRing, serviceAccountFile), @@ -1707,7 +1758,7 @@ func configureRequestWithVars(keyMetadataFile, keyPolicyFile, keyRing, serviceAc } } -func createKeyMetadataFile(t *testing.T, content string) string { +func createKeyIdentifierFile(t *testing.T, content string) string { tempDir := t.TempDir() tempFilePath := filepath.ToSlash(filepath.Join(tempDir, validServerIDFile)) @@ -1730,12 +1781,13 @@ func getCustomPolicyFile(t *testing.T) string { return tempFilePath } -func serializedConfiguration(keyMetadataFile, keyRing string) string { +func serializedConfiguration(keyIdentifierConfigName KeyIdentifierConfigName, keyIdentifierConfigValue, keyRing string) string { return fmt.Sprintf(`{ - "key_metadata_file":"%s", + "%s":"%s", "key_ring":"%s" }`, - keyMetadataFile, + keyIdentifierConfigName, + keyIdentifierConfigValue, keyRing) } From 834212d40e2c4ea653d3ecb76f376d3799ddc968 Mon Sep 17 00:00:00 2001 From: Keegan Witt Date: Fri, 8 Dec 2023 10:43:56 -0500 Subject: [PATCH 03/17] Add Azure key validation Signed-off-by: Keegan Witt --- .../keymanager/azurekeyvault/azure_key_vault.go | 10 ++++++++++ .../keymanager/azurekeyvault/azure_key_vault_test.go | 12 ++++++++++++ 2 files changed, 22 insertions(+) diff --git a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go index 3ac518a671..dfa2c73529 100644 --- a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go +++ b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go @@ -10,6 +10,7 @@ import ( "math/big" "net/http" "os" + "regexp" "strings" "sync" "time" @@ -698,6 +699,15 @@ func parseAndValidateConfig(c string) (*Config, error) { return nil, status.Error(codes.InvalidArgument, "configuration is missing the Key Vault URI") } + if config.KeyIdentifierValue != "" { + re := regexp.MustCompile(".*[^A-z0-9/_-].*") + if re.MatchString(config.KeyIdentifierValue) { + return nil, status.Error(codes.InvalidArgument, "Key identifier must contain only alphanumeric characters, forward slashes (/), underscores (_), and dashes (-)") + } + if len(config.KeyIdentifierValue) > 256 { + return nil, status.Error(codes.InvalidArgument, "Key identifier must not be longer than 256 characters") + } + } if config.KeyMetadataFile == "" && config.KeyIdentifierFile == "" && config.KeyIdentifierValue == "" { return nil, status.Error(codes.InvalidArgument, "configuration requires server id or server id file path") } diff --git a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go index 2fb2de2a08..e08fc49750 100644 --- a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go +++ b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go @@ -138,6 +138,18 @@ func TestConfigure(t *testing.T) { err: "configuration must not contain both `key_identifier_file` and deprecated `key_metadata_file`", code: codes.InvalidArgument, }, + { + name: "key metadata value invalid character", + configureRequest: configureRequestWithVars(KeyIdentifierValue, "@key_identifier_value@", validKeyVaultURI, validTenantID, validSubscriptionID, validAppID, validAppSecret, "false"), + err: "Key identifier must contain only alphanumeric characters, forward slashes (/), underscores (_), and dashes (-)", + code: codes.InvalidArgument, + }, + { + name: "key metadata value too long", + configureRequest: configureRequestWithVars(KeyIdentifierValue, "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", validKeyVaultURI, validTenantID, validSubscriptionID, validAppID, validAppSecret, "false"), + err: "Key identifier must not be longer than 256 characters", + code: codes.InvalidArgument, + }, { name: "missing client authentication config", configureRequest: configureRequestWithVars(KeyMetadataFile, createKeyIdentifierFile(t), validKeyVaultURI, "", "", "", "", "false"), From a0bc1e5050802a3e35a784f4636ea4eafa500564 Mon Sep 17 00:00:00 2001 From: Keegan Witt Date: Mon, 11 Dec 2023 20:51:56 -0500 Subject: [PATCH 04/17] Ensure server ID is always logged Signed-off-by: Keegan Witt --- pkg/server/plugin/keymanager/awskms/awskms.go | 3 +-- pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go | 3 +-- pkg/server/plugin/keymanager/gcpkms/gcpkms.go | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/pkg/server/plugin/keymanager/awskms/awskms.go b/pkg/server/plugin/keymanager/awskms/awskms.go index 374464e120..fc3dedcfa6 100644 --- a/pkg/server/plugin/keymanager/awskms/awskms.go +++ b/pkg/server/plugin/keymanager/awskms/awskms.go @@ -150,15 +150,14 @@ func (p *Plugin) Configure(ctx context.Context, req *configv1.ConfigureRequest) if err != nil { return nil, err } - p.log.Debug("Loaded server id", "server_id", serverID) } if serverID == "" && config.KeyIdentifierFile != "" { serverID, err = getOrCreateServerID(config.KeyIdentifierFile) if err != nil { return nil, err } - p.log.Debug("Loaded server id", "server_id", serverID) } + p.log.Debug("Loaded server id", "server_id", serverID) awsCfg, err := newAWSConfig(ctx, config) if err != nil { diff --git a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go index dfa2c73529..11f132e6da 100644 --- a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go +++ b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go @@ -149,15 +149,14 @@ func (p *Plugin) Configure(ctx context.Context, req *configv1.ConfigureRequest) if err != nil { return nil, err } - p.log.Debug("Loaded server id", "server_id", serverID) } if serverID == "" && config.KeyIdentifierFile != "" { serverID, err = getOrCreateServerID(config.KeyIdentifierFile) if err != nil { return nil, err } - p.log.Debug("Loaded server id", "server_id", serverID) } + p.log.Debug("Loaded server id", "server_id", serverID) var client cloudKeyManagementService diff --git a/pkg/server/plugin/keymanager/gcpkms/gcpkms.go b/pkg/server/plugin/keymanager/gcpkms/gcpkms.go index 32ea51f440..d27eee0492 100644 --- a/pkg/server/plugin/keymanager/gcpkms/gcpkms.go +++ b/pkg/server/plugin/keymanager/gcpkms/gcpkms.go @@ -177,15 +177,14 @@ func (p *Plugin) Configure(ctx context.Context, req *configv1.ConfigureRequest) if err != nil { return nil, err } - p.log.Debug("Loaded server id", "server_id", serverID) } if serverID == "" && config.KeyIdentifierFile != "" { serverID, err = getOrCreateServerID(config.KeyIdentifierFile) if err != nil { return nil, err } - p.log.Debug("Loaded server id", "server_id", serverID) } + p.log.Debug("Loaded server id", "server_id", serverID) var customPolicy *iam.Policy3 if config.KeyPolicyFile != "" { From 023e717fbbb0a59f683c2b77fd7e0c8f3d4e9eba Mon Sep 17 00:00:00 2001 From: Keegan Witt Date: Wed, 13 Dec 2023 23:00:20 -0500 Subject: [PATCH 05/17] Replace backticks with single quotes Signed-off-by: Keegan Witt --- pkg/server/plugin/keymanager/awskms/awskms.go | 4 ++-- pkg/server/plugin/keymanager/awskms/awskms_test.go | 2 +- pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go | 4 ++-- .../plugin/keymanager/azurekeyvault/azure_key_vault_test.go | 2 +- pkg/server/plugin/keymanager/gcpkms/gcpkms.go | 2 +- pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/server/plugin/keymanager/awskms/awskms.go b/pkg/server/plugin/keymanager/awskms/awskms.go index fc3dedcfa6..54dcfaef25 100644 --- a/pkg/server/plugin/keymanager/awskms/awskms.go +++ b/pkg/server/plugin/keymanager/awskms/awskms.go @@ -145,7 +145,7 @@ func (p *Plugin) Configure(ctx context.Context, req *configv1.ConfigureRequest) var serverID = config.KeyIdentifierValue if serverID == "" && config.KeyMetadataFile != "" { - p.log.Warn("`key_metadata_file` is deprecated in favor of `key_identifier_file` and will be removed in a future version") + p.log.Warn("'key_metadata_file' is deprecated in favor of 'key_identifier_file' and will be removed in a future version") serverID, err = getOrCreateServerID(config.KeyMetadataFile) if err != nil { return nil, err @@ -864,7 +864,7 @@ func parseAndValidateConfig(c string) (*Config, error) { return nil, status.Error(codes.InvalidArgument, "configuration must not contain both server id and server id file path") } if config.KeyMetadataFile != "" && config.KeyIdentifierFile != "" { - return nil, status.Error(codes.InvalidArgument, "configuration must not contain both `key_identifier_file` and deprecated `key_metadata_file`") + return nil, status.Error(codes.InvalidArgument, "configuration must not contain both 'key_identifier_file' and deprecated 'key_metadata_file'") } return config, nil diff --git a/pkg/server/plugin/keymanager/awskms/awskms_test.go b/pkg/server/plugin/keymanager/awskms/awskms_test.go index 4556582e81..05fcd15250 100644 --- a/pkg/server/plugin/keymanager/awskms/awskms_test.go +++ b/pkg/server/plugin/keymanager/awskms/awskms_test.go @@ -246,7 +246,7 @@ func TestConfigure(t *testing.T) { { name: "key metadata file and key identifier file", configureRequest: configureRequestWithString(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_metadata_file":"key_metadata_file","key_identifier_file":"key_identifier_file","key_policy_file":""}`), - err: "configuration must not contain both `key_identifier_file` and deprecated `key_metadata_file`", + err: "configuration must not contain both 'key_identifier_file' and deprecated 'key_metadata_file'", code: codes.InvalidArgument, }, { diff --git a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go index 11f132e6da..74bb340c98 100644 --- a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go +++ b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go @@ -144,7 +144,7 @@ func (p *Plugin) Configure(ctx context.Context, req *configv1.ConfigureRequest) var serverID = config.KeyIdentifierValue if serverID == "" && config.KeyMetadataFile != "" { - p.log.Warn("`key_metadata_file` is deprecated in favor of `key_identifier_file` and will be removed in a future version") + p.log.Warn("'key_metadata_file' is deprecated in favor of 'key_identifier_file' and will be removed in a future version") serverID, err = getOrCreateServerID(config.KeyMetadataFile) if err != nil { return nil, err @@ -714,7 +714,7 @@ func parseAndValidateConfig(c string) (*Config, error) { return nil, status.Error(codes.InvalidArgument, "configuration must not contain both server id and server id file path") } if config.KeyMetadataFile != "" && config.KeyIdentifierFile != "" { - return nil, status.Error(codes.InvalidArgument, "configuration must not contain both `key_identifier_file` and deprecated `key_metadata_file`") + return nil, status.Error(codes.InvalidArgument, "configuration must not contain both 'key_identifier_file' and deprecated 'key_metadata_file'") } return config, nil diff --git a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go index e08fc49750..ccd7355bfe 100644 --- a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go +++ b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go @@ -135,7 +135,7 @@ func TestConfigure(t *testing.T) { { name: "key metadata file and key identifier file", configureRequest: configureRequestWithString(fmt.Sprintf(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_metadata_file":"key_metadata_file","key_identifier_file":"key_identifier_file","key_policy_file":"","key_vault_uri":"%s"}`, validKeyVaultURI)), - err: "configuration must not contain both `key_identifier_file` and deprecated `key_metadata_file`", + err: "configuration must not contain both 'key_identifier_file' and deprecated 'key_metadata_file'", code: codes.InvalidArgument, }, { diff --git a/pkg/server/plugin/keymanager/gcpkms/gcpkms.go b/pkg/server/plugin/keymanager/gcpkms/gcpkms.go index d27eee0492..89ca07bb99 100644 --- a/pkg/server/plugin/keymanager/gcpkms/gcpkms.go +++ b/pkg/server/plugin/keymanager/gcpkms/gcpkms.go @@ -1124,7 +1124,7 @@ func parseAndValidateConfig(c string) (*Config, error) { return nil, status.Error(codes.InvalidArgument, "configuration must not contain both server id and server id file path") } if config.KeyMetadataFile != "" && config.KeyIdentifierFile != "" { - return nil, status.Error(codes.InvalidArgument, "configuration must not contain both `key_identifier_file` and deprecated `key_metadata_file`") + return nil, status.Error(codes.InvalidArgument, "configuration must not contain both 'key_identifier_file' and deprecated 'key_metadata_file'") } return config, nil diff --git a/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go b/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go index 7929514dee..09e00b3998 100644 --- a/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go +++ b/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go @@ -262,7 +262,7 @@ func TestConfigure(t *testing.T) { { name: "key metadata file and key identifier file", configureRequest: configureRequestWithString(fmt.Sprintf(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_metadata_file":"key_metadata_file","key_identifier_file":"key_identifier_file","key_policy_file":"","key_ring":"%s"}`, validKeyRing)), - expectMsg: "configuration must not contain both `key_identifier_file` and deprecated `key_metadata_file`", + expectMsg: "configuration must not contain both 'key_identifier_file' and deprecated 'key_metadata_file'", expectCode: codes.InvalidArgument, }, { From 0b8c4f3b25e28cc06b626356949e96b41626e3ac Mon Sep 17 00:00:00 2001 From: Keegan Witt Date: Fri, 15 Dec 2023 17:59:34 -0500 Subject: [PATCH 06/17] Update doc/plugin_server_keymanager_aws_kms.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Agustín Martínez Fayó Signed-off-by: Keegan Witt --- doc/plugin_server_keymanager_aws_kms.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/plugin_server_keymanager_aws_kms.md b/doc/plugin_server_keymanager_aws_kms.md index 64ee080a4a..9c1625bb8a 100644 --- a/doc/plugin_server_keymanager_aws_kms.md +++ b/doc/plugin_server_keymanager_aws_kms.md @@ -13,7 +13,7 @@ The plugin accepts the following configuration options: | region | string | yes | The region where the keys will be stored | | | key_metadata_file | string | yes | A file path location where information about generated keys will be persisted (deprecated) | | | key_identifier_file | string | yes | A file path location where information about generated keys will be persisted | | -| key_identifier_value | string | yes | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | | +| key_identifier_value | string | Required if key_identifier_file is not set | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | | | key_policy_file | string | no | A file path location to a custom key policy in JSON format | "" | ### Alias and Key Management From 0b79dd1310ab41651bc60d12348cc86c19cc7a19 Mon Sep 17 00:00:00 2001 From: Keegan Witt Date: Fri, 15 Dec 2023 17:59:54 -0500 Subject: [PATCH 07/17] Update doc/plugin_server_keymanager_aws_kms.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Agustín Martínez Fayó Signed-off-by: Keegan Witt --- doc/plugin_server_keymanager_aws_kms.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/plugin_server_keymanager_aws_kms.md b/doc/plugin_server_keymanager_aws_kms.md index 9c1625bb8a..43bc9c5d9e 100644 --- a/doc/plugin_server_keymanager_aws_kms.md +++ b/doc/plugin_server_keymanager_aws_kms.md @@ -11,7 +11,7 @@ The plugin accepts the following configuration options: | access_key_id | string | see [AWS KMS Access](#aws-kms-access) | The Access Key Id used to authenticate to KMS | Value of the AWS_ACCESS_KEY_ID environment variable | | secret_access_key | string | see [AWS KMS Access](#aws-kms-access) | The Secret Access Key used to authenticate to KMS | Value of the AWS_SECRET_ACCESS_KEY environment variable | | region | string | yes | The region where the keys will be stored | | -| key_metadata_file | string | yes | A file path location where information about generated keys will be persisted (deprecated) | | +| key_metadata_file | string | Required if key_identifier_value is not set | A file path location where information about generated keys will be persisted (deprecated, use key_identifier_file instead) | | | key_identifier_file | string | yes | A file path location where information about generated keys will be persisted | | | key_identifier_value | string | Required if key_identifier_file is not set | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | | | key_policy_file | string | no | A file path location to a custom key policy in JSON format | "" | From 9be5edeeb4152b3500100ea4e3b79a9dabf9b3a5 Mon Sep 17 00:00:00 2001 From: Keegan Witt Date: Fri, 15 Dec 2023 18:00:04 -0500 Subject: [PATCH 08/17] Update doc/plugin_server_keymanager_aws_kms.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Agustín Martínez Fayó Signed-off-by: Keegan Witt --- doc/plugin_server_keymanager_aws_kms.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/plugin_server_keymanager_aws_kms.md b/doc/plugin_server_keymanager_aws_kms.md index 43bc9c5d9e..b7b70ae748 100644 --- a/doc/plugin_server_keymanager_aws_kms.md +++ b/doc/plugin_server_keymanager_aws_kms.md @@ -12,7 +12,7 @@ The plugin accepts the following configuration options: | secret_access_key | string | see [AWS KMS Access](#aws-kms-access) | The Secret Access Key used to authenticate to KMS | Value of the AWS_SECRET_ACCESS_KEY environment variable | | region | string | yes | The region where the keys will be stored | | | key_metadata_file | string | Required if key_identifier_value is not set | A file path location where information about generated keys will be persisted (deprecated, use key_identifier_file instead) | | -| key_identifier_file | string | yes | A file path location where information about generated keys will be persisted | | +| key_identifier_file | string | Required if key_identifier_value is not set | A file path location where information about generated keys will be persisted | | | key_identifier_value | string | Required if key_identifier_file is not set | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | | | key_policy_file | string | no | A file path location to a custom key policy in JSON format | "" | From 45b2cb1430dad88d3f2b03c9c0e6fa6a538cf539 Mon Sep 17 00:00:00 2001 From: Keegan Witt Date: Fri, 15 Dec 2023 18:12:06 -0500 Subject: [PATCH 09/17] Remove character check for Azure Signed-off-by: Keegan Witt --- .../plugin/keymanager/azurekeyvault/azure_key_vault.go | 5 ----- .../plugin/keymanager/azurekeyvault/azure_key_vault_test.go | 6 ------ 2 files changed, 11 deletions(-) diff --git a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go index 74bb340c98..d63fa1959f 100644 --- a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go +++ b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault.go @@ -10,7 +10,6 @@ import ( "math/big" "net/http" "os" - "regexp" "strings" "sync" "time" @@ -699,10 +698,6 @@ func parseAndValidateConfig(c string) (*Config, error) { } if config.KeyIdentifierValue != "" { - re := regexp.MustCompile(".*[^A-z0-9/_-].*") - if re.MatchString(config.KeyIdentifierValue) { - return nil, status.Error(codes.InvalidArgument, "Key identifier must contain only alphanumeric characters, forward slashes (/), underscores (_), and dashes (-)") - } if len(config.KeyIdentifierValue) > 256 { return nil, status.Error(codes.InvalidArgument, "Key identifier must not be longer than 256 characters") } diff --git a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go index ccd7355bfe..5c09e4d605 100644 --- a/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go +++ b/pkg/server/plugin/keymanager/azurekeyvault/azure_key_vault_test.go @@ -138,12 +138,6 @@ func TestConfigure(t *testing.T) { err: "configuration must not contain both 'key_identifier_file' and deprecated 'key_metadata_file'", code: codes.InvalidArgument, }, - { - name: "key metadata value invalid character", - configureRequest: configureRequestWithVars(KeyIdentifierValue, "@key_identifier_value@", validKeyVaultURI, validTenantID, validSubscriptionID, validAppID, validAppSecret, "false"), - err: "Key identifier must contain only alphanumeric characters, forward slashes (/), underscores (_), and dashes (-)", - code: codes.InvalidArgument, - }, { name: "key metadata value too long", configureRequest: configureRequestWithVars(KeyIdentifierValue, "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", validKeyVaultURI, validTenantID, validSubscriptionID, validAppID, validAppSecret, "false"), From fb061c09d5b29d81eef6bb9292ab289ab7387acf Mon Sep 17 00:00:00 2001 From: Keegan Witt Date: Fri, 15 Dec 2023 23:43:52 -0500 Subject: [PATCH 10/17] Add GCP validation Signed-off-by: Keegan Witt --- pkg/server/plugin/keymanager/gcpkms/gcpkms.go | 25 +++++++++++++++++++ .../plugin/keymanager/gcpkms/gcpkms_test.go | 21 +++++++++++++++- 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/pkg/server/plugin/keymanager/gcpkms/gcpkms.go b/pkg/server/plugin/keymanager/gcpkms/gcpkms.go index 89ca07bb99..6c470f7a30 100644 --- a/pkg/server/plugin/keymanager/gcpkms/gcpkms.go +++ b/pkg/server/plugin/keymanager/gcpkms/gcpkms.go @@ -11,9 +11,12 @@ import ( "fmt" "hash/crc32" "os" + "regexp" + "strconv" "strings" "sync" "time" + "unicode" "cloud.google.com/go/iam" "cloud.google.com/go/iam/apiv1/iampb" @@ -1126,10 +1129,32 @@ func parseAndValidateConfig(c string) (*Config, error) { if config.KeyMetadataFile != "" && config.KeyIdentifierFile != "" { return nil, status.Error(codes.InvalidArgument, "configuration must not contain both 'key_identifier_file' and deprecated 'key_metadata_file'") } + if config.KeyIdentifierValue != "" { + if !validateCharacters(config.KeyIdentifierValue) { + return nil, status.Error(codes.InvalidArgument, "Key identifier must contain only alphanumeric characters, underscores (_), and dashes (-)") + } + if !unicode.IsLetter(rune(config.KeyIdentifierValue[0])) { + return nil, status.Error(codes.InvalidArgument, "Key identifier must start with a letter character") + } + if len(config.KeyIdentifierValue) > 63 { + return nil, status.Error(codes.InvalidArgument, "Key identifier must not be longer than 63 characters") + } + } return config, nil } +func validateCharacters(str string) bool { + re := regexp.MustCompile("[0-9_-]") + for _, r := range str { + s := strconv.QuoteRune(r) + if !unicode.IsLetter(r) && !re.MatchString(s) { + return false + } + } + return true +} + // parsePolicyFile parses a file containing iam.Policy3 data in JSON format. func parsePolicyFile(policyFile string) (*iam.Policy3, error) { policyBytes, err := os.ReadFile(policyFile) diff --git a/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go b/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go index 09e00b3998..3205cff88d 100644 --- a/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go +++ b/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go @@ -265,6 +265,24 @@ func TestConfigure(t *testing.T) { expectMsg: "configuration must not contain both 'key_identifier_file' and deprecated 'key_metadata_file'", expectCode: codes.InvalidArgument, }, + { + name: "key metadata value invalid character", + configureRequest: configureRequestWithString(fmt.Sprintf(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_identifier_value":"key_identifier_value@","key_policy_file":"","key_ring":"%s"}`, validKeyRing)), + expectMsg: "Key identifier must contain only alphanumeric characters, underscores (_), and dashes (-)", + expectCode: codes.InvalidArgument, + }, + { + name: "key metadata value too long", + configureRequest: configureRequestWithString(fmt.Sprintf(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_identifier_value":"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA","key_policy_file":"","key_ring":"%s"}`, validKeyRing)), + expectMsg: "Key identifier must not be longer than 63 characters", + expectCode: codes.InvalidArgument, + }, + { + name: "key metadata value starts with non alphabetic character", + configureRequest: configureRequestWithString(fmt.Sprintf(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_identifier_value":"0_key_identifier_value","key_policy_file":"","key_ring":"%s"}`, validKeyRing)), + expectMsg: "Key identifier must start with a letter character", + expectCode: codes.InvalidArgument, + }, { name: "custom policy file does not exist", config: &Config{ @@ -1737,7 +1755,8 @@ func configureRequestWithDefaults(t *testing.T) *configv1.ConfigureRequest { func configureRequestWithString(config string) *configv1.ConfigureRequest { return &configv1.ConfigureRequest{ - HclConfiguration: config, + HclConfiguration: config, + CoreConfiguration: &configv1.CoreConfiguration{TrustDomain: "test.example.org"}, } } From e078c063739e2eb340d0c3b0a4c8533a5432a9d3 Mon Sep 17 00:00:00 2001 From: Keegan Witt Date: Mon, 18 Dec 2023 15:58:59 -0500 Subject: [PATCH 11/17] GCP validation PR suggestions Signed-off-by: Keegan Witt --- pkg/server/plugin/keymanager/gcpkms/gcpkms.go | 11 ++--------- pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go | 10 ++-------- 2 files changed, 4 insertions(+), 17 deletions(-) diff --git a/pkg/server/plugin/keymanager/gcpkms/gcpkms.go b/pkg/server/plugin/keymanager/gcpkms/gcpkms.go index 6c470f7a30..071fd415a1 100644 --- a/pkg/server/plugin/keymanager/gcpkms/gcpkms.go +++ b/pkg/server/plugin/keymanager/gcpkms/gcpkms.go @@ -11,8 +11,6 @@ import ( "fmt" "hash/crc32" "os" - "regexp" - "strconv" "strings" "sync" "time" @@ -1131,10 +1129,7 @@ func parseAndValidateConfig(c string) (*Config, error) { } if config.KeyIdentifierValue != "" { if !validateCharacters(config.KeyIdentifierValue) { - return nil, status.Error(codes.InvalidArgument, "Key identifier must contain only alphanumeric characters, underscores (_), and dashes (-)") - } - if !unicode.IsLetter(rune(config.KeyIdentifierValue[0])) { - return nil, status.Error(codes.InvalidArgument, "Key identifier must start with a letter character") + return nil, status.Error(codes.InvalidArgument, "Key identifier must contain only letters, numbers, underscores (_), and dashes (-)") } if len(config.KeyIdentifierValue) > 63 { return nil, status.Error(codes.InvalidArgument, "Key identifier must not be longer than 63 characters") @@ -1145,10 +1140,8 @@ func parseAndValidateConfig(c string) (*Config, error) { } func validateCharacters(str string) bool { - re := regexp.MustCompile("[0-9_-]") for _, r := range str { - s := strconv.QuoteRune(r) - if !unicode.IsLetter(r) && !re.MatchString(s) { + if !unicode.IsLower(r) && unicode.IsNumber(r) && r != '-' && r != '_' { return false } } diff --git a/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go b/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go index 3205cff88d..272d8dde03 100644 --- a/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go +++ b/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go @@ -267,8 +267,8 @@ func TestConfigure(t *testing.T) { }, { name: "key metadata value invalid character", - configureRequest: configureRequestWithString(fmt.Sprintf(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_identifier_value":"key_identifier_value@","key_policy_file":"","key_ring":"%s"}`, validKeyRing)), - expectMsg: "Key identifier must contain only alphanumeric characters, underscores (_), and dashes (-)", + configureRequest: configureRequestWithString(fmt.Sprintf(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_identifier_value":"key identifier value","key_policy_file":"","key_ring":"%s"}`, validKeyRing)), + expectMsg: "Key identifier must contain only letters, numbers, underscores (_), and dashes (-)", expectCode: codes.InvalidArgument, }, { @@ -277,12 +277,6 @@ func TestConfigure(t *testing.T) { expectMsg: "Key identifier must not be longer than 63 characters", expectCode: codes.InvalidArgument, }, - { - name: "key metadata value starts with non alphabetic character", - configureRequest: configureRequestWithString(fmt.Sprintf(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_identifier_value":"0_key_identifier_value","key_policy_file":"","key_ring":"%s"}`, validKeyRing)), - expectMsg: "Key identifier must start with a letter character", - expectCode: codes.InvalidArgument, - }, { name: "custom policy file does not exist", config: &Config{ From 3ab2e5a740f001f6e3499b5a0d16ffff5b8ad6ec Mon Sep 17 00:00:00 2001 From: Keegan Witt Date: Tue, 19 Dec 2023 14:55:23 -0500 Subject: [PATCH 12/17] Fix unit test Signed-off-by: Keegan Witt --- pkg/server/plugin/keymanager/gcpkms/gcpkms.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/server/plugin/keymanager/gcpkms/gcpkms.go b/pkg/server/plugin/keymanager/gcpkms/gcpkms.go index 071fd415a1..d718bed31e 100644 --- a/pkg/server/plugin/keymanager/gcpkms/gcpkms.go +++ b/pkg/server/plugin/keymanager/gcpkms/gcpkms.go @@ -1141,7 +1141,7 @@ func parseAndValidateConfig(c string) (*Config, error) { func validateCharacters(str string) bool { for _, r := range str { - if !unicode.IsLower(r) && unicode.IsNumber(r) && r != '-' && r != '_' { + if !unicode.IsLower(r) && !unicode.IsNumber(r) && r != '-' && r != '_' { return false } } From d6f09c688702e488f1b14deb4a8ae4034396060b Mon Sep 17 00:00:00 2001 From: Keegan Witt Date: Tue, 19 Dec 2023 15:24:32 -0500 Subject: [PATCH 13/17] Fix unit test Signed-off-by: Keegan Witt --- pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go b/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go index 272d8dde03..c772483519 100644 --- a/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go +++ b/pkg/server/plugin/keymanager/gcpkms/gcpkms_test.go @@ -273,7 +273,7 @@ func TestConfigure(t *testing.T) { }, { name: "key metadata value too long", - configureRequest: configureRequestWithString(fmt.Sprintf(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_identifier_value":"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA","key_policy_file":"","key_ring":"%s"}`, validKeyRing)), + configureRequest: configureRequestWithString(fmt.Sprintf(`{"access_key_id":"access_key_id","secret_access_key":"secret_access_key","region":"region","key_identifier_value":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","key_policy_file":"","key_ring":"%s"}`, validKeyRing)), expectMsg: "Key identifier must not be longer than 63 characters", expectCode: codes.InvalidArgument, }, From 7dffcfab062d2cbbcde836a9283ff79e9de512b7 Mon Sep 17 00:00:00 2001 From: Keegan Witt Date: Mon, 1 Jan 2024 17:00:43 -0500 Subject: [PATCH 14/17] Replace deprecated key_metadata_file with key_identifier_file Signed-off-by: Keegan Witt --- conf/server/server_full.conf | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/conf/server/server_full.conf b/conf/server/server_full.conf index 26d65bb0a5..c0d15c84e9 100644 --- a/conf/server/server_full.conf +++ b/conf/server/server_full.conf @@ -269,8 +269,8 @@ plugins { # region: AWS Region to use. # region = "" # - # key_metadata_file: A file path location where information about generated keys will be persisted - # key_metadata_file = "./file_path" + # key_identifier_file: A file path location where information about generated keys will be persisted + # key_identifier_file = "./file_path" # } # } @@ -286,9 +286,9 @@ plugins { # and stores keys in Google Cloud KMS. # KeyManager "gcp_kms" { # plugin_data = { - # # key_metadata_file: A file path location where information about + # # key_identifier_file: A file path location where information about # # generated keys will be persisted. - # key_metadata_file = "./file_path" + # key_identifier_file = "./file_path" # # # key_policy_file: A file path location to a custom IAM Policy (v3) # # in JSON format to be attached to created CryptoKeys. @@ -308,9 +308,9 @@ plugins { # and stores keys in Microsoft Azure Key Vault. # KeyManager "azure_key_vault" { # plugin_data = { - # # key_metadata_file: A file path location where information about + # # key_identifier_file: A file path location where information about # # generated keys will be persisted. - # key_metadata_file = "./file_path" + # key_identifier_file = "./file_path" # # key_vault_uri: The key vault URI where the keys managed by this # # plugin reside. From 8141cbf7f8c6e2133f967c18b0f26fc2de05f85d Mon Sep 17 00:00:00 2001 From: Keegan Witt Date: Mon, 1 Jan 2024 17:02:36 -0500 Subject: [PATCH 15/17] Mark key_metadata_file as not required Signed-off-by: Keegan Witt --- doc/plugin_server_keymanager_aws_kms.md | 18 +++++++++--------- ...plugin_server_keymanager_azure_key_vault.md | 2 +- doc/plugin_server_keymanager_gcp_kms.md | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/doc/plugin_server_keymanager_aws_kms.md b/doc/plugin_server_keymanager_aws_kms.md index b7b70ae748..5110f4dee2 100644 --- a/doc/plugin_server_keymanager_aws_kms.md +++ b/doc/plugin_server_keymanager_aws_kms.md @@ -6,15 +6,15 @@ The `aws_kms` key manager plugin leverages the AWS Key Management Service (KMS) The plugin accepts the following configuration options: -| Key | Type | Required | Description | Default | -|----------------------|--------|---------------------------------------|--------------------------------------------------------------------------------------------|---------------------------------------------------------| -| access_key_id | string | see [AWS KMS Access](#aws-kms-access) | The Access Key Id used to authenticate to KMS | Value of the AWS_ACCESS_KEY_ID environment variable | -| secret_access_key | string | see [AWS KMS Access](#aws-kms-access) | The Secret Access Key used to authenticate to KMS | Value of the AWS_SECRET_ACCESS_KEY environment variable | -| region | string | yes | The region where the keys will be stored | | -| key_metadata_file | string | Required if key_identifier_value is not set | A file path location where information about generated keys will be persisted (deprecated, use key_identifier_file instead) | | -| key_identifier_file | string | Required if key_identifier_value is not set | A file path location where information about generated keys will be persisted | | -| key_identifier_value | string | Required if key_identifier_file is not set | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | | -| key_policy_file | string | no | A file path location to a custom key policy in JSON format | "" | +| Key | Type | Required | Description | Default | +|----------------------|--------|---------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------|---------------------------------------------------------| +| access_key_id | string | see [AWS KMS Access](#aws-kms-access) | The Access Key Id used to authenticate to KMS | Value of the AWS_ACCESS_KEY_ID environment variable | +| secret_access_key | string | see [AWS KMS Access](#aws-kms-access) | The Secret Access Key used to authenticate to KMS | Value of the AWS_SECRET_ACCESS_KEY environment variable | +| region | string | yes | The region where the keys will be stored | | +| key_metadata_file | string | no | A file path location where information about generated keys will be persisted (deprecated, use key_identifier_file instead) | | +| key_identifier_file | string | Required if key_identifier_value is not set | A file path location where information about generated keys will be persisted | | +| key_identifier_value | string | Required if key_identifier_file is not set | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | | +| key_policy_file | string | no | A file path location to a custom key policy in JSON format | "" | ### Alias and Key Management diff --git a/doc/plugin_server_keymanager_azure_key_vault.md b/doc/plugin_server_keymanager_azure_key_vault.md index 005583f3d0..c914481e8c 100644 --- a/doc/plugin_server_keymanager_azure_key_vault.md +++ b/doc/plugin_server_keymanager_azure_key_vault.md @@ -12,7 +12,7 @@ The plugin accepts the following configuration options: | Key | Type | Required | Description | Default | |----------------------|---------|----------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------| -| key_metadata_file | string | yes | A file path location where key metadata used by the plugin will be persisted (deprecated). See "[Management of keys](#management-of-keys)" for more information. | "" | +| key_metadata_file | string | no | A file path location where key metadata used by the plugin will be persisted (deprecated). See "[Management of keys](#management-of-keys)" for more information. | "" | | key_identifier_file | string | yes | A file path location where key metadata used by the plugin will be persisted. See "[Management of keys](#management-of-keys)" for more information. | "" | | key_identifier_value | string | yes | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | "" | | key_vault_uri | string | Yes | The Key Vault URI where the keys managed by this plugin reside. | "" | diff --git a/doc/plugin_server_keymanager_gcp_kms.md b/doc/plugin_server_keymanager_gcp_kms.md index 24d16c53c6..681ea24363 100644 --- a/doc/plugin_server_keymanager_gcp_kms.md +++ b/doc/plugin_server_keymanager_gcp_kms.md @@ -13,7 +13,7 @@ The plugin accepts the following configuration options: | Key | Type | Required | Description | Default | |----------------------|--------|----------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-----------------------------------------------------------------| | key_policy_file | string | no | A file path location to a custom [IAM Policy (v3)](https://cloud.google.com/pubsub/docs/reference/rpc/google.iam.v1#google.iam.v1.Policy) in JSON format to be attached to created CryptoKeys. | "" | -| key_metadata_file | string | yes | A file path location where key metadata used by the plugin will be persisted (deprecated). See "[Management of keys](#management-of-keys)" for more information. | "" | +| key_metadata_file | string | no | A file path location where key metadata used by the plugin will be persisted (deprecated). See "[Management of keys](#management-of-keys)" for more information. | "" | | key_identifier_file | string | yes | A file path location where key metadata used by the plugin will be persisted. See "[Management of keys](#management-of-keys)" for more information. | "" | | key_identifier_value | string | yes | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | "" | | key_ring | string | yes | Resource ID of the key ring where the keys managed by this plugin reside, in the format projects/\*/locations/\*/keyRings/\* | "" | From 09d54e488c9334639daf6acb73d3079068a85728 Mon Sep 17 00:00:00 2001 From: Keegan Witt Date: Mon, 1 Jan 2024 17:07:00 -0500 Subject: [PATCH 16/17] Indicate that one of key_identifier_file and key_identifier_file are required Signed-off-by: Keegan Witt --- ...lugin_server_keymanager_azure_key_vault.md | 22 +++++++++---------- doc/plugin_server_keymanager_gcp_kms.md | 16 +++++++------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/doc/plugin_server_keymanager_azure_key_vault.md b/doc/plugin_server_keymanager_azure_key_vault.md index c914481e8c..972273f319 100644 --- a/doc/plugin_server_keymanager_azure_key_vault.md +++ b/doc/plugin_server_keymanager_azure_key_vault.md @@ -10,17 +10,17 @@ SPIRE. The plugin accepts the following configuration options: -| Key | Type | Required | Description | Default | -|----------------------|---------|----------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------| -| key_metadata_file | string | no | A file path location where key metadata used by the plugin will be persisted (deprecated). See "[Management of keys](#management-of-keys)" for more information. | "" | -| key_identifier_file | string | yes | A file path location where key metadata used by the plugin will be persisted. See "[Management of keys](#management-of-keys)" for more information. | "" | -| key_identifier_value | string | yes | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | "" | -| key_vault_uri | string | Yes | The Key Vault URI where the keys managed by this plugin reside. | "" | -| use_msi | boolean | [Deprecated](#authenticating-to-azure) | Whether or not to use MSI to authenticate to Azure Key Vault. | false | -| subscription_id | string | [Optional](#authenticating-to-azure) | The subscription id. | "" | -| app_id | string | [Optional](#authenticating-to-azure) | The application id. | "" | -| app_secret | string | [Optional](#authenticating-to-azure) | The application secret. | "" | -| tenant_id | string | [Optional](#authenticating-to-azure) | The tenant id. | "" | +| Key | Type | Required | Description | Default | +|----------------------|---------|---------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------| +| key_metadata_file | string | no | A file path location where key metadata used by the plugin will be persisted (deprecated). See "[Management of keys](#management-of-keys)" for more information. | "" | +| key_identifier_file | string | Required if key_identifier_value is not set | A file path location where key metadata used by the plugin will be persisted. See "[Management of keys](#management-of-keys)" for more information. | "" | +| key_identifier_value | string | Required if key_identifier_file is not set | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | "" | +| key_vault_uri | string | Yes | The Key Vault URI where the keys managed by this plugin reside. | "" | +| use_msi | boolean | [Deprecated](#authenticating-to-azure) | Whether or not to use MSI to authenticate to Azure Key Vault. | false | +| subscription_id | string | [Optional](#authenticating-to-azure) | The subscription id. | "" | +| app_id | string | [Optional](#authenticating-to-azure) | The application id. | "" | +| app_secret | string | [Optional](#authenticating-to-azure) | The application secret. | "" | +| tenant_id | string | [Optional](#authenticating-to-azure) | The tenant id. | "" | ### Authenticating to Azure diff --git a/doc/plugin_server_keymanager_gcp_kms.md b/doc/plugin_server_keymanager_gcp_kms.md index 681ea24363..6436c8b208 100644 --- a/doc/plugin_server_keymanager_gcp_kms.md +++ b/doc/plugin_server_keymanager_gcp_kms.md @@ -10,14 +10,14 @@ SPIRE. The plugin accepts the following configuration options: -| Key | Type | Required | Description | Default | -|----------------------|--------|----------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-----------------------------------------------------------------| -| key_policy_file | string | no | A file path location to a custom [IAM Policy (v3)](https://cloud.google.com/pubsub/docs/reference/rpc/google.iam.v1#google.iam.v1.Policy) in JSON format to be attached to created CryptoKeys. | "" | -| key_metadata_file | string | no | A file path location where key metadata used by the plugin will be persisted (deprecated). See "[Management of keys](#management-of-keys)" for more information. | "" | -| key_identifier_file | string | yes | A file path location where key metadata used by the plugin will be persisted. See "[Management of keys](#management-of-keys)" for more information. | "" | -| key_identifier_value | string | yes | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | "" | -| key_ring | string | yes | Resource ID of the key ring where the keys managed by this plugin reside, in the format projects/\*/locations/\*/keyRings/\* | "" | -| service_account_file | string | no | Path to the service account file used to authenticate with the Cloud KMS API. | Value of `GOOGLE_APPLICATION_CREDENTIALS` environment variable. | +| Key | Type | Required | Description | Default | +|----------------------|--------|---------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-----------------------------------------------------------------| +| key_policy_file | string | no | A file path location to a custom [IAM Policy (v3)](https://cloud.google.com/pubsub/docs/reference/rpc/google.iam.v1#google.iam.v1.Policy) in JSON format to be attached to created CryptoKeys. | "" | +| key_metadata_file | string | no | A file path location where key metadata used by the plugin will be persisted (deprecated). See "[Management of keys](#management-of-keys)" for more information. | "" | +| key_identifier_file | string | Required if key_identifier_value is not set | A file path location where key metadata used by the plugin will be persisted. See "[Management of keys](#management-of-keys)" for more information. | "" | +| key_identifier_value | string | Required if key_identifier_file is not set | A static identifier for the SPIRE server instance (used instead of `key_metadata_file`) | "" | +| key_ring | string | yes | Resource ID of the key ring where the keys managed by this plugin reside, in the format projects/\*/locations/\*/keyRings/\* | "" | +| service_account_file | string | no | Path to the service account file used to authenticate with the Cloud KMS API. | Value of `GOOGLE_APPLICATION_CREDENTIALS` environment variable. | ### Authenticating with the Cloud KMS API From c500465e014f0beb9e1ad54769d10a96408f5f51 Mon Sep 17 00:00:00 2001 From: Keegan Witt Date: Mon, 1 Jan 2024 21:04:10 -0500 Subject: [PATCH 17/17] Log that key_metadata_file is deprecated Signed-off-by: Keegan Witt --- pkg/server/plugin/keymanager/gcpkms/gcpkms.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/server/plugin/keymanager/gcpkms/gcpkms.go b/pkg/server/plugin/keymanager/gcpkms/gcpkms.go index d718bed31e..54b5dae697 100644 --- a/pkg/server/plugin/keymanager/gcpkms/gcpkms.go +++ b/pkg/server/plugin/keymanager/gcpkms/gcpkms.go @@ -174,6 +174,7 @@ func (p *Plugin) Configure(ctx context.Context, req *configv1.ConfigureRequest) var serverID = config.KeyIdentifierValue if serverID == "" && config.KeyMetadataFile != "" { + p.log.Warn("'key_metadata_file' is deprecated in favor of 'key_identifier_file' and will be removed in a future version") serverID, err = getOrCreateServerID(config.KeyMetadataFile) if err != nil { return nil, err