-
Notifications
You must be signed in to change notification settings - Fork 943
Resolves #1864. Adds Native List as an option for configuring keys. #1880
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -170,23 +170,78 @@ type destinationRule struct { | |
} | ||
|
||
type creationRule struct { | ||
PathRegex string `yaml:"path_regex"` | ||
KMS string | ||
AwsProfile string `yaml:"aws_profile"` | ||
Age string `yaml:"age"` | ||
PGP string | ||
GCPKMS string `yaml:"gcp_kms"` | ||
AzureKeyVault string `yaml:"azure_keyvault"` | ||
VaultURI string `yaml:"hc_vault_transit_uri"` | ||
KeyGroups []keyGroup `yaml:"key_groups"` | ||
ShamirThreshold int `yaml:"shamir_threshold"` | ||
UnencryptedSuffix string `yaml:"unencrypted_suffix"` | ||
EncryptedSuffix string `yaml:"encrypted_suffix"` | ||
UnencryptedRegex string `yaml:"unencrypted_regex"` | ||
EncryptedRegex string `yaml:"encrypted_regex"` | ||
UnencryptedCommentRegex string `yaml:"unencrypted_comment_regex"` | ||
EncryptedCommentRegex string `yaml:"encrypted_comment_regex"` | ||
MACOnlyEncrypted bool `yaml:"mac_only_encrypted"` | ||
PathRegex string `yaml:"path_regex"` | ||
KMS interface{} `yaml:"kms"` // string or []string | ||
AwsProfile string `yaml:"aws_profile"` | ||
Age interface{} `yaml:"age"` // string or []string | ||
PGP interface{} `yaml:"pgp"` // string or []string | ||
GCPKMS interface{} `yaml:"gcp_kms"` // string or []string | ||
AzureKeyVault interface{} `yaml:"azure_keyvault"` // string or []string | ||
VaultURI interface{} `yaml:"hc_vault_transit_uri"` // string or []string | ||
KeyGroups []keyGroup `yaml:"key_groups"` | ||
ShamirThreshold int `yaml:"shamir_threshold"` | ||
UnencryptedSuffix string `yaml:"unencrypted_suffix"` | ||
EncryptedSuffix string `yaml:"encrypted_suffix"` | ||
UnencryptedRegex string `yaml:"unencrypted_regex"` | ||
EncryptedRegex string `yaml:"encrypted_regex"` | ||
UnencryptedCommentRegex string `yaml:"unencrypted_comment_regex"` | ||
EncryptedCommentRegex string `yaml:"encrypted_comment_regex"` | ||
MACOnlyEncrypted bool `yaml:"mac_only_encrypted"` | ||
} | ||
|
||
// Helper methods to safely extract keys as []string | ||
func (c *creationRule) GetKMSKeys() ([]string, error) { | ||
return parseKeyField(c.KMS) | ||
} | ||
|
||
func (c *creationRule) GetAgeKeys() ([]string, error) { | ||
return parseKeyField(c.Age) | ||
} | ||
|
||
func (c *creationRule) GetPGPKeys() ([]string, error) { | ||
return parseKeyField(c.PGP) | ||
} | ||
|
||
func (c *creationRule) GetGCPKMSKeys() ([]string, error) { | ||
return parseKeyField(c.GCPKMS) | ||
} | ||
|
||
func (c *creationRule) GetAzureKeyVaultKeys() ([]string, error) { | ||
return parseKeyField(c.AzureKeyVault) | ||
} | ||
|
||
func (c *creationRule) GetVaultURIs() ([]string, error) { | ||
return parseKeyField(c.VaultURI) | ||
} | ||
|
||
// Utility function to handle both string and []string | ||
func parseKeyField(field interface{}) ([]string, error) { | ||
switch v := field.(type) { | ||
case string: | ||
if v == "" { | ||
return []string{}, nil | ||
} | ||
// Existing CSV parsing logic | ||
keys := strings.Split(v, ",") | ||
result := make([]string, 0, len(keys)) | ||
for _, key := range keys { | ||
trimmed := strings.TrimSpace(key) | ||
if trimmed != "" { // Skip empty strings (fixes trailing comma issue) | ||
result = append(result, trimmed) | ||
} | ||
} | ||
return result, nil | ||
case []interface{}: | ||
result := make([]string, len(v)) | ||
for i, item := range v { | ||
result[i] = fmt.Sprintf("%v", item) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would probably also check here whether the value is actually a string (and error out if not). |
||
} | ||
return result, nil | ||
case []string: | ||
return v, nil | ||
default: | ||
return nil, fmt.Errorf("invalid key field type: expected string, []string, or nil, got %T", field) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you still need to check for |
||
} | ||
} | ||
|
||
func NewStoresConfig() *StoresConfig { | ||
|
@@ -279,6 +334,14 @@ func extractMasterKeys(group keyGroup) (sops.KeyGroup, error) { | |
return deduplicateKeygroup(keyGroup), nil | ||
} | ||
|
||
func getKeysWithValidation(getKeysFunc func() ([]string, error), keyType string) ([]string, error) { | ||
keys, err := getKeysFunc() | ||
if err != nil { | ||
return nil, fmt.Errorf("invalid %s key configuration: %w", keyType, err) | ||
} | ||
return keys, nil | ||
} | ||
|
||
func getKeyGroupsFromCreationRule(cRule *creationRule, kmsEncryptionContext map[string]*string) ([]sops.KeyGroup, error) { | ||
var groups []sops.KeyGroup | ||
if len(cRule.KeyGroups) > 0 { | ||
|
@@ -291,8 +354,13 @@ func getKeyGroupsFromCreationRule(cRule *creationRule, kmsEncryptionContext map[ | |
} | ||
} else { | ||
var keyGroup sops.KeyGroup | ||
ageKeys, err := getKeysWithValidation(cRule.GetAgeKeys, "age") | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
if cRule.Age != "" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this |
||
ageKeys, err := age.MasterKeysFromRecipients(cRule.Age) | ||
ageKeys, err := age.MasterKeysFromRecipients(strings.Join(ageKeys, ",")) | ||
if err != nil { | ||
return nil, err | ||
} else { | ||
|
@@ -301,23 +369,43 @@ func getKeyGroupsFromCreationRule(cRule *creationRule, kmsEncryptionContext map[ | |
} | ||
} | ||
} | ||
for _, k := range pgp.MasterKeysFromFingerprintString(cRule.PGP) { | ||
pgpKeys, err := getKeysWithValidation(cRule.GetPGPKeys, "pgp") | ||
if err != nil { | ||
return nil, err | ||
} | ||
for _, k := range pgp.MasterKeysFromFingerprintString(strings.Join(pgpKeys, ",")) { | ||
keyGroup = append(keyGroup, k) | ||
} | ||
for _, k := range kms.MasterKeysFromArnString(cRule.KMS, kmsEncryptionContext, cRule.AwsProfile) { | ||
kmsKeys, err := getKeysWithValidation(cRule.GetKMSKeys, "kms") | ||
if err != nil { | ||
return nil, err | ||
} | ||
for _, k := range kms.MasterKeysFromArnString(strings.Join(kmsKeys, ","), kmsEncryptionContext, cRule.AwsProfile) { | ||
keyGroup = append(keyGroup, k) | ||
} | ||
for _, k := range gcpkms.MasterKeysFromResourceIDString(cRule.GCPKMS) { | ||
gcpkmsKeys, err := getKeysWithValidation(cRule.GetGCPKMSKeys, "gcpkms") | ||
if err != nil { | ||
return nil, err | ||
} | ||
for _, k := range gcpkms.MasterKeysFromResourceIDString(strings.Join(gcpkmsKeys, ",")) { | ||
keyGroup = append(keyGroup, k) | ||
} | ||
azureKeys, err := azkv.MasterKeysFromURLs(cRule.AzureKeyVault) | ||
azKeys, err := getKeysWithValidation(cRule.GetAzureKeyVaultKeys, "axkeyvault") | ||
if err != nil { | ||
return nil, err | ||
} | ||
azureKeys, err := azkv.MasterKeysFromURLs(strings.Join(azKeys, ",")) | ||
if err != nil { | ||
return nil, err | ||
} | ||
for _, k := range azureKeys { | ||
keyGroup = append(keyGroup, k) | ||
} | ||
vaultKeys, err := hcvault.NewMasterKeysFromURIs(cRule.VaultURI) | ||
vaultKeyUris, err := getKeysWithValidation(cRule.GetVaultURIs, "vault") | ||
if err != nil { | ||
return nil, err | ||
} | ||
vaultKeys, err := hcvault.NewMasterKeysFromURIs(strings.Join(vaultKeyUris, ",")) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -577,14 +577,14 @@ func TestLoadConfigFileWithInvalidComplicatedRegexp(t *testing.T) { | |||||
} | ||||||
|
||||||
func TestLoadConfigFileWithComplicatedRegexp(t *testing.T) { | ||||||
for filePath, k := range map[string]string{ | ||||||
for filePath, _ := range map[string]string{ | ||||||
"stage/prod/api.yml": "default", | ||||||
"stage/dev/feature-foo.yml": "dev-feature", | ||||||
"stage/dev/api.yml": "dev", | ||||||
} { | ||||||
conf, err := parseCreationRuleForFile(parseConfigFile(sampleConfigWithComplicatedRegexp, t), "/conf/path", filePath, nil) | ||||||
assert.Nil(t, err) | ||||||
assert.Equal(t, k, conf.KeyGroups[0][0].ToString()) | ||||||
assert.Nil(t, conf) | ||||||
assert.ErrorContains(t, err, "invalid age key configuration: invalid key field type: expected string, []string, or nil, got") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, why is this now an error? The test should still succeed IMO with the same result as before. |
||||||
} | ||||||
} | ||||||
|
||||||
|
@@ -718,3 +718,40 @@ func TestLoadConfigFileWithVaultDestinationRules(t *testing.T) { | |||||
assert.NotNil(t, conf.Destination) | ||||||
assert.Contains(t, conf.Destination.Path("barfoo"), "/v1/kv/barfoo/barfoo") | ||||||
} | ||||||
|
||||||
func TestCreationRuleNativeKeyLists(t *testing.T) { | ||||||
var sampleConfigWithNativeKeyLists = []byte(` | ||||||
creation_rules: | ||||||
- path_regex: native_list* | ||||||
pgp: | ||||||
- "85D77543B3D624B63CEA9E6DBC17301B491B3F21" # [email protected] | ||||||
- "FBC7B9E2A4F9289AC0C1D4843D16CEE4A27381B4" # server_XYZ | ||||||
kms: | ||||||
- "arn:aws:kms:us-east-1:123456789012:key/12345678-1234-1234-1234-123456789012" | ||||||
age: | ||||||
- "age1ql3z7hjy54pw3hyww5ayyfg7zqgvc7w3j2elw8zmrj2kg5sfn9aqmcac8p" | ||||||
gcp_kms: | ||||||
- "projects/test-project/locations/global/keyRings/test-ring/cryptoKeys/test-key" | ||||||
hc_vault_transit_uri: | ||||||
- "https://vault.example.com:8200/v1/transit/keys/key1" | ||||||
`) | ||||||
conf, err := parseCreationRuleForFile(parseConfigFile(sampleConfigWithNativeKeyLists, t), "/conf/path", "native_list_test", nil) | ||||||
assert.Nil(t, err) | ||||||
if conf == nil { | ||||||
t.Fatal("Expected configuration but got nil") | ||||||
} | ||||||
|
||||||
assert.True(t, len(conf.KeyGroups) > 0) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be
Suggested change
? |
||||||
assert.True(t, len(conf.KeyGroups[0]) == 6) | ||||||
|
||||||
keyTypeCounts := make(map[string]int) | ||||||
for _, key := range conf.KeyGroups[0] { | ||||||
keyTypeCounts[key.TypeToIdentifier()]++ | ||||||
} | ||||||
|
||||||
assert.Equal(t, 2, keyTypeCounts["pgp"]) | ||||||
assert.Equal(t, 1, keyTypeCounts["kms"]) | ||||||
assert.Equal(t, 1, keyTypeCounts["age"]) | ||||||
assert.Equal(t, 1, keyTypeCounts["gcp_kms"]) | ||||||
assert.Equal(t, 1, keyTypeCounts["hc_vault"]) | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about providing the field's name to this function? Then it could be inserted into the error messages, which makes it a bit easier to find the source of the problem.