Skip to content

Commit 4f5d717

Browse files
wprosswpross
authored
Allow mixed keys in provider and dns secrets (#1667)
* Allow mixing standard and DNS specific keys in both, infrastructure and DNS secrets * Ensure that duplicate keys for the same field are not present
1 parent ba28963 commit 4f5d717

2 files changed

Lines changed: 84 additions & 66 deletions

File tree

‎pkg/apis/aws/validation/secrets.go‎

Lines changed: 49 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ var supportedSecretKinds = []string{
2929
string(SecretKindDns),
3030
}
3131

32-
// ValidateCloudProviderSecret checks whether the given secret contains valid AWS access keys.
32+
// ValidateCloudProviderSecret checks whether the given secret contains valid AWS credentials
3333
func ValidateCloudProviderSecret(secret *corev1.Secret, fldPath *field.Path, kind SecretKind) field.ErrorList {
3434
allErrs := field.ErrorList{}
3535
dataPath := fldPath.Child("data")
@@ -39,36 +39,61 @@ func ValidateCloudProviderSecret(secret *corev1.Secret, fldPath *field.Path, kin
3939
var accessKeyID, secretAccessKey, region []byte
4040
var accessKeyIDExists, secretAccessKeyExists, regionExists bool
4141

42-
switch kind {
43-
case SecretKindInfrastructure:
42+
// Check for duplicate keys (both standard and DNS-specific for the same field)
43+
_, hasStandardAccessKey := secret.Data[aws.AccessKeyID]
44+
_, hasDNSAccessKey := secret.Data[aws.DNSAccessKeyID]
45+
_, hasStandardSecretKey := secret.Data[aws.SecretAccessKey]
46+
_, hasDNSSecretKey := secret.Data[aws.DNSSecretAccessKey]
47+
48+
if hasStandardAccessKey && hasDNSAccessKey {
49+
allErrs = append(allErrs, field.Invalid(dataPath, "(multiple keys)",
50+
fmt.Sprintf("cannot have both %q and %q in secret %s", aws.AccessKeyID, aws.DNSAccessKeyID, secretRef)))
51+
}
52+
53+
if hasStandardSecretKey && hasDNSSecretKey {
54+
allErrs = append(allErrs, field.Invalid(dataPath, "(multiple keys)",
55+
fmt.Sprintf("cannot have both %q and %q in secret %s", aws.SecretAccessKey, aws.DNSSecretAccessKey, secretRef)))
56+
}
57+
58+
// Check for DNS-specific keys first, then fall back to standard keys
59+
accessKeyID, accessKeyIDExists = secret.Data[aws.DNSAccessKeyID]
60+
if accessKeyIDExists {
61+
accessKeyIDKey = aws.DNSAccessKeyID
62+
} else {
63+
accessKeyID, accessKeyIDExists = secret.Data[aws.AccessKeyID]
4464
accessKeyIDKey = aws.AccessKeyID
65+
}
66+
67+
secretAccessKey, secretAccessKeyExists = secret.Data[aws.DNSSecretAccessKey]
68+
if secretAccessKeyExists {
69+
secretAccessKeyKey = aws.DNSSecretAccessKey
70+
} else {
71+
secretAccessKey, secretAccessKeyExists = secret.Data[aws.SecretAccessKey]
4572
secretAccessKeyKey = aws.SecretAccessKey
46-
accessKeyID, accessKeyIDExists = secret.Data[accessKeyIDKey]
47-
secretAccessKey, secretAccessKeyExists = secret.Data[secretAccessKeyKey]
73+
}
4874

49-
// Validate no unexpected keys exist
75+
switch kind {
76+
case SecretKindInfrastructure:
77+
// Allow both standard and DNS-specific keys
5078
allErrs = append(allErrs, validateNoUnexpectedKeys(secret.Data, dataPath, secretRef,
51-
aws.AccessKeyID, aws.SecretAccessKey)...)
79+
aws.AccessKeyID, aws.SecretAccessKey,
80+
aws.DNSAccessKeyID, aws.DNSSecretAccessKey)...)
5281

5382
case SecretKindDns:
54-
// For DNS secrets, check for DNS-specific key aliases first, then fall back to
55-
// standard infrastructure keys
56-
accessKeyID, accessKeyIDExists = secret.Data[aws.DNSAccessKeyID]
57-
if accessKeyIDExists {
58-
accessKeyIDKey = aws.DNSAccessKeyID
59-
} else {
60-
accessKeyID, accessKeyIDExists = secret.Data[aws.AccessKeyID]
61-
accessKeyIDKey = aws.AccessKeyID
62-
}
83+
// Check for duplicate region keys
84+
_, hasStandardRegion := secret.Data[aws.Region]
85+
_, hasDNSRegion := secret.Data[aws.DNSRegion]
6386

64-
secretAccessKey, secretAccessKeyExists = secret.Data[aws.DNSSecretAccessKey]
65-
if secretAccessKeyExists {
66-
secretAccessKeyKey = aws.DNSSecretAccessKey
67-
} else {
68-
secretAccessKey, secretAccessKeyExists = secret.Data[aws.SecretAccessKey]
69-
secretAccessKeyKey = aws.SecretAccessKey
87+
if hasStandardRegion && hasDNSRegion {
88+
allErrs = append(allErrs, field.Invalid(dataPath, "(multiple keys)",
89+
fmt.Sprintf("cannot have both %q and %q in secret %s", aws.Region, aws.DNSRegion, secretRef)))
7090
}
7191

92+
// Allow both standard and DNS-specific keys
93+
allErrs = append(allErrs, validateNoUnexpectedKeys(secret.Data, dataPath, secretRef,
94+
aws.AccessKeyID, aws.SecretAccessKey, aws.Region,
95+
aws.DNSAccessKeyID, aws.DNSSecretAccessKey, aws.DNSRegion)...)
96+
7297
region, regionExists = secret.Data[aws.DNSRegion]
7398
if regionExists {
7499
regionKey = aws.DNSRegion
@@ -77,19 +102,8 @@ func ValidateCloudProviderSecret(secret *corev1.Secret, fldPath *field.Path, kin
77102
regionKey = aws.Region
78103
}
79104

80-
// Validate no unexpected keys exist
81-
// For DNS, we allow either the standard infrastructure keys or the DNS-specific alias keys, but not a mix
82-
// Prefer standard keys if any are present
83-
_, hasStandardAccessKey := secret.Data[aws.AccessKeyID]
84-
_, hasStandardSecretKey := secret.Data[aws.SecretAccessKey]
85-
_, hasStandardRegionKey := secret.Data[aws.Region]
86-
87-
if hasStandardAccessKey || hasStandardSecretKey || hasStandardRegionKey {
88-
allErrs = append(allErrs, validateNoUnexpectedKeys(secret.Data, dataPath, secretRef,
89-
aws.AccessKeyID, aws.SecretAccessKey, aws.Region)...)
90-
} else {
91-
allErrs = append(allErrs, validateNoUnexpectedKeys(secret.Data, dataPath, secretRef,
92-
aws.DNSAccessKeyID, aws.DNSSecretAccessKey, aws.DNSRegion)...)
105+
if regionExists && len(region) > 0 {
106+
allErrs = append(allErrs, validateRegion(string(region), dataPath.Key(regionKey))...)
93107
}
94108

95109
default:
@@ -120,11 +134,6 @@ func ValidateCloudProviderSecret(secret *corev1.Secret, fldPath *field.Path, kin
120134
allErrs = append(allErrs, validateSecretAccessKey(string(secretAccessKey), dataPath.Key(secretAccessKeyKey))...)
121135
}
122136

123-
// Validate region
124-
if regionExists && len(region) > 0 {
125-
allErrs = append(allErrs, validateRegion(string(region), dataPath.Key(regionKey))...)
126-
}
127-
128137
return allErrs
129138
}
130139

‎pkg/apis/aws/validation/secrets_test.go‎

Lines changed: 35 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -471,48 +471,53 @@ var _ = Describe("Secret validation", func() {
471471
})
472472

473473
Context("with mixed keys", func() {
474-
It("should fail when both standard and DNS alias keys are present for accessKeyID", func() {
474+
It("should pass when mixing standard accessKeyID with DNS secretAccessKey", func() {
475+
secret.Data = map[string][]byte{
476+
aws.AccessKeyID: []byte(validAccessKeyID),
477+
aws.DNSSecretAccessKey: []byte(validSecretAccessKey),
478+
}
479+
480+
errs := ValidateCloudProviderSecret(secret, fldPath, SecretKindDns)
481+
Expect(errs).To(BeEmpty())
482+
})
483+
484+
It("should pass when mixing DNS accessKeyID with standard secretAccessKey", func() {
475485
secret.Data = map[string][]byte{
476-
aws.AccessKeyID: []byte(validAccessKeyID),
477486
aws.DNSAccessKeyID: []byte(validAccessKeyID),
478487
aws.SecretAccessKey: []byte(validSecretAccessKey),
479488
}
480489

481490
errs := ValidateCloudProviderSecret(secret, fldPath, SecretKindDns)
482-
Expect(errs).To(ContainElement(PointTo(MatchFields(IgnoreExtras, Fields{
483-
"Type": Equal(field.ErrorTypeForbidden),
484-
"Field": Equal("secret.data[AWS_ACCESS_KEY_ID]"),
485-
}))))
491+
Expect(errs).To(BeEmpty())
486492
})
487493

488-
It("should fail when both standard and DNS alias keys are present for secretAccessKey", func() {
494+
It("should fail when both standard and DNS accessKeyID are present", func() {
489495
secret.Data = map[string][]byte{
490-
aws.AccessKeyID: []byte(validAccessKeyID),
491-
aws.SecretAccessKey: []byte(validSecretAccessKey),
492-
aws.DNSSecretAccessKey: []byte(validSecretAccessKey),
496+
aws.AccessKeyID: []byte(validAccessKeyID),
497+
aws.DNSAccessKeyID: []byte(validAccessKeyID),
498+
aws.SecretAccessKey: []byte(validSecretAccessKey),
493499
}
494500

495501
errs := ValidateCloudProviderSecret(secret, fldPath, SecretKindDns)
496502
Expect(errs).To(ContainElement(PointTo(MatchFields(IgnoreExtras, Fields{
497-
"Type": Equal(field.ErrorTypeForbidden),
498-
"Field": Equal("secret.data[AWS_SECRET_ACCESS_KEY]"),
503+
"Type": Equal(field.ErrorTypeInvalid),
499504
}))))
500505
})
501506

502-
It("should fail when mixing one standard key with one DNS alias key", func() {
507+
It("should fail when both standard and DNS secretAccessKey are present", func() {
503508
secret.Data = map[string][]byte{
504509
aws.AccessKeyID: []byte(validAccessKeyID),
510+
aws.SecretAccessKey: []byte(validSecretAccessKey),
505511
aws.DNSSecretAccessKey: []byte(validSecretAccessKey),
506512
}
507513

508514
errs := ValidateCloudProviderSecret(secret, fldPath, SecretKindDns)
509515
Expect(errs).To(ContainElement(PointTo(MatchFields(IgnoreExtras, Fields{
510-
"Type": Equal(field.ErrorTypeForbidden),
511-
"Field": Equal("secret.data[AWS_SECRET_ACCESS_KEY]"),
516+
"Type": Equal(field.ErrorTypeInvalid),
512517
}))))
513518
})
514519

515-
It("should fail when all four keys are present", func() {
520+
It("should fail when all four credential keys are present", func() {
516521
secret.Data = map[string][]byte{
517522
aws.AccessKeyID: []byte(validAccessKeyID),
518523
aws.DNSAccessKeyID: []byte(validAccessKeyID),
@@ -522,16 +527,10 @@ var _ = Describe("Secret validation", func() {
522527

523528
errs := ValidateCloudProviderSecret(secret, fldPath, SecretKindDns)
524529
Expect(errs).To(Not(BeEmpty()))
525-
Expect(errs).To(ContainElement(PointTo(MatchFields(IgnoreExtras, Fields{
526-
"Type": Equal(field.ErrorTypeForbidden),
527-
"Field": Equal("secret.data[AWS_ACCESS_KEY_ID]"),
528-
}))))
529-
Expect(errs).To(ContainElement(PointTo(MatchFields(IgnoreExtras, Fields{
530-
"Type": Equal(field.ErrorTypeForbidden),
531-
"Field": Equal("secret.data[AWS_SECRET_ACCESS_KEY]"),
532-
}))))
530+
Expect(errs).To(HaveLen(2)) // Two duplicate errors
533531
})
534532
})
533+
535534
Context("with region field", func() {
536535
It("should pass with valid AWS_REGION", func() {
537536
secret.Data = map[string][]byte{
@@ -577,6 +576,17 @@ var _ = Describe("Secret validation", func() {
577576
Expect(errs).To(BeEmpty())
578577
})
579578

579+
It("should pass with valid EU sovereign cloud region", func() {
580+
secret.Data = map[string][]byte{
581+
aws.DNSAccessKeyID: []byte(validAccessKeyID),
582+
aws.DNSSecretAccessKey: []byte(validSecretAccessKey),
583+
aws.DNSRegion: []byte("eusc-de-east-1"),
584+
}
585+
586+
errs := ValidateCloudProviderSecret(secret, fldPath, SecretKindDns)
587+
Expect(errs).To(BeEmpty())
588+
})
589+
580590
It("should pass when region field is empty", func() {
581591
secret.Data = map[string][]byte{
582592
aws.DNSAccessKeyID: []byte(validAccessKeyID),
@@ -678,8 +688,7 @@ var _ = Describe("Secret validation", func() {
678688

679689
errs := ValidateCloudProviderSecret(secret, fldPath, SecretKindDns)
680690
Expect(errs).To(ContainElement(PointTo(MatchFields(IgnoreExtras, Fields{
681-
"Type": Equal(field.ErrorTypeForbidden),
682-
"Field": Equal("secret.data[AWS_REGION]"),
691+
"Type": Equal(field.ErrorTypeInvalid),
683692
}))))
684693
})
685694
})

0 commit comments

Comments
 (0)