USHIFT-2454: audit log configuration #3105

copejon · 2024-03-04T19:17:58Z

No description provided.

openshift-ci-robot · 2024-03-04T19:18:01Z

@copejon: This pull request references USHIFT-2454 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

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 openshift-eng/jira-lifecycle-plugin repository.

dhellmann · 2024-03-26T20:00:49Z

pkg/controllers/kube-apiserver.go

-apiVersion: audit.k8s.io/v1
-kind: Policy
-metadata:
-  name: Default


Does this hard-coded policy match the default in the library, or was it hand-crafted?

The generated default policy (sourced from OCP) includes a couple extra rules than the hardcoded policy and which are moot on MicroShift. Including the rules as their defined for an OCP cluster doesn't interfere with the kube-apiserver.

This one would be applied to the apiserver.openshift.io group, which isn't installed on MicroShift.

- level: None namespaces: - "" resources: - group: apiserver.openshift.io resources: - apirequestcounts - apirequestcounts/* users: - system:apiserver

Same for this rule. The user and oauth APIs aren't deployed on MicroShift.

- level: RequestResponse resources: - group: user.openshift.io resources: - identities - group: oauth.openshift.io resources: - oauthaccesstokens - oauthauthorizetokens verbs: - create - update - patch - delete

So the new, generated Policy is superset (with the same values) of this hardcoded Police we're deleting?

Right. The policies are sourced from embedded manifests in the vendored package. It's somewhat simple in that it has a base manifest to which it appends different rules depending on the profile.

pkg/config/apiserver.go

pmtk · 2024-03-27T13:29:58Z

pkg/config/apiserver/audit.go

+	if err != nil {
+		return nil, err
+	}
+	return ap.DeepCopy(), nil


Is DeepCopy necessary? Looks like GetAuditPolicy already copies base policy before editing just to return it.

The way I understood the comment on audit.GetAuditPolicy, the prudent thing to do is deepcopy the returned value:

// Note: the returned policy must not be modifed by the caller prior to a deepcopy.

pkg/config/apiserver.go

pmtk · 2024-03-27T13:58:33Z

pkg/controllers/kube-apiserver.go

-apiVersion: audit.k8s.io/v1
-kind: Policy
-metadata:
-  name: Default


So the new, generated Policy is superset (with the same values) of this hardcoded Police we're deleting?

pmtk · 2024-04-04T10:15:58Z

pkg/config/apiserver.go

 	// The URL and Port of the API server cannot be changed by the user.
 	URL  string `json:"-"`
 	Port int    `json:"-"`
 }
+
+type AuditLog struct {


I would expect adding this would update the default config (packaging/microshift/config.yaml) and configs in docs (docs/user/howto_config.md)

pmtk · 2024-04-04T10:16:38Z

vendor/github.com/openshift/library-go/pkg/crypto/crypto.go

@@ -371,7 +371,7 @@ func (c *TLSCertificateConfig) GetPEMBytes() ([]byte, []byte, error) {
 	if err != nil {
 		return nil, nil, err
 	}
-	keyBytes, err := EncodeKey(c.Key)
+	keyBytes, err := encodeKey(c.Key)


How this change happened without updating go.mod?

good question, let me see

pmtk · 2024-04-05T11:03:01Z

test/suites/standard1/configuration.robot

+    [Documentation]    Setup audit
+    ${merged}=    Extend MicroShift Config    ${AUDIT_PROFILE}
+    Upload MicroShift Config    ${merged}
+    Restart MicroShift


I'm wondering if we should refresh the ${CURSOR} before restarting so that we look at more specific log span? Otherwise it seems to use CURSOR set up at the beginning of this suite. I don't think it functionally changes too much, but you know... correctness :P

I'd done some messing around with that, but decided it belonged in a separate PR (adding new setup/teardown keywords and [setup][teardowns] per test). You're abs right though, it should refresh each test.

test/suites/standard1/configuration.robot

pacevedom · 2024-04-09T19:33:32Z

pkg/controllers/kube-apiserver.go

-  - "RequestReceived"`)
-
+func (s *KubeAPIServer) configureAuditPolicy(cfg *config.Config) error {
+	p, err := apiserver.GetPolicy(cfg.ApiServer.AuditLog.Profile)


Would it be possible to validate this in the config package?

Yes, but I'm doubtful of the value there. Is the idea to catch breaking changes to the OpenShift policies?

You would get the validation also in show-config command, so that any invalid values also signal the error and you dont need to wait until starting microshift.

Ah my bad, I see what you mean (thought you were referring to testing).

I think this is not addressed yet? Otherwise the PR looks pretty good

pkg/config/config.go

copejon · 2024-04-09T21:35:29Z

/test microshift-metal-tests-arm

docs/user/howto_config.md

pmtk · 2024-04-10T06:42:27Z

One q about the generated default config in the docs, but otherwise looks good to me

pkg/config/apiserver/testdata/AllRequestBodies

vendor/github.com/openshift/library-go/pkg/operator/resource/resourceread/config.go

Signed-off-by: Jon Cope <jcope@redhat.com>

… invalid fields and report them as a whole so that repeated executions are not necessary to validate the auditlog config Signed-off-by: Jon Cope <jcope@redhat.com>

add entry to TestValidate table to verify auditLog apiserver flags (maxFiles,maxFileAge,maxFileSize) are checked correctly Signed-off-by: Jon Cope <jcope@redhat.com>

openshift-ci · 2024-04-18T22:20:59Z

@copejon: all tests passed!

Full PR test history. Your PR dashboard.

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.

pacevedom

This is lgtm for me, waiting on others reviews. When done I will label it if its not already there.

pmtk · 2024-04-19T08:34:36Z

/lgtm

openshift-ci · 2024-04-19T08:35:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: copejon, pmtk

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [copejon,pmtk]

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

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 4, 2024

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 4, 2024

openshift-ci bot requested review from jerpeter1 and pmtk March 4, 2024 19:19

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 4, 2024

copejon force-pushed the ushift-2454-audit-log-config branch 3 times, most recently from c8b0bb0 to 8911fcc Compare March 8, 2024 19:11

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 17, 2024

dhellmann reviewed Mar 26, 2024

View reviewed changes

pmtk reviewed Mar 27, 2024

View reviewed changes

copejon force-pushed the ushift-2454-audit-log-config branch from 4ddf35c to 0c9b22b Compare March 28, 2024 15:11

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 28, 2024

copejon changed the title ~~[WIP] USHIFT-2454: audit log configuration~~ USHIFT-2454: audit log configuration Mar 28, 2024

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 28, 2024

copejon force-pushed the ushift-2454-audit-log-config branch 4 times, most recently from e639a65 to a84cd37 Compare April 3, 2024 17:20

pmtk reviewed Apr 4, 2024

View reviewed changes

pmtk reviewed Apr 5, 2024

View reviewed changes

test/suites/standard1/configuration.robot Outdated Show resolved Hide resolved

pacevedom reviewed Apr 9, 2024

View reviewed changes

pmtk reviewed Apr 10, 2024

View reviewed changes

docs/user/howto_config.md Outdated Show resolved Hide resolved

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2024

copejon force-pushed the ushift-2454-audit-log-config branch from 63dc0f8 to dfc7c8c Compare April 11, 2024 20:37

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2024

copejon force-pushed the ushift-2454-audit-log-config branch 2 times, most recently from c454ad4 to 256ca40 Compare April 15, 2024 14:53

pmtk reviewed Apr 15, 2024

View reviewed changes

pkg/config/apiserver/testdata/AllRequestBodies Outdated Show resolved Hide resolved

vendor/github.com/openshift/library-go/pkg/operator/resource/resourceread/config.go Outdated Show resolved Hide resolved

copejon force-pushed the ushift-2454-audit-log-config branch from 256ca40 to f920d82 Compare April 15, 2024 23:07

ushift-2454 - add audit log configuration to microshift config

b008846

Signed-off-by: Jon Cope <jcope@redhat.com>

copejon force-pushed the ushift-2454-audit-log-config branch from f920d82 to b008846 Compare April 16, 2024 19:02

copejon added 2 commits April 18, 2024 15:32

validate auditLog when showConfig is called. concatenate errs for all…

d12f282

… invalid fields and report them as a whole so that repeated executions are not necessary to validate the auditlog config Signed-off-by: Jon Cope <jcope@redhat.com>

tweaked error message format to print 1 error per line

7515741

add entry to TestValidate table to verify auditLog apiserver flags (maxFiles,maxFileAge,maxFileSize) are checked correctly Signed-off-by: Jon Cope <jcope@redhat.com>

pacevedom reviewed Apr 19, 2024

View reviewed changes

openshift-ci bot assigned pmtk Apr 19, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2024

openshift-merge-bot bot merged commit 8376e0b into openshift:main Apr 19, 2024
9 checks passed

copejon deleted the ushift-2454-audit-log-config branch April 19, 2024 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

USHIFT-2454: audit log configuration #3105

USHIFT-2454: audit log configuration #3105

copejon commented Mar 4, 2024

openshift-ci-robot commented Mar 4, 2024 •

edited by openshift-ci bot

Loading

dhellmann Mar 26, 2024

copejon Mar 26, 2024 •

edited

Loading

pmtk Mar 27, 2024

copejon Mar 27, 2024

pmtk Mar 27, 2024

copejon Mar 27, 2024

pmtk Mar 27, 2024

pmtk Apr 4, 2024

pmtk Apr 4, 2024

copejon Apr 5, 2024

pmtk Apr 5, 2024 •

edited

Loading

copejon Apr 5, 2024

pacevedom Apr 9, 2024

copejon Apr 9, 2024

pacevedom Apr 16, 2024

copejon Apr 16, 2024

pmtk Apr 17, 2024

copejon commented Apr 9, 2024

pmtk commented Apr 10, 2024

openshift-ci bot commented Apr 18, 2024

pacevedom left a comment

pmtk commented Apr 19, 2024

openshift-ci bot commented Apr 19, 2024

USHIFT-2454: audit log configuration #3105

USHIFT-2454: audit log configuration #3105

Conversation

copejon commented Mar 4, 2024

openshift-ci-robot commented Mar 4, 2024 • edited by openshift-ci bot Loading

Choose a reason for hiding this comment

copejon Mar 26, 2024 • edited Loading

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

pmtk Apr 5, 2024 • edited Loading

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

copejon commented Apr 9, 2024

pmtk commented Apr 10, 2024

openshift-ci bot commented Apr 18, 2024

pacevedom left a comment

Choose a reason for hiding this comment

pmtk commented Apr 19, 2024

openshift-ci bot commented Apr 19, 2024

openshift-ci-robot commented Mar 4, 2024 •

edited by openshift-ci bot

Loading

copejon Mar 26, 2024 •

edited

Loading

pmtk Apr 5, 2024 •

edited

Loading