Skip to content
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

Advanced Auditing 1.12 umbrella bug #65266

Closed
5 of 11 tasks
x13n opened this issue Jun 20, 2018 · 36 comments · Fixed by #65891
Closed
5 of 11 tasks

Advanced Auditing 1.12 umbrella bug #65266

x13n opened this issue Jun 20, 2018 · 36 comments · Fixed by #65891
Assignees
@x13n @loburm @CaoShuFeng
Labels
area/audit kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. sig/auth Categorizes an issue or PR as relevant to SIG Auth.
Milestone
v1.12

Comments

@x13n
Copy link
Member

x13n commented Jun 20, 2018 •
edited by liggitt

This is a continuation of #60392

API-related changes

Bugfixes and improvements

Policy changes

To discuss

  • [Optional] Auditing federation setups

I mostly carried over the unfinished work from 1.11 issue. Two things added: the ability to reject apiserver requests when audit logging fails (configurable via audit policy) and - optionally - recent proposal for dynamic audit configuration.

/kind feature
/sig auth
/area audit

cc @loburm @tallclair @soltysh
@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. sig/auth Categorizes an issue or PR as relevant to SIG Auth. area/audit labels Jun 20, 2018
@CaoShuFeng
Copy link
Contributor

Maybe add this: #64791

@piosz piosz assigned x13n and loburm Jun 20, 2018
@piosz
Copy link
Member

piosz commented Jun 20, 2018

cc @destijl

@tallclair
Copy link
Member

Add a setting in audit policy that will allow rejecting API request when audit logging fails

We already have that per-backend with Blocking mode. Is the idea that it would be nice to require blocking for only some highly-sensitive requests?

@tpepper
Copy link
Member

tpepper commented Jun 20, 2018

/milestone v1.12

@k8s-ci-robot k8s-ci-robot added this to the v1.12 milestone Jun 20, 2018
@tallclair tallclair added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Jun 20, 2018
@x13n
Copy link
Member Author

x13n commented Jun 21, 2018 •
edited

@CaoShuFeng I see Tim already added this.
@tallclair As far as I can tell, blocking will wait for audit event to be written, but upon a write failure (say, the webhook goes down, or the disk used for log backend is full) the request to API server will be processed anyway. The audit failure will only be written to the logs. I think that either:

  • 'blocking' should mean that the request to API server fails if audit logging fails OR
  • there should be a third option ('blocking-strict'? naming is hard) that would do that and existing 'blocking' will continue to behave as it does today.

@tallclair
Copy link
Member

I see, thanks for clarifying. Yeah, blocking should have been that (blocking-strict sgtm)

@x13n
Copy link
Member Author

x13n commented Jun 22, 2018

Sorry, can you clarify which one do you think makes more sense? Adding third option (e.g. blocking-strict) or changing behavior of existing 'blocking' setting?

@loburm
Copy link
Contributor

loburm commented Jun 22, 2018

@timstclair could you approve it for milestone?

@tallclair
Copy link
Member

I don't think we can change blocking without breaking backwards compatibliity, so I'm in favor of blocking-strict

@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Issue: Up-to-date for process

@loburm @x13n

Issue Labels
  • sig/auth: Issue will be escalated to these SIGs if needed.
  • priority/important-longterm: Escalate to the issue owners; move out of the milestone after 1 attempt.
  • kind/feature: New functionality.
Help

@liggitt
Copy link
Member

liggitt commented Jun 22, 2018

I don't think we can change blocking without breaking backwards compatibliity, so I'm in favor of blocking-strict

what would it mean for a mutating request to fail if the responsecomplete audit logging fails after the object is persisted? just a failure http status?

@tallclair
Copy link
Member

Right, to clarify this would only work for the RequestReceived stage. Actually, that could be used to get this behavior only for sensitive requests (by omitting RequestReceived for less sensitive requests).

@x13n
Copy link
Member Author

x13n commented Jun 25, 2018

Yeah, this makes sense only for RequestReceived stage, other stages would continue to stay best effort.

@loburm
Copy link
Contributor

loburm commented Jul 3, 2018

@CaoShuFeng I see that you have been performing API graduation from alpha to beta last time. Do you want to take care about doing the same for stable, or should I take it?

And do you think that there are any blockers for it?

@CaoShuFeng
Copy link
Contributor

CaoShuFeng commented Jul 3, 2018 •
edited

Do you want to take care about doing the same for stable, or should I take it?

Would be good either way.

I remembered that we will delete some fields when upgrade the audit API.
#52160 (comment)

And do you think that there are any blockers for it?

Not very sure. Do we wait until issues here get solved?

@x13n x13n mentioned this issue Jul 3, 2018
Merged
@loburm
Copy link
Contributor

loburm commented Jul 4, 2018 •
edited

I remembered that we will delete some fields when upgrade the audit API. #52160 (comment)

Thanks for pointing. Do we need any period for testing newly introduced/renamed fields?

Not very sure. Do we wait until issues here get solved?

user-agent field was added. @x13n could you update it?
Other changes (non optional one) are not changing API fields. Code removal for legacy audit logging can be done in parallel.

@CaoShuFeng
Copy link
Contributor

Do we need any period for testing newly introduced/renamed fields?

Agree. I think so.

Other changes (non optional one) are not changing API fields. Code removal for legacy audit logging can be done in parallel.

Sounds good to me. I could start coding in Friday, if I am responsible for upgrading the API. What do you think?

k8s-publishing-bot added a commit to kubernetes/kube-aggregator that referenced this issue Aug 8, 2018
@k8s-publishing-bot
Merge pull request #65891 from CaoShuFeng/audit_v1_stable
c23fbc3 
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

upgrade Audit api version to stable

Partial Fix: kubernetes/kubernetes#65266

TODO:
    use v1 version of advanced audit policy in [kubeadm](https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/cmd/kubeadm/app/util/audit/utils.go#L29), [gce script](https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/cluster/gce/gci/configure-helper.sh#L743), [kubemark](https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/test/kubemark/resources/start-kubemark-master.sh#L349)

**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:

**Release note**:

```release-note
audit.k8s.io api group is upgraded from v1beta1 to v1.
Deprecated element metav1.ObjectMeta and Timestamp are removed from audit Events in v1 version.
Default value of option --audit-webhook-version and --audit-log-version will be changed from `audit.k8s.io/v1beta1` to `audit.k8s.io/v1` in release 1.13
```

Kubernetes-commit: 28b2b2128723d382ce241e9b67c7e875b9dfba78
k8s-publishing-bot added a commit to kubernetes/sample-apiserver that referenced this issue Aug 8, 2018
@k8s-publishing-bot
Merge pull request #65891 from CaoShuFeng/audit_v1_stable
5353411 
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

upgrade Audit api version to stable

Partial Fix: kubernetes/kubernetes#65266

TODO:
    use v1 version of advanced audit policy in [kubeadm](https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/cmd/kubeadm/app/util/audit/utils.go#L29), [gce script](https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/cluster/gce/gci/configure-helper.sh#L743), [kubemark](https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/test/kubemark/resources/start-kubemark-master.sh#L349)

**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:

**Release note**:

```release-note
audit.k8s.io api group is upgraded from v1beta1 to v1.
Deprecated element metav1.ObjectMeta and Timestamp are removed from audit Events in v1 version.
Default value of option --audit-webhook-version and --audit-log-version will be changed from `audit.k8s.io/v1beta1` to `audit.k8s.io/v1` in release 1.13
```

Kubernetes-commit: 28b2b2128723d382ce241e9b67c7e875b9dfba78
k8s-publishing-bot added a commit to kubernetes/apiextensions-apiserver that referenced this issue Aug 8, 2018
@k8s-publishing-bot
Merge pull request #65891 from CaoShuFeng/audit_v1_stable
4d05e43 
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

upgrade Audit api version to stable

Partial Fix: kubernetes/kubernetes#65266

TODO:
    use v1 version of advanced audit policy in [kubeadm](https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/cmd/kubeadm/app/util/audit/utils.go#L29), [gce script](https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/cluster/gce/gci/configure-helper.sh#L743), [kubemark](https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/test/kubemark/resources/start-kubemark-master.sh#L349)

**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:

**Release note**:

```release-note
audit.k8s.io api group is upgraded from v1beta1 to v1.
Deprecated element metav1.ObjectMeta and Timestamp are removed from audit Events in v1 version.
Default value of option --audit-webhook-version and --audit-log-version will be changed from `audit.k8s.io/v1beta1` to `audit.k8s.io/v1` in release 1.13
```

Kubernetes-commit: 28b2b2128723d382ce241e9b67c7e875b9dfba78
@tallclair tallclair reopened this Aug 8, 2018
sttts pushed a commit to sttts/kube-aggregator that referenced this issue Aug 9, 2018
@k8s-publishing-bot
Merge pull request #65891 from CaoShuFeng/audit_v1_stable
ba579f9 
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

upgrade Audit api version to stable

Partial Fix: kubernetes/kubernetes#65266

TODO:
    use v1 version of advanced audit policy in [kubeadm](https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/cmd/kubeadm/app/util/audit/utils.go#L29), [gce script](https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/cluster/gce/gci/configure-helper.sh#L743), [kubemark](https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/test/kubemark/resources/start-kubemark-master.sh#L349)

**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:

**Release note**:

```release-note
audit.k8s.io api group is upgraded from v1beta1 to v1.
Deprecated element metav1.ObjectMeta and Timestamp are removed from audit Events in v1 version.
Default value of option --audit-webhook-version and --audit-log-version will be changed from `audit.k8s.io/v1beta1` to `audit.k8s.io/v1` in release 1.13
```

Kubernetes-commit: 28b2b2128723d382ce241e9b67c7e875b9dfba78
sttts pushed a commit to sttts/sample-apiserver that referenced this issue Aug 9, 2018
@k8s-publishing-bot
Merge pull request #65891 from CaoShuFeng/audit_v1_stable
fcb878e 
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

upgrade Audit api version to stable

Partial Fix: kubernetes/kubernetes#65266

TODO:
    use v1 version of advanced audit policy in [kubeadm](https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/cmd/kubeadm/app/util/audit/utils.go#L29), [gce script](https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/cluster/gce/gci/configure-helper.sh#L743), [kubemark](https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/test/kubemark/resources/start-kubemark-master.sh#L349)

**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:

**Release note**:

```release-note
audit.k8s.io api group is upgraded from v1beta1 to v1.
Deprecated element metav1.ObjectMeta and Timestamp are removed from audit Events in v1 version.
Default value of option --audit-webhook-version and --audit-log-version will be changed from `audit.k8s.io/v1beta1` to `audit.k8s.io/v1` in release 1.13
```

Kubernetes-commit: 28b2b2128723d382ce241e9b67c7e875b9dfba78
sttts pushed a commit to sttts/apiextensions-apiserver that referenced this issue Aug 9, 2018
@k8s-publishing-bot
Merge pull request #65891 from CaoShuFeng/audit_v1_stable
ccd02ac 
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

upgrade Audit api version to stable

Partial Fix: kubernetes/kubernetes#65266

TODO:
    use v1 version of advanced audit policy in [kubeadm](https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/cmd/kubeadm/app/util/audit/utils.go#L29), [gce script](https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/cluster/gce/gci/configure-helper.sh#L743), [kubemark](https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/test/kubemark/resources/start-kubemark-master.sh#L349)

**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:

**Release note**:

```release-note
audit.k8s.io api group is upgraded from v1beta1 to v1.
Deprecated element metav1.ObjectMeta and Timestamp are removed from audit Events in v1 version.
Default value of option --audit-webhook-version and --audit-log-version will be changed from `audit.k8s.io/v1beta1` to `audit.k8s.io/v1` in release 1.13
```

Kubernetes-commit: 28b2b2128723d382ce241e9b67c7e875b9dfba78
@CaoShuFeng
Copy link
Contributor

/open

@CaoShuFeng
Copy link
Contributor

/reopen

@k8s-ci-robot
Copy link
Contributor

@CaoShuFeng: you can't re-open an issue/PR unless you authored it or you are assigned to it.

In response to this:

/reopen

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.

@CaoShuFeng
Copy link
Contributor

/assign

@CaoShuFeng
Copy link
Contributor

/reopen

@k8s-ci-robot k8s-ci-robot reopened this Aug 9, 2018
@sttts
Copy link
Contributor

sttts commented Aug 9, 2018

Stop putting "bla bla Fix: #65266" into PR descriptions :-)

@x13n
Copy link
Member Author

x13n commented Sep 4, 2018

/reopen

@k8s-ci-robot
Copy link
Contributor

@x13n: Reopening this issue.

In response to this:

/reopen

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.

@k8s-ci-robot k8s-ci-robot reopened this Sep 4, 2018
@guineveresaenger
Copy link
Contributor

Please continue this work in 1.13.
/milestone clear
/milestone v1.13

@k8s-ci-robot k8s-ci-robot removed this from the v1.12 milestone Sep 18, 2018
@guineveresaenger
Copy link
Contributor

/milestone v1.13

@k8s-ci-robot k8s-ci-robot added this to the v1.13 milestone Sep 18, 2018
@liggitt
Copy link
Member

liggitt commented Sep 18, 2018

Actually, we'll tombstone this to track what was done in 1.12 and copy unfinished items to a new issue

/milestone v1.12
/close

@k8s-ci-robot
Copy link
Contributor

@liggitt: Closing this issue.

In response to this:

Actually, we'll tombstone this to track what was done in 1.12 and copy unfinished items to a new issue

/milestone v1.12
/close

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.

@k8s-ci-robot k8s-ci-robot modified the milestones: v1.13, v1.12 Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@x13n x13n

@loburm loburm

@CaoShuFeng CaoShuFeng

Labels
area/audit kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. sig/auth Categorizes an issue or PR as relevant to SIG Auth.
Projects
None yet
Milestone
v1.12