Extend authorization policy action with 'Audit' action by davidraskin · Pull Request #1552 · istio/api

davidraskin · 2020-07-22T17:33:16Z

We'd like to add an audit action to the Authorization Policy resource, which would be used to determine whether requests should be logged, and can be supported by Istio telemetry v2 plugins. When a policy matches the request, an attribute would be set through the envoy RBAC filter, which plugins can retrieve to check the decision.

See istio/istio#25591

istio-testing · 2020-07-22T17:33:29Z

Hi @davidraskin. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

davidraskin · 2020-07-22T17:38:24Z

cc @liminw @mandarjog @louiscryan @smawson @yangminzhu

howardjohn

What is the goal of this PR? Previously we have had API PRs for un-approved docs and we end up with comments split between the PR and the doc which is very confusing. Should we iron out the design and get it approved first?

davidraskin · 2020-07-22T17:47:45Z

Do you recommend we discuss specifically what the docs should say first, and get a PR in for those?

smawson

Like all API changes, this needs an RFC and a discussion.

smawson · 2020-07-22T18:02:45Z

security/v1beta1/authorization.proto

-// to create an allow policy. The default action is "ALLOW" but it is useful
-// to be explicit in the policy.
+// Istio Authorization Policy also supports the AUDIT action to decide whether to log requests. 
+// The decision can be read by telemetry plugins using the function getAuditPolicy


This is user facing doc, is this what we actually want to tell users? That they should go implement a telemetry filter to use this, and here is the documentation on how to do that?

This should instead be focused on when users should use this and how, and leave implementation to a separate developer-facing doc.

BTW, if the user does not have an audit-capable filter installed, and they add an audit policy, what happens? Do we have a way to detect and alert them?

davidraskin · 2020-07-28T21:41:25Z

Audit policy RFC

mandarjog · 2020-07-28T22:02:24Z

security/v1beta1/authorization.proto

+// Istio Authorization Policy also supports the AUDIT action to decide whether to log requests. 
+// The decision can be read by telemetry plugins using the function getAuditPolicy
+// defined [here](https://github.com/istio/proxy/blob/master/extensions/common/context.h).
+// A call to getAuditPolicy will only return `true` in the following cases:


The context.h reference is fine in the attached design doc, but not in the API doc.
wWe should only specify the filter state key that this process is setting, and how a downstream filter like logging, or even metrics can use it. The setting of the filter state is the only externally observable action of AUDIT

Yeah, I'll be changing that. Is it fine to refer to the envoy specific dynamic metadata, or would it be better to just provide a list of access logging plugins that check this key?

smawson · 2020-07-28T22:20:17Z

I would put anything envoy specific in a hide-from-docs section, that shouldn't show up in the public docs. I like the idea of public API docs listing what telemetry filters support this action.

smawson · 2020-07-29T18:56:56Z

security/v1beta1/authorization.proto

+// A request will be audited in the following cases:
 //
 // - There exists an AUDIT policy on the workload that matches the request
 // - There are no AUDIT policies on the workload


I'm confused by this, the request will be audited if there are no AUDIT policies on the workload?

That does come off as weird. The point was to log everything if there are no policies. If we're making two separate streams for logs though, then it might be better to log only when an audit policy exists.

To log everything they could create a global empty audit policy right? So I'd just say that a request is audited if there is an audit rule that matches the request.

this doesn't sound right to me, why would a request be audited even if there is no AUDIT policies on it? This is not the case described in the design doc.

Yep you're right. I was thinking of it from the perspective of ALLOW policy where if there is no policy on the workload requests are allowed by default. But obviously this doesn't apply here.

yangminzhu · 2020-07-29T19:29:23Z

security/v1beta1/authorization.proto

+// A request will be audited in the following cases:
 //
 // - There exists an AUDIT policy on the workload that matches the request
 // - There are no AUDIT policies on the workload


this doesn't sound right to me, why would a request be audited even if there is no AUDIT policies on it? This is not the case described in the design doc.

yangminzhu · 2020-07-29T19:30:53Z

security/v1beta1/authorization.proto

+// - There exists an AUDIT policy on the workload that matches the request
+// - There are no AUDIT policies on the workload
+// 
+// AUDIT policies do not affect whether requests are allowed to the workload.


allowed or denied

Also can we move this to the above where we first talked about the AUDIT action? (after line 37)

yangminzhu · 2020-07-29T19:34:03Z

security/v1beta1/authorization.proto

-// to create an allow policy. The default action is "ALLOW" but it is useful
-// to be explicit in the policy.
+// Istio Authorization Policy also supports the AUDIT action to decide whether to log requests.
+// Telemetry v2 access logging plugins can be set to abide by the audit decision.


Can we make it super clear that the AUDIT action in AuthZ policy doesn't actually record or log anything anywhere, what it really does is to record the decision that the request should be audited, and that decision must be implemented by some other logging plugins to actually record and log the request, for example, the Stackdriver?

And more importantly, if there is no such logging plugin configured, nothing will be logged or audited at all. We need make sure the user won't misunderstand this feature and think this is all they need for auditing a request but later found out nothing is really audited by just using the authz policy.

Also is the Telemetry v2 access logging the same as the Stackdriver? What about the Envoy Access Log filter?

Telemtry v2 access logging is stackdriver, and in the furture will be all access log plugins. For envoy access log filter, a boolean needs to be added to meshconfig (which will trigger configuration of the metadata access log filter). I'll add this to the documentation if this is something we want to do as well.

liminw

The change looks good to me. Thanks.

yangminzhu · 2020-07-29T23:02:26Z

security/v1beta1/authorization.proto

+// AUDIT policies do not affect whether requests are allowed or denied to the workload.
+// Requests will be allowed or denied based solely on ALLOW and DENY policies.
+//
+// Telemetry v2 access logging plugins can be set to abide by the audit decision.


Can we change it to the following to make this part more clear (line 41-44), thanks:

A request will be internally marked that it should be audited if there is an AUDIT policy on the workload that matches the request.

A separate plugin must be configured and enabled to actually fulfill the audit decision and complete the audit behavior. The request will not be audited if there are no such supporting plugins enabled.

Currently the only supported plugin is the Telemetry v2 Stackdriver plugin.

concerns addresses

smawson · 2020-07-30T22:36:20Z

/ok-to-test

* Modified rbac proto * change module name * change module again * change go.mod again * Changed back go.mod, go.sum, proto.lock * Updated proto.lock * Updated Audit documentation * Documentation update * Updated documentation

* Modified rbac proto * change module name * change module again * change go.mod again * Changed back go.mod, go.sum, proto.lock * Updated proto.lock * Updated Audit documentation * Documentation update * Updated documentation Co-authored-by: David Raskin <66272127+davidraskin@users.noreply.github.com>

davidraskin added 7 commits June 11, 2020 04:09

Modified rbac proto

07072cc

change module name

8f9b2a8

change module again

1b2659b

change go.mod again

9d98e3d

Changed back go.mod, go.sum, proto.lock

325e52b

Change action to AUDIT

33854b2

Updated proto.lock

be071dd

davidraskin requested review from dcberg, duderino, linsun, louiscryan, rshriram and smawson as code owners July 22, 2020 17:33

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 22, 2020

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jul 22, 2020

istio-testing added the needs-ok-to-test label Jul 22, 2020

howardjohn previously requested changes Jul 22, 2020

View reviewed changes

smawson reviewed Jul 22, 2020

View reviewed changes

mandarjog reviewed Jul 28, 2020

View reviewed changes

Updated Audit documentation

ab15e6e

davidraskin requested a review from nrjpoddar as a code owner July 29, 2020 18:49

smawson reviewed Jul 29, 2020

View reviewed changes

yangminzhu reviewed Jul 29, 2020

View reviewed changes

Documentation update

7a6b01d

liminw approved these changes Jul 29, 2020

View reviewed changes

mandarjog approved these changes Jul 29, 2020

View reviewed changes

yangminzhu reviewed Jul 29, 2020

View reviewed changes

Updated documentation

a531c59

istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Jul 30, 2020

smawson approved these changes Jul 30, 2020

View reviewed changes

istio-testing merged commit 50557a0 into istio:master Jul 30, 2020

This was referenced Aug 5, 2020

Add davidraskin as member istio/community#389

Merged

Add audit log stream to stackdriver filter istio/proxy#2970

Merged

Implementation for 'Audit' action in Authorization Policy istio/istio#26439

Merged

liminw added the cherrypick/release-1.7 label Sep 3, 2020

istio-testing mentioned this pull request Sep 3, 2020

[release-1.7] Extend authorization policy action with 'Audit' action #1621

Closed

istio deleted a comment from istio-testing Sep 3, 2020

williamaronli mentioned this pull request Sep 3, 2020

Cherrypick to release 1.7 Extend authorization policy action with 'Audit' action (#1552) #1622

Merged

istio deleted a comment from istio-testing Sep 3, 2020

Conversation

davidraskin commented Jul 22, 2020

Uh oh!

istio-testing commented Jul 22, 2020

Uh oh!

davidraskin commented Jul 22, 2020

Uh oh!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

davidraskin commented Jul 22, 2020

Uh oh!

smawson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidraskin commented Jul 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smawson commented Jul 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangminzhu Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liminw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smawson commented Jul 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

yangminzhu Jul 29, 2020 •

edited

Loading