Skip to content

Add DNS secret validation for AWS shoots#1612

Merged
wpross merged 7 commits intogardener:masterfrom
wpross:dnssec-val
Jan 14, 2026
Merged

Add DNS secret validation for AWS shoots#1612
wpross merged 7 commits intogardener:masterfrom
wpross:dnssec-val

Conversation

@wpross
Copy link
Copy Markdown
Contributor

@wpross wpross commented Dec 19, 2025

  • Validate aws-dns provider secrets in shoot spec
  • Refactor secret validation to use field.Path
  • Refactor shoot validator to return all field errors
  • Update tests accordingly

How to categorize this PR?

/area security
/kind enhancement
/platform aws

What this PR does / why we need it:
This PR adds input validation for DNS provider secrets for the aws-dns provider added in the shoots spec

...
  dns:
    domain: crazy-botany.core.my-custom-domain.com
    # Provider configuration required if custom shoot domain is configured.
    providers:
    - type: aws-dns
      secretName: my-custom-domain-secret
...

It complements PR1479

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

Add input validation for DNS provider secrets referenced in the shoot spec.

@gardener-robot gardener-robot added area/security Security related kind/enhancement Enhancement, improvement, extension platform/aws Amazon web services platform/infrastructure needs/review Needs review size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs/second-opinion Needs second review by someone else labels Dec 19, 2025
@ghost ghost added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 19, 2025
@wpross
Copy link
Copy Markdown
Contributor Author

wpross commented Dec 19, 2025

Created it as a draft since I haven't tested it yet.

@github-actions github-actions Bot added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 19, 2025
@ghost ghost added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 19, 2025
@github-actions github-actions Bot removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 19, 2025
@ghost ghost added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 19, 2025
@github-actions github-actions Bot removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 19, 2025
@ghost ghost added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 19, 2025
@github-actions github-actions Bot removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 19, 2025
@ghost ghost added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 19, 2025
@github-actions github-actions Bot removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 19, 2025
@gardener-ci-robot
Copy link
Copy Markdown
Contributor

The Gardener project currently lacks enough active contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:

  • After 15d of inactivity, lifecycle/stale is applied
  • After 15d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 7d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Mark this PR as rotten with /lifecycle rotten
  • Close this PR with /close

/lifecycle stale

@gardener-robot gardener-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 3, 2026
@wpross
Copy link
Copy Markdown
Contributor Author

wpross commented Jan 12, 2026

/remove-lifecycle stale

@gardener-robot
Copy link
Copy Markdown

@wpross Command /remove-lifecycle is not known.

@wpross
Copy link
Copy Markdown
Contributor Author

wpross commented Jan 12, 2026

/remove-lifecycle-stale

@gardener-robot
Copy link
Copy Markdown

@wpross Command /remove-lifecycle-stale is not known.

@wpross wpross removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 12, 2026
@wpross wpross marked this pull request as ready for review January 12, 2026 13:13
@wpross wpross requested a review from a team as a code owner January 12, 2026 13:13
@ghost ghost added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 12, 2026
@github-actions github-actions Bot removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 12, 2026
@ghost ghost added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 12, 2026
@github-actions github-actions Bot removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 12, 2026
@AndreasBurger AndreasBurger added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 13, 2026
@github-actions github-actions Bot removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 13, 2026
Copy link
Copy Markdown
Member

@AndreasBurger AndreasBurger left a comment

Choose a reason for hiding this comment

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

Some remarks, generally lgtm

Comment thread pkg/admission/validator/shoot.go Outdated
return allErrs
}

providersPath := specPath.Child("dns").Child("providers")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could also be moved to the initial declaration-block for consistency

Comment thread pkg/apis/aws/validation/secrets.go Outdated
Comment on lines +84 to +85
fmt.Sprintf("unsupported secret kind for secret %s, allowed values are %q and %q",
secretRef, SecretKindInfrastructure, SecretKindDns)),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thats fine (great, even) if we don't expect to extend the kinds. If we do, I'd suggest to maintain a list of known kinds (somewhere around l.20) that can be the source of truth for known kinds. Higher likelihood of remembering to update that, in case of changes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, there seems to be a field.NotSupported for these cases

Comment thread pkg/apis/aws/validation/secrets.go Outdated
allErrs = append(allErrs, field.Required(dataPath.Key(accessKeyIDKey),
fmt.Sprintf("missing required field %q in secret %s", accessKeyIDKey, secretRef)))
} else if len(accessKeyID) == 0 {
allErrs = append(allErrs, field.Invalid(dataPath.Key(accessKeyIDKey), "",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this should also be field.Required

Comment thread pkg/apis/aws/validation/secrets.go Outdated
allErrs = append(allErrs, field.Required(dataPath.Key(secretAccessKeyKey),
fmt.Sprintf("missing required field %q in secret %s", secretAccessKeyKey, secretRef)))
} else if len(secretAccessKey) == 0 {
allErrs = append(allErrs, field.Invalid(dataPath.Key(secretAccessKeyKey), "",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(possibly) field.Required

Comment thread pkg/admission/validator/shoot.go
@gardener-prow gardener-prow Bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 13, 2026
@ghost ghost added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 13, 2026
@github-actions github-actions Bot removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 13, 2026
@github-actions
Copy link
Copy Markdown
Contributor

This change enhances the AWS provider's secret validation system by implementing structured field-level validation with improved error handling and security measures. The update transitions from simple string-based error returns to a comprehensive field-based validation approach using Kubernetes' validation framework.

Walkthrough

  • Refactor: Updated secret validation functions to use field.ErrorList instead of simple errors, providing structured field-path-based validation messages
  • Refactor: Consolidated validation logic by removing code duplication across credentialsbinding, secretbinding, and secrets validators
  • New Feature: Added DNS provider secret validation to shoot validation, supporting both standard and DNS-specific credential keys
  • Refactor: Enhanced secret validation with stricter AWS credential format requirements (exact 20-character access keys, 40-character secret keys with proper character sets)
  • Refactor: Implemented sensitive value hiding in validation error messages to prevent credential exposure in logs
  • Test: Updated test suites with comprehensive validation scenarios using realistic AWS credential formats and proper mock configurations

Model: claude-sonnet-4-20250514 | Prompt Tokens: 21173 | Completion Tokens: 226

@ghost ghost added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 13, 2026
@github-actions github-actions Bot removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 13, 2026
@gardener-prow
Copy link
Copy Markdown

gardener-prow Bot commented Jan 14, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AndreasBurger, wpross

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [AndreasBurger,wpross]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@wpross wpross merged commit e2518ee into gardener:master Jan 14, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/security Security related cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review needs/second-opinion Needs second review by someone else platform/aws Amazon web services platform/infrastructure size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants