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.

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

Add user-agent to audit-logging #64812

Merged
merged 4 commits into from Jun 26, 2018

Conversation

@hzxuzhonghu
Copy link
Member

hzxuzhonghu commented Jun 6, 2018

What this PR does / why we need it:

Add User-Agent to audit event.

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 #64791

Special notes for your reviewer:

Release note:

Add user-agent to audit-logging.
@hzxuzhonghu

This comment has been minimized.

Copy link
Member Author

hzxuzhonghu commented Jun 6, 2018

@k8s-ci-robot k8s-ci-robot requested review from CaoShuFeng , sttts and tallclair Jun 6, 2018

@@ -42,6 +42,7 @@ func NewEventFromRequest(req *http.Request, level auditinternal.Level, attribs a
RequestReceivedTimestamp: metav1.NewMicroTime(time.Now()),
Verb: attribs.GetVerb(),
RequestURI: req.URL.RequestURI(),
UserAgent: req.Header.Get("User-Agent"),

This comment has been minimized.

@CaoShuFeng

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Jun 6, 2018

Author Member

good idea

@CaoShuFeng

This comment has been minimized.

Copy link
Member

CaoShuFeng commented Jun 6, 2018

Test OK in my environment.

@@ -2,7 +2,7 @@
"swagger": "2.0",
"info": {
"title": "Kubernetes",
"version": "v1.12.0"

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Jun 6, 2018

Member

We don't need this.

I think this will disappear after these steps:

git checkout master
git pull upstream master --tags
git checkout xxx
git rebase master

@hzxuzhonghu hzxuzhonghu force-pushed the hzxuzhonghu:audit-useragent branch from d9e7c8e to 06cbe36 Jun 6, 2018

@@ -88,6 +88,9 @@ type Event struct {

// RequestURI is the request URI as sent by the client to a server.
RequestURI string
// UserAgent is an optional field that specifies the caller of this request.
// +optional
UserAgent string

This comment has been minimized.

@sttts

sttts Jun 6, 2018

Contributor

am surprised we did not have this.

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Jun 6, 2018

Looks sensible. As this changes the API, let's give the other reviewer a chance to take a look.

@tallclair

This comment has been minimized.

Copy link
Member

tallclair commented Jun 7, 2018

/sig auth
/kind feature

@@ -88,6 +88,9 @@ type Event struct {

// RequestURI is the request URI as sent by the client to a server.
RequestURI string
// UserAgent is an optional field that specifies the caller of this request.
// +optional
UserAgent string

This comment has been minimized.

@tallclair

tallclair Jun 7, 2018

Member

Nit: move this to below "SourceIPs", the other client-spoofable field.

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Jun 8, 2018

Author Member

ok

@@ -88,6 +88,9 @@ type Event struct {

// RequestURI is the request URI as sent by the client to a server.
RequestURI string
// UserAgent is an optional field that specifies the caller of this request.

This comment has been minimized.

@tallclair

tallclair Jun 7, 2018

Member

Please add a note that this is trivially spoofable. e.g.

// Note that the UserAgent is provided by the client, and must not be trusted.

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Jun 8, 2018

Author Member

ok


// UserAgent is an optional field that specifies the caller of this request.
// +optional
UserAgent string `json:"userAgent,omitempty" protobuf:"bytes,18,opt,name=userAgent"`

This comment has been minimized.

@tallclair

tallclair Jun 7, 2018

Member

Please be consistent in the field ordering. See above comment.


// UserAgent is an optional field that specifies the caller of this request.
// +optional
UserAgent string `json:"userAgent,omitempty" protobuf:"bytes,18,opt,name=userAgent"`

This comment has been minimized.

@tallclair

tallclair Jun 7, 2018

Member

ditto.

@@ -42,6 +42,7 @@ func NewEventFromRequest(req *http.Request, level auditinternal.Level, attribs a
RequestReceivedTimestamp: metav1.NewMicroTime(time.Now()),
Verb: attribs.GetVerb(),
RequestURI: req.URL.RequestURI(),
UserAgent: req.UserAgent(),

This comment has been minimized.

@tallclair

tallclair Jun 7, 2018

Member

Mind adding a unit test? You could either add a unit test for this method, or check this in https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/audit_test.go

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Jun 8, 2018

Author Member

done

This comment has been minimized.

@liggitt

liggitt Jun 20, 2018

Member

we should also consider setting a maximum length on the size of the data allowed in this field

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Jun 20, 2018

Author Member

Is there any doc saying that what value to set for user-agent?
I only find apiserver set http.Server.MaxHeaderBytes=1 << 20 to limit max header size. Not find any thing about User-Agent

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Jun 20, 2018

Member

Because user agent is logged at metadata level, and it's provided by client, so we should verify that audit events cannot be made arbitrary large.

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Jun 20, 2018

Author Member

Yes, I agree should limit size. My question is set this limit to what.

This comment has been minimized.

@tallclair

tallclair Jun 20, 2018

Member

How about 1024? And we should append something like ...TRUNCATED if it's too long.

This comment has been minimized.

@liggitt

liggitt Jun 20, 2018

Member

works for me

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Jun 21, 2018

Author Member

Thanks for your suggestion.

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Jun 8, 2018

@hzxuzhonghu hzxuzhonghu force-pushed the hzxuzhonghu:audit-useragent branch from c976639 to 98f634d Jun 8, 2018

@hzxuzhonghu

This comment has been minimized.

Copy link
Member Author

hzxuzhonghu commented Jun 8, 2018

@tallclair all addressed

@hzxuzhonghu

This comment has been minimized.

Copy link
Member Author

hzxuzhonghu commented Jun 8, 2018

/test pull-kubernetes-node-e2e

@@ -99,6 +99,10 @@ type Event struct {
// Source IPs, from where the request originated and intermediate proxies.
// +optional
SourceIPs []string
// UserAgent is an optional field that specifies the caller of this request.

This comment has been minimized.

@tallclair

tallclair Jun 8, 2018

Member

nit: drop "is an optional field" - it's redundant.

How about: UserAgent records the user agent string reported by the client.

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Jun 9, 2018

Author Member

SGTM

@@ -131,6 +140,7 @@ func TestAudit(t *testing.T) {
[]eventCheck{
noRequestBody(0),
responseBodyMatches(0, `{.*"name":"c".*}`),
requestUserAgentMatches(0, userAgent),

This comment has been minimized.

@tallclair

tallclair Jun 8, 2018

Member

Since this is constant for all testcases, just check it in the loop below rather than specifying here

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Jun 9, 2018

Author Member

ok

@hzxuzhonghu hzxuzhonghu force-pushed the hzxuzhonghu:audit-useragent branch from 98f634d to cbc223e Jun 9, 2018

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jun 22, 2018

user-agent isn't an appropriate way to transmit this data. I agree with @tallclair's suggestion of client-side logging combined with an audit id. I also would not be in favor of including data in the user-agent that revealed local path names or source packages to the server, for arbitrary code-bases client-go would be used in.

@CaoShuFeng

This comment has been minimized.

Copy link
Member

CaoShuFeng commented Jun 22, 2018

Like this:

$ curl -v -q -k --cert  /var/kube/kubecfg.crt -XGET  --key /var/kube/kubecfg.key -H "Audit-Id: 88888888-8888-8888-8888-888888888888"  'https://172.16.29.130:443/api/v1/nodes'
...
$ grep 88888888-8888-8888-8888-888888888888 /var/log/k8s-audits.log
...

Edit
Or this:

$ curl -v -s --cacert /var/kube/ca.crt --cert  /var/kube/kubecfg.crt -XGET  --key /var/kube/kubecfg.key  'https://172.16.29.130:443/api/v1/namespaces/default/configmaps' 2>&1 | grep Audit
< Audit-Id: fbebf661-c178-4264-83f1-60255616287d
$ grep "dff71208-5369-4318-8d5b-69ea0f0b7d25" /var/log/k8s-audits.log

@hzxuzhonghu hzxuzhonghu force-pushed the hzxuzhonghu:audit-useragent branch 2 times, most recently from 50f0573 to 5f1d0cd Jun 22, 2018

@hzxuzhonghu hzxuzhonghu force-pushed the hzxuzhonghu:audit-useragent branch from 5f1d0cd to f0b1f1c Jun 22, 2018

@hzxuzhonghu

This comment has been minimized.

Copy link
Member Author

hzxuzhonghu commented Jun 22, 2018

/retest

@hh

This comment has been minimized.

Copy link
Member

hh commented Jun 22, 2018

I am trying to identify when an api call is being made from the same line of code / callstack without client side logging or logic (so it can be enabled for any application using client-go).

To limit exposing local path names and source, client-go could instead generate a hash of the data (generalized, so it's just the paths+linums under $GOPATH)?

This hash could be included in User-Agent, Audit-Id, or possibly a new Callstack-Hash, but only when configured to do so via a KUBE_CALLSTACK_HASH style env var.

@hzxuzhonghu

This comment has been minimized.

Copy link
Member Author

hzxuzhonghu commented Jun 25, 2018

ping @tallclair @liggitt @CaoShuFeng for approval

@hh

This comment has been minimized.

Copy link
Member

hh commented Jun 25, 2018

/hold cancel
I'll try and put this together into a KEP for wider discussion.
Let's move forward with the 1k limit for now.

@hh

This comment has been minimized.

Copy link
Member

hh commented Jun 25, 2018

@liggitt thanks for the alternate approach 👍

I'd prefer if there were zero changes to applications using client-go, instead using an env var to enable callstack-hash-sending. Off by default, when on we reveal only a hash (of the go-paths only, local paths removed), via the appropriate aggregation method.

My personal preference is the hash somehow appear directly in the audit-logs, but if there are other options that allow anyone to easily contribute data, I'd love to hear them!

Also what's the best place to get wider feedback on something like this?
Should I move these thoughts / approach into a KEP or a post to k8s-dev?

/cc @tallclair @CaoShuFeng

@k8s-ci-robot k8s-ci-robot requested review from CaoShuFeng and tallclair Jun 25, 2018

@hh

This comment has been minimized.

Copy link
Member

hh commented Jun 25, 2018

Thanks for your work on this @hzxuzhonghu ! 👏 🥇

@tallclair

This comment has been minimized.

Copy link
Member

tallclair commented Jun 25, 2018

/lgtm

@hzxuzhonghu

This comment has been minimized.

Copy link
Member Author

hzxuzhonghu commented Jun 26, 2018

ping @sttts @liggitt for approval

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jun 26, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jun 26, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CaoShuFeng, hzxuzhonghu, liggitt, tallclair

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:

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 26, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 26, 2018

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

@k8s-github-robot k8s-github-robot merged commit 1f4f012 into kubernetes:master Jun 26, 2018

16 of 17 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation hzxuzhonghu authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@hzxuzhonghu hzxuzhonghu deleted the hzxuzhonghu:audit-useragent branch Jun 26, 2018

@krmayankk

This comment has been minimized.

Copy link
Contributor

krmayankk commented Oct 14, 2018

what milestone did this go ? will this be cherry picked to older releases like 1.9/1.10 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.