Skip to content

feat: enforce kind on admission review #2512

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

Merged
maxsmythe merged 3 commits into from
Jan 18, 2023
Merged

feat: enforce kind on admission review #2512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
37668e8
feat: enforce kind on admisisno review
acpana Jan 12, 2023
1c74cb9
Merge branch 'master' into acpana/enforce-kind
acpana Jan 17, 2023
47ba4ab
Merge branch 'master' into acpana/enforce-kind
acpana Jan 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/gator/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,6 @@ var (
ErrNilOldObject = errors.New("oldObject is nil")
// ErrInvalidYAML indicates that a .yaml/.yml file was not parseable.
ErrInvalidYAML = errors.New("invalid yaml")
// ErrUnmarshallObject happens when the yaml defines an invalid object or oldObject.
ErrUnmarshallObject = errors.New("object or oldObject cannot be unmarshalled")
)
41 changes: 41 additions & 0 deletions pkg/gator/fixtures/fixtures.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,7 @@ kind: AdmissionReview
apiVersion: admission.k8s.io/v1beta1
request:
oldObject:
kind: Pod
labels:
- app: "bar"
`
Expand All @@ -500,13 +501,40 @@ apiVersion: admission.k8s.io/v1beta1
request:
operation: "DELETE"
object:
kind: Pod
labels:
- app: "bar"
`

DeleteAdmissionReviewWithOldObjectMissingKind = `
kind: AdmissionReview
apiVersion: admission.k8s.io/v1beta1
request:
operation: "DELETE"
object:
Copy link
Copy Markdown
Member

@ritazh ritazh Jan 12, 2023

Choose a reason for hiding this comment

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

Do we want to emulate real admission requests for the DELETE operation such that

object is the new object being admitted.
It is null for DELETE operations.

https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#webhook-request-and-response

Copy link
Copy Markdown
Member

@ritazh ritazh Jan 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

When I print input.review.object in the rego after running kubectl delete I definitely see the object and it is not null. This was on a k8s v1.25 cluster. Not sure if the doc is old because the behavior is updated but this was definitely the case before. #144 (comment) very strange.

Copy link
Copy Markdown
Contributor Author

@acpana acpana Jan 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, this is interesting. We specifically decided to overwrite the Object when I/ we worked on this: https://github.com/open-policy-agent/gatekeeper/blob/master/pkg/util/request_validation.go#L15-L32 which was what g8r was doing before too:

  • gatekeeper/pkg/webhook/policy.go

    Lines 148 to 152 in 149fb90

    if err := util.SetObjectOnDelete(&req); err != nil {
    vResp := admission.Denied(err.Error())
    vResp.Result.Code = http.StatusInternalServerError
    return vResp
    }

If we did want to change things around here, I'd be open to opening a new issue and tackling this in a new PR. Just want to keep this one's scope small to kind enforcement. Would that be ok?

Copy link
Copy Markdown
Member

@ritazh ritazh Jan 13, 2023

Choose a reason for hiding this comment

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

LOL looks like I added this in 2019 #146 so it is a GK specific behavior. So for gator, we can assume object is not null then?

Copy link
Copy Markdown
Contributor

@maxsmythe maxsmythe Jan 13, 2023

Choose a reason for hiding this comment

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

IMO the input should match what we expect to receive from the API server (meaning an appropriate test would show object as nil and gator should handle the rewriting to make object == oldObject).

Copy link
Copy Markdown
Contributor Author

@acpana acpana Jan 13, 2023

Choose a reason for hiding this comment

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

an appropriate test would show object as nil and gator should handle the rewriting to make object == oldObject).

could I handle this in a followup PR?

kind: Pod
labels:
- app: "bar"
oldObject:
labels:
- app: "bar"
`
// SystemAdmissionReview holds a request coming from a system user name.
SystemAdmissionReview = `
kind: AdmissionReview
apiVersion: admission.k8s.io/v1beta1
request:
userInfo:
username: "system:foo"
object:
kind: Pod
labels:
- app: "bar"
`

SystemAdmissionReviewMissingKind = `
kind: AdmissionReview
apiVersion: admission.k8s.io/v1beta1
request:
userInfo:
username: "system:foo"
Expand All @@ -523,6 +551,7 @@ request:
userInfo:
username: "foo"
object:
kind: Pod
labels:
- app: "bar"
`
Expand All @@ -541,4 +570,16 @@ apiVersion: constraints.gatekeeper.sh/v1beta1
metadata:
name: always-pass
`

ConstraintAlwaysValidateUserInfoWithMatch = `
kind: ValidateUserInfo
apiVersion: constraints.gatekeeper.sh/v1beta1
metadata:
name: always-pass
spec:
match:
kinds:
- apiGroups: ["*"]
kinds: ["*"]
`
)
14 changes: 14 additions & 0 deletions pkg/gator/verify/runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,20 @@ func (r *Runner) validateAndReviewAdmissionReviewRequest(ctx context.Context, c
return nil, fmt.Errorf("%w: AdmissionRequest does not contain an \"object\" or \"oldObject\" to review", gator.ErrNoObjectForReview)
}

// make sure that kind is set for object, oldObject if present since we expect it on Decode
// https://github.com/kubernetes/apimachinery/blob/27a96d86e70e9bbd40639a0539a14423f55afa07/pkg/apis/meta/v1/unstructured/helpers.go#L341
obj := &unstructured.Unstructured{}
if ar.Request.Object.Raw != nil {
if err := obj.UnmarshalJSON(ar.Request.Object.Raw); err != nil {
return nil, fmt.Errorf("%w: %v", gator.ErrUnmarshallObject, err.Error())
}
}
if ar.Request.OldObject.Raw != nil {
if err := obj.UnmarshalJSON(ar.Request.OldObject.Raw); err != nil {
return nil, fmt.Errorf("%w: %v", gator.ErrUnmarshallObject, err.Error())
}
}

// parse into webhook/admission type
req := &admission.Request{AdmissionRequest: *ar.Request}
if err := util.SetObjectOnDelete(req); err != nil {
Expand Down
33 changes: 33 additions & 0 deletions pkg/gator/verify/runner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1077,6 +1077,23 @@ func TestRunner_Run(t *testing.T) {
},
},
},
{
Name: "invalid admission review request usage", // this test covers error handling for invalid admission review objects
Template: "template.yaml",
Constraint: "constraint-with-match.yaml",
Cases: []*Case{
{
Name: "no kind on object", // this is an AdmissionRequest w a DELETE operation but no oldObject provided
Object: "no-kind-object-ar.yaml",
Assertions: []Assertion{{}},
},
{
Name: "no kind on old object", // this is an AdmissionRequest w a DELETE operation but no oldObject provided
Object: "no-kind-oldObject-ar.yaml",
Assertions: []Assertion{{}},
},
},
},
},
},
f: fstest.MapFS{
Expand All @@ -1086,6 +1103,9 @@ func TestRunner_Run(t *testing.T) {
"constraint.yaml": &fstest.MapFile{
Data: []byte(fixtures.ConstraintAlwaysValidateUserInfo),
},
"constraint-with-match.yaml": &fstest.MapFile{
Data: []byte(fixtures.ConstraintAlwaysValidateUserInfoWithMatch),
},
"no-objects-ar.yaml": &fstest.MapFile{
Data: []byte(fixtures.AdmissionReviewMissingObjectAndOldObject),
},
Expand All @@ -1107,6 +1127,12 @@ func TestRunner_Run(t *testing.T) {
"no-oldObject-ar.yaml": &fstest.MapFile{
Data: []byte(fixtures.DeleteAdmissionReviewWithNoOldObject),
},
"no-kind-object-ar.yaml": &fstest.MapFile{
Data: []byte(fixtures.SystemAdmissionReviewMissingKind),
},
"no-kind-oldObject-ar.yaml": &fstest.MapFile{
Data: []byte(fixtures.DeleteAdmissionReviewWithOldObjectMissingKind),
},
},
want: SuiteResult{
TestResults: []TestResult{
Expand All @@ -1132,6 +1158,13 @@ func TestRunner_Run(t *testing.T) {
{Name: "no oldObject on delete", Error: gator.ErrNilOldObject},
},
},
{
Name: "invalid admission review request usage",
CaseResults: []CaseResult{
{Name: "no kind on object", Error: gator.ErrUnmarshallObject},
{Name: "no kind on old object", Error: gator.ErrUnmarshallObject},
},
},
},
},
},
Expand Down