audit policy: reject audit policy files without apiVersion and kind #54267

ericchiang · 2017-10-20T00:32:32Z

/cc @sttts @CaoShuFeng @crassirostris @tallclair

/sig auth
/kind cleanup

Audit policy files without apiVersion and kind are treated as invalid.

CaoShuFeng · 2017-10-20T06:54:56Z

staging/src/k8s.io/apiserver/pkg/audit/policy/reader_test.go

+	defer os.Remove(f)
+
+	_, err = LoadPolicyFromFile(f)
+	require.Error(t, err)


I'd like to replace this line with:

assert.Containsf(t,err.Error(), "policy file contained invalid apiVersion and kind field combination", "a policy without apiVersion or Kind should not be accepted.")

So that we are sure this err is not caused by other reason

CaoShuFeng · 2017-10-20T06:55:31Z

Looks good to me and test OK in my environment.
Thanks.

sttts · 2017-10-20T08:34:17Z

CHANGELOG-1.8.md

@@ -416,7 +416,7 @@ Consider the following changes, limitations, and guidelines before you upgrade:

  * The `--audit-policy-file` option is required if the `AdvancedAudit` feature is not explicitly turned off (`--feature-gates=AdvancedAudit=false`) on the API server.
  * The audit log file defaults to JSON encoding when using the advanced auditing feature gate.
-  * The `--audit-policy-file` option requires `kind` and `apiVersion` fields specifying what format version the `Policy` is using.
+  * An audit policy file without an `apiVersion` or `kind` field may be treated as invalid.


why not is treated as invalid?

#54254 (comment)

I don't understand @liggitt's comment or its context. This PR here treats those files as invalid. Why soft language in the changelog?

In 1.8.0 1.8.1, such policy files are not treated as invalid.

Do we cherry pick this to 1.8?
@sttts

Got it. Do we actually reference this file instead of the 1.8 branch one?

Do we cherry pick this to 1.8?

We should IMO.

Do we actually reference this file instead of the 1.8 branch one?

I don't know.

we don't cherry pick CHANGELOG changes to release branches

sttts · 2017-10-20T08:36:51Z

staging/src/k8s.io/apiserver/pkg/audit/policy/reader.go

 		return nil, fmt.Errorf("failed decoding file %q: %v", filePath, err)
 	}

+	// Ensure the policy file contained an apiVersion and kind.
+	found := false
+	for _, gv := range apiGroupVersions {


make apiGroupVersions a set and the loop will turn into a oneliner check.

Sorry, what's a oneliner check?
Something like this?

var = (a > b) ? a : b

if !apiGroupVersions.Has(schema.GroupVersion{gvk.Group, gvk.Version}.String()) {

sttts · 2017-10-20T08:38:53Z

staging/src/k8s.io/apiserver/pkg/audit/policy/reader.go

+		}
+	}
+	if !found {
+		return nil, fmt.Errorf("policy file contained invalid apiVersion and kind field combination")


this "combination" wording sounds strange. "unknown group version %v in policy file %q"

ericchiang · 2017-10-20T17:30:11Z

Thanks for the reviews. Updated.

crassirostris

LGTM, thanks!

crassirostris · 2017-10-20T20:40:09Z

CHANGELOG-1.8.md

@@ -416,7 +416,7 @@ Consider the following changes, limitations, and guidelines before you upgrade:

  * The `--audit-policy-file` option is required if the `AdvancedAudit` feature is not explicitly turned off (`--feature-gates=AdvancedAudit=false`) on the API server.
  * The audit log file defaults to JSON encoding when using the advanced auditing feature gate.
-  * The `--audit-policy-file` option requires `kind` and `apiVersion` fields specifying what format version the `Policy` is using.
+  * An audit policy file without an `apiVersion` or `kind` field may be treated as invalid.


Small nit: ...without either an apiVersion or a kind field...

sttts · 2017-10-23T07:56:31Z

staging/src/k8s.io/apiserver/pkg/audit/policy/reader_test.go

+
+	_, err = LoadPolicyFromFile(f)
+	assert.Contains(t, err.Error(), "unknown group version field")
+


nit: no empty line

sttts · 2017-10-23T09:39:02Z

/approve

sttts · 2017-10-23T09:39:28Z

Leaving lgtm to @crassirostris when the nits are addressed.

ericchiang · 2017-10-24T05:32:55Z

/test pull-kubernetes-e2e-gce-gpu

crassirostris · 2017-10-24T09:04:05Z

/lgtm

ericchiang · 2017-10-24T15:23:41Z

CHANGELOG-1.8.md

@@ -416,7 +416,7 @@ Consider the following changes, limitations, and guidelines before you upgrade:

  * The `--audit-policy-file` option is required if the `AdvancedAudit` feature is not explicitly turned off (`--feature-gates=AdvancedAudit=false`) on the API server.
  * The audit log file defaults to JSON encoding when using the advanced auditing feature gate.
-  * The `--audit-policy-file` option requires `kind` and `apiVersion` fields specifying what format version the `Policy` is using.
+  * An audit policy file without either an `apiVersion` or a `kind` field may be treated as invalid.


For reviewers. Release note change because the old one was inaccurate. See discussion in #54254

ericchiang · 2017-10-24T15:24:20Z

cc @lavalamp @smarterclayton need a top level OWNER to approve because this PR tweaks the release notes.

ericchiang · 2017-10-30T20:24:49Z

/retest

CaoShuFeng · 2017-10-31T01:13:32Z

/test pull-kubernetes-e2e-gce-gpu

CaoShuFeng · 2017-11-01T08:51:33Z

/test pull-kubernetes-e2e-gce-gpu

CaoShuFeng · 2017-11-02T10:31:33Z

/test pull-kubernetes-e2e-gce-gpu

crassirostris · 2017-11-02T10:46:36Z

/assign @jpbetz
Are you OK with this PR?

CaoShuFeng · 2017-11-06T03:25:36Z

/test pull-kubernetes-e2e-gce-gpu

ericchiang · 2017-11-06T16:47:38Z

(rebased, hope that will help the flaking test)

k8s-ci-robot · 2017-11-06T17:12:24Z

@ericchiang: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-gpu	54f49838581399f616f250c98b66e4391b1b3bff	link	`/test pull-kubernetes-e2e-gce-gpu`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

ericchiang · 2017-11-06T18:14:07Z

Flaking on #54095

/test pull-kubernetes-unit

CaoShuFeng · 2017-11-07T02:06:00Z

/lgtm

sttts · 2017-11-07T07:58:51Z

/approve

crassirostris · 2017-11-07T11:47:41Z

/assign @thockin

Could you please approve?

thockin · 2017-11-09T05:37:49Z

/approve

k8s-github-robot · 2017-11-09T05:38:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CaoShuFeng, crassirostris, ericchiang, sttts, thockin

Associated issue: 54254

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

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [thockin]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-11-09T06:56:34Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from CaoShuFeng, crassirostris, sttts and tallclair October 20, 2017 00:32

k8s-github-robot assigned ncdc and deads2k Oct 20, 2017

ericchiang force-pushed the audit-policy-file-without-kind-or-version branch 2 times, most recently from 52c0e48 to b3baa03 Compare October 20, 2017 00:39

CaoShuFeng reviewed Oct 20, 2017

View reviewed changes

sttts reviewed Oct 20, 2017

View reviewed changes

ericchiang force-pushed the audit-policy-file-without-kind-or-version branch from b3baa03 to baf6be0 Compare October 20, 2017 17:29

crassirostris approved these changes Oct 20, 2017

View reviewed changes

sttts reviewed Oct 23, 2017

View reviewed changes

ericchiang force-pushed the audit-policy-file-without-kind-or-version branch from baf6be0 to 54f4983 Compare October 23, 2017 22:08

k8s-ci-robot assigned crassirostris Oct 24, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2017

ericchiang commented Oct 24, 2017

View reviewed changes

crassirostris mentioned this pull request Oct 25, 2017

Advanced Auditing 1.9 umbrella bug #54551

Closed

14 tasks

k8s-ci-robot assigned jpbetz Nov 2, 2017

ericchiang added 2 commits November 6, 2017 08:46

audit policy: reject audit policy files without apiVersion and kind

fa40bc8

CHANGELOG: loosen language around audit policy file kind and apiVersion

393ac3c

ericchiang force-pushed the audit-policy-file-without-kind-or-version branch from 54f4983 to 393ac3c Compare November 6, 2017 16:47

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2017

k8s-ci-robot assigned CaoShuFeng Nov 7, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2017

k8s-ci-robot assigned thockin Nov 7, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2017

k8s-github-robot merged commit ab44ec9 into kubernetes:master Nov 9, 2017

ericchiang deleted the audit-policy-file-without-kind-or-version branch November 14, 2017 19:38


		_, err = LoadPolicyFromFile(f)
		assert.Contains(t, err.Error(), "unknown group version field")

audit policy: reject audit policy files without apiVersion and kind #54267

audit policy: reject audit policy files without apiVersion and kind #54267

Conversation

ericchiang commented Oct 20, 2017 • edited

Choose a reason for hiding this comment

CaoShuFeng commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaoShuFeng Oct 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang commented Oct 20, 2017

crassirostris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Oct 23, 2017

sttts commented Oct 23, 2017 • edited

ericchiang commented Oct 24, 2017

crassirostris commented Oct 24, 2017

Choose a reason for hiding this comment

ericchiang commented Oct 24, 2017

ericchiang commented Oct 30, 2017

CaoShuFeng commented Oct 31, 2017

CaoShuFeng commented Nov 1, 2017

CaoShuFeng commented Nov 2, 2017

crassirostris commented Nov 2, 2017

CaoShuFeng commented Nov 6, 2017

ericchiang commented Nov 6, 2017

k8s-ci-robot commented Nov 6, 2017 • edited

ericchiang commented Nov 6, 2017

CaoShuFeng commented Nov 7, 2017

sttts commented Nov 7, 2017

crassirostris commented Nov 7, 2017

thockin commented Nov 9, 2017

k8s-github-robot commented Nov 9, 2017

k8s-github-robot commented Nov 9, 2017

ericchiang commented Oct 20, 2017 •

edited

CaoShuFeng Oct 20, 2017 •

edited

sttts commented Oct 23, 2017 •

edited

k8s-ci-robot commented Nov 6, 2017 •

edited