[Security Hardened Shoot Cluster] Rule 1003 implementation #454

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Merged

AleksandarSavchev merged 5 commits into gardener:main from AleksandarSavchev:add-rule-1003-shs

Mar 25, 2025

Member

AleksandarSavchev commented Mar 20, 2025

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #431

Special notes for your reviewer:

Release note:

Implementation for rule `1003` from the `security-hardened-shoot-cluster` ruleset for provider `garden`.

AleksandarSavchev requested a review from a team as a code owner

March 20, 2025 14:08

gardener-robot added needs/review size/l needs/second-opinion labels

gardener-robot-ci-1 added reviewed/ok-to-test needs/ok-to-test and removed reviewed/ok-to-test labels

dimityrmirchev requested changes

View reviewed changes

Member

dimityrmirchev left a comment

Thanks. I have some suggestions

docs/rulesets/security-hardened-shoot-cluster/ruleset.md

@@ @@ -44,7 +44,13 @@ spec: @@
                           version: <supported-version>
               ```
-              The supported versions can be found in the used `CloudProfile`.

Member

dimityrmirchev Mar 21, 2025

Why is this line removed?

Member Author

AleksandarSavchev Mar 24, 2025

Must have written on top of it 😅 my bad

docs/rulesets/security-hardened-shoot-cluster/ruleset.md Outdated

@@ @@ -44,7 +44,13 @@ spec: @@
                           version: <supported-version>
               ```
-              The supported versions can be found in the used `CloudProfile`.
+              ### 1003 - Shoot clusters should have Lakom extension configured correctly. <a id="1003"></a>

Member

dimityrmirchev Mar 21, 2025

Suggested change

      
            ### 1003 - Shoot clusters should have Lakom extension configured correctly. <a id="1003"></a>
          
            ### 1003 - Shoot clusters should have the Lakom extension configured. <a id="1003"></a>

Member

dimityrmirchev Mar 21, 2025

Would it be better to use must instead of should when severity is ranked HIGH?

docs/rulesets/security-hardened-shoot-cluster/ruleset.md Outdated

+              ### 1003 - Shoot clusters should have Lakom extension configured correctly. <a id="1003"></a>
+              #### Description
+              Shoot clusters should have Lakom extension configured correctly. Trusted public keys should be configured for the Lakom extension.

Member

dimityrmirchev Mar 21, 2025

Suggested change

      
            Shoot clusters should have Lakom extension configured correctly. Trusted public keys should be configured for the Lakom extension.
          
            Lakom is an admission controller which implements image signature verification. Shoot clusters should have the Lakom extension configured with trusted public keys so that only trusted images are allowed in the cluster.

docs/rulesets/security-hardened-shoot-cluster/ruleset.md Outdated

+              Shoot clusters should have Lakom extension configured correctly. Trusted public keys should be configured for the Lakom extension.
+              #### Fix
+              Follow the Lakom extension documentation on how to configure [TrustedKeysResourceName](https://github.com/gardener/gardener-extension-shoot-lakom-service/blob/v0.18.1/docs/usage/shoot-extension.md#trustedkeysresourcename)

Member

dimityrmirchev Mar 21, 2025

Suggested change

      
            Follow the Lakom extension documentation on how to configure [TrustedKeysResourceName](https://github.com/gardener/gardener-extension-shoot-lakom-service/blob/v0.18.1/docs/usage/shoot-extension.md#trustedkeysresourcename)
          
            Follow the Lakom extension documentation on how to configure [TrustedKeysResourceName](https://github.com/gardener/gardener-extension-shoot-lakom-service/blob/v0.18.1/docs/usage/shoot-extension.md#trustedkeysresourcename).

pkg/provider/garden/ruleset/securityhardenedshoot/rules/1003.go Outdated

+              }
+              func (r *Rule1003) Severity() rule.SeverityLevel {
+              	return rule.SeverityMedium

Member

dimityrmirchev Mar 21, 2025

You set this to MEDIUM, but the documentation says HIGH?

pkg/provider/garden/ruleset/securityhardenedshoot/rules/1003.go Outdated

+              		}
+              		if lakomConfig.TrustedKeysResourceName == nil || len(*lakomConfig.TrustedKeysResourceName) == 0 {
+              			return rule.Result(r, rule.FailedCheckResult(fmt.Sprintf("Extension %s does not have TrustedKeysResourceName configured.", lakomExtensionType), rule.NewTarget())), nil

Member

dimityrmirchev Mar 21, 2025

Suggested change

      
            			return rule.Result(r, rule.FailedCheckResult(fmt.Sprintf("Extension %s does not have TrustedKeysResourceName configured.", lakomExtensionType), rule.NewTarget())), nil
          
            			return rule.Result(r, rule.FailedCheckResult(fmt.Sprintf("Extension %s does not configure trusted keys.", lakomExtensionType), rule.NewTarget())), nil

pkg/provider/garden/ruleset/securityhardenedshoot/rules/1003.go Outdated

+              		if lakomConfig.TrustedKeysResourceName == nil || len(*lakomConfig.TrustedKeysResourceName) == 0 {
+              			return rule.Result(r, rule.FailedCheckResult(fmt.Sprintf("Extension %s does not have TrustedKeysResourceName configured.", lakomExtensionType), rule.NewTarget())), nil
+              		} else {

Member

dimityrmirchev Mar 21, 2025

No need for the else. It causes not needed nesting

pkg/provider/garden/ruleset/securityhardenedshoot/rules/1003.go Outdated

+              			return rule.Result(r, rule.PassedCheckResult(fmt.Sprintf("Extension %s is correctly configured for the shoot cluster.", lakomExtensionType), rule.NewTarget())), nil
+              		}
+              	case extensionLabelValue == "true" && !extensionDisabled:
+              		return rule.Result(r, rule.FailedCheckResult(fmt.Sprintf("Extension %s does not have extension config configured for the shoot cluster.", lakomExtensionType), rule.NewTarget())), nil

Member

dimityrmirchev Mar 21, 2025

Suggested change

      
            		return rule.Result(r, rule.FailedCheckResult(fmt.Sprintf("Extension %s does not have extension config configured for the shoot cluster.", lakomExtensionType), rule.NewTarget())), nil
          
            		return rule.Result(r, rule.FailedCheckResult(fmt.Sprintf("Extension %s is not configured for the shoot cluster.", lakomExtensionType), rule.NewTarget())), nil

pkg/provider/garden/ruleset/securityhardenedshoot/rules/1003.go Outdated

+              	case extensionLabelValue == "true" && !extensionDisabled:
+              		return rule.Result(r, rule.FailedCheckResult(fmt.Sprintf("Extension %s does not have extension config configured for the shoot cluster.", lakomExtensionType), rule.NewTarget())), nil
+              	case extensionLabelValue == "true" && extensionDisabled:
+              		return rule.Result(r, rule.FailedCheckResult(fmt.Sprintf("Extension %s is disabled in the shoot spec and enabled in labels.", lakomExtensionType), rule.NewTarget())), nil

Member

dimityrmirchev Mar 21, 2025

Should this be a warning? What can cause this behaviour?

Member Author

AleksandarSavchev Mar 24, 2025

It does make sense for this to not be marked as failed as we should never enter this case.

pkg/provider/garden/ruleset/securityhardenedshoot/rules/1003.go Outdated

+              	case extensionLabelValue == "true" && extensionDisabled:
+              		return rule.Result(r, rule.FailedCheckResult(fmt.Sprintf("Extension %s is disabled in the shoot spec and enabled in labels.", lakomExtensionType), rule.NewTarget())), nil
+              	default:
+              		return rule.Result(r, rule.FailedCheckResult(fmt.Sprintf("Extension %s has unexpected label value: %s.", lakomExtensionType, extensionLabelValue), rule.NewTarget())), nil

Member

dimityrmirchev Mar 21, 2025

Should this be a warning?

Member Author

AleksandarSavchev Mar 24, 2025

I have changed this one to warning now and added the changes to rule 1000 as well.

gardener-robot added the needs/changes label

gardener-robot-ci-1 added reviewed/ok-to-test and removed reviewed/ok-to-test labels

AleksandarSavchev force-pushed the add-rule-1003-shs branch from 1edd3eb to ac3e652 Compare

March 24, 2025 11:40

gardener-robot-ci-3 added reviewed/ok-to-test and removed reviewed/ok-to-test labels

AleksandarSavchev added 5 commits

March 25, 2025 10:38


          Add rule 1003 implementation

82d1172


          Add rule 1003 to security hardened shoot guide

d9143dc


          Add suggested changes

9811f38


          Warn in rule 1000

c8bb66e


          Fix name of rule 1002

2e8287a

AleksandarSavchev force-pushed the add-rule-1003-shs branch from ac3e652 to 2e8287a Compare

March 25, 2025 08:40

gardener-robot-ci-3 added reviewed/ok-to-test and removed reviewed/ok-to-test labels

dimityrmirchev approved these changes

View reviewed changes

Member

dimityrmirchev left a comment

/lgtm

gardener-robot added reviewed/lgtm and removed needs/changes needs/review needs/second-opinion labels

gardener-robot-ci-2 added the reviewed/ok-to-test label

AleksandarSavchev merged commit 5a5b919 into gardener:main

9 checks passed

gardener-robot added the status/closed label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs/ok-to-test reviewed/lgtm reviewed/ok-to-test size/l status/closed