-
Notifications
You must be signed in to change notification settings - Fork 8
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
[Security Hardened Kubernetes Cluster] Rule 2005 implementation #380
[Security Hardened Kubernetes Cluster] Rule 2005 implementation #380
Conversation
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.
Just a few change requests.
|
||
type Rule2005 struct { | ||
Client client.Client | ||
Options *Options2005 |
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.
Please ensure that this implements the required Option interface.
} | ||
|
||
type Options2005 struct { | ||
AllowedRepositories []AllowedRepository `json:"allowedRepositories" yaml:"allowedRepositories"` |
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.
AllowedRepositories []AllowedRepository `json:"allowedRepositories" yaml:"allowedRepositories"` | |
AllowedRepositories []AllowedRepository `json:"allowedImages" yaml:"allowedImages"` |
return rule.Result(r, rule.ErroredCheckResult("rule options are missing, but required", nil)), nil | ||
} | ||
|
||
var checkResults []rule.CheckResult |
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.
Please move this closer to its use.
checkResults = append(checkResults, rule.ErroredCheckResult(err.Error(), containerTarget.With("imageRef", imageRef))) | ||
continue | ||
} | ||
imageBase := named.Name() |
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.
Let's also include the version/hash when comparing to prefixes
}) { | ||
checkResults = append(checkResults, rule.PassedCheckResult("Image comes from allowed repository.", containerTarget.With("imageBase", imageBase))) | ||
} else { | ||
checkResults = append(checkResults, rule.FailedCheckResult("Image comes from not allowed repository.", containerTarget.With("imageBase", imageBase))) |
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.
checkResults = append(checkResults, rule.FailedCheckResult("Image comes from not allowed repository.", containerTarget.With("imageBase", imageBase))) | |
checkResults = append(checkResults, rule.FailedCheckResult("Image has not allowed prefix.", containerTarget.With("imageRef", imageRef))) |
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.
Two really small suggestions
|
||
func (r *Rule2005) Run(ctx context.Context) (rule.RuleResult, error) { | ||
if r.Options == nil { | ||
return rule.Result(r, rule.ErroredCheckResult("rule options are missing, but required", nil)), nil |
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.
I think that we should rather report such error earlier. And/Or return a failed result when allowed images are not configured since all images are disallowed. WDYT?
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.
I think it would be best if the ruleset can run without requiring any options to be set. The earlier place to report this error would be when we register the rules.
We can change this check result to Failed and change the message to "There are no configured allowed images."
example/config/managedk8s.yaml
Outdated
# - ruleID: "2005" | ||
# args: | ||
# allowedImages: | ||
# - prefix: "repository.prefix" |
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.
# - prefix: "repository.prefix" | |
# - prefix: "example.foo.repository/organisation/releases/" |
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.
/lgtm
62d4b37
to
061b390
Compare
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.
/lgtm
What this PR does / why we need it:
Which issue(s) this PR fixes:
Part of #356
Special notes for your reviewer:
Release note: