Add audit log stream to stackdriver filter by davidraskin · Pull Request #2970 · istio/proxy

davidraskin · 2020-08-06T17:14:07Z

This adds an audit log stream to stackdriver. The goal would be to have requests to be auditted will be filtered through the audit action on Istio Authorization Policy.
See: istio/api#1552, Audit log stream proposal

cc @liminw @gargnupur @kyessenov @mandarjog

This is WIP. It should only be merged after envoyproxy/envoy#11705

Signed-off-by: davidraskin <draskin@google.com>

istio-policy-bot · 2020-08-06T17:14:12Z

😊 Welcome @davidraskin! This is either your first contribution to the Istio proxy repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

Signed-off-by: davidraskin <draskin@google.com>

douglas-reid · 2020-08-06T17:16:34Z

extensions/stackdriver/config/v1alpha1/stackdriver_plugin_config.proto

  bool disable_server_access_logging = 1;

+  // Optional. Controls whether to export audit log.
+  bool enable_server_access_auditting = 10;


nit: can we name this audit_server_access ?

douglas-reid · 2020-08-06T17:45:27Z

extensions/stackdriver/log/logger.cc


 // Name of the HTTP server access log.
 constexpr char kServerAccessLogName[] = "server-accesslog-stackdriver";
+constexpr char kAuditAccessLogName[] = "server-accesslog-stackdriver-audit";


is this naming convention important? does it need to match the historic name for the other access logs, or should we consider calling this server-audit-log (or something else less verbose)?

I don't think that's important. I just went the simplest route. I'll rename it.

bianpengyuan · 2020-08-06T18:08:11Z

This might be a dumb question, but isn't it that request should not proceed until audit log succeed? Looks like the current implementation is still logging at Log callback which happens after request finishes.

davidraskin · 2020-08-06T18:13:03Z

The request is not exported if there are no audit log entries added to it. This works the same way as with regular log entries currently.

gargnupur · 2020-08-06T18:17:53Z

extensions/stackdriver/log/logger.cc

  log_entries_request_ =
      std::make_unique<google::logging::v2::WriteLogEntriesRequest>();

+  audit_entries_request_ =


check this out... I have made it a map: https://github.com/istio/proxy/pull/2955/files#diff-d116ca95f1292dece4d352210d220a13 so would be easier to add many types of entries now...

bianpengyuan · 2020-08-06T18:26:15Z

@davidraskin Yeah, but current access log implementation is for telemetry and at best effort. The exporting could have transient failure which would leave some requests not logged. Is this fine for audit log? I thought the assumption of audit log is that every request reaches application must be logged.

davidraskin · 2020-08-06T18:35:46Z

@bianpengyuan I see what you're saying. I think it's still important to see the result of the request, and whether it succeeds or fails. But maybe it should fail closed rather than open.

bianpengyuan · 2020-08-06T18:44:00Z

extensions/stackdriver/log/logger.cc


 // Name of the HTTP server access log.
 constexpr char kServerAccessLogName[] = "server-accesslog-stackdriver";
+constexpr char kAuditAccessLogName[] = "server-accesslog-stackdriver-audit";


Out of curiosity, have you thought about fill in protoPayload in logEntry, which supports google.cloud.audit.AuditLog: https://cloud.google.com/logging/docs/reference/audit/auditlog/rest/Shared.Types/AuditLog. Sound like that is a more standard way to do audit log at Google?

Yes, the AuditLog payload is used primarily for auditting GCP service calls, it doesn't apply to application logging, afaik.

@bianpengyuan @davidraskin can you find out if google.cloud.audit.AuditLog cannot be used to audit other services?

@mandarjog Maybe it can theoretically be used, because I think the only difference is the proto payload, but the Audit Log docs state that it is for GCP API calls. Additionally, configuring the Data Access Audit Logs is done through the Cloud Console, which could be confusing since that wouldn't be the case here.

Should these logs be actually called audit logs? https://cloud.google.com/logging/docs/audit cloud audit logs looks like are logs for telling who or what accessed a GCP resource... This is more about capturing auth info for application?

They're still audit logs I think. Just ones at the application level, separate from GCP audit logs. We can call them IstioAudit logs to reduce confusion.

Signed-off-by: davidraskin <draskin@google.com>

davidraskin · 2020-08-18T17:12:35Z

/retest

bianpengyuan

Code lgtm, the only comment is about adding more details on audit logging. Also can we make sure that it is ok for audit log happens after request finishes? Sorry for being paranoid here, but I still don't feel this is quite right... @liminw Can you comment on this?

gargnupur · 2020-08-18T21:24:27Z

extensions/stackdriver/stackdriver.cc

+                            false /* audit */);
  }
+
+  if (enableAuditLog() && shouldAuditThisRequest()) {


need to call ::Wasm::Common::populateExtendedRequestInfo(&request_info); here too?

gargnupur · 2020-08-18T21:31:21Z

extensions/stackdriver/log/logger.cc

-  auto log_entry_type = GetLogEntryType(outbound);
+    const ::Wasm::Common::FlatNode& local_node_info, bool outbound,
+    bool audit) {
+  const LogEntryType log_entry_type = GetLogEntryType(outbound, audit);


no need for const with enum

gargnupur · 2020-08-18T21:32:48Z

extensions/stackdriver/log/logger.cc

-    google::logging::v2::LogEntry* log_entry) {
+    const ::Wasm::Common::FlatNode& peer_node_info,
+    google::logging::v2::LogEntry* log_entry, bool outbound, bool audit) {
+  auto& entries_request =


this can be const auto& i think...

gargnupur · 2020-08-18T21:34:21Z

code looks ok, with some minor nits.. can you also add an integration test (https://github.com/istio/proxy/blob/master/test/envoye2e/stackdriver_plugin/stackdriver_test.go) ?

liminw · 2020-08-19T17:55:19Z

@davidraskin Yeah, but current access log implementation is for telemetry and at best effort. The exporting could have transient failure which would leave some requests not logged. Is this fine for audit log? I thought the assumption of audit log is that every request reaches application must be logged.

Peter, you are totally correct. Audit log has strong reliability requirement. This is the first step of enabling configurable audit feature. Going forward, we plan to improve the reliability of audit stream, which requires a separate design.

bianpengyuan · 2020-08-19T18:21:53Z

@liminw Ok, thanks for clarification! Can you elaborate a bit on the first step of enabling configurable audit feature? It does not look right to me to check in code that is not going to used for production. It will likely just become tech debt so hope we can avoid that here.

davidraskin · 2020-08-19T18:38:19Z

@bianpengyuan If request completion is the concern, we can just do the logging in the requestHeaders callback, and fail if there is some issue, and/or have an extra verification step to see that the log is received.

bianpengyuan · 2020-08-19T18:44:13Z

we can just do the logging in the requestHeaders callback

That would be ideal IMO. thanks!

liminw · 2020-08-19T19:50:14Z

@liminw Ok, thanks for clarification! Can you elaborate a bit on the first step of enabling configurable audit feature? It does not look right to me to check in code that is not going to used for production. It will likely just become tech debt so hope we can avoid that here.

I don't think we are close to claim audit feature production ready. As far as I know, Cloud audit team has done a lot of work to improve the reliability of audit stream. We still need to learn from them and design for it. But this can be done incrementally.

Signed-off-by: davidraskin <draskin@google.com>

davidraskin · 2020-08-19T20:51:03Z

/retest

Signed-off-by: davidraskin <draskin@google.com>

gargnupur · 2020-08-19T23:02:31Z

test/envoye2e/stackdriver_plugin/stackdriver_test.go

+		respCode      string
+		logEntryCount int
+	}{
+		{"StackdriverAudit", "200", 5},


As there is only one testcase, we don't need this?

gargnupur

Just have one minor nit.. adding "do-not-merge" for that and approval from @bianpengyuan

Signed-off-by: davidraskin <draskin@google.com>

elfinhe · 2020-09-14T18:20:31Z

/cherry-pick release-1.7

istio-testing · 2020-09-14T18:20:36Z

@elfinhe: #2970 failed to apply on top of branch "release-1.7":

Applying: Initial auditEntry implementation
Using index info to reconstruct a base tree...
M	extensions/stackdriver/log/logger.cc
M	extensions/stackdriver/log/logger.h
Falling back to patching base and 3-way merge...
Auto-merging extensions/stackdriver/log/logger.h
Auto-merging extensions/stackdriver/log/logger.cc
Applying: Initial audit log implementation
Using index info to reconstruct a base tree...
M	extensions/common/context.cc
M	extensions/common/context.h
M	extensions/stackdriver/log/logger.cc
M	extensions/stackdriver/log/logger.h
Falling back to patching base and 3-way merge...
Auto-merging extensions/stackdriver/log/logger.h
CONFLICT (content): Merge conflict in extensions/stackdriver/log/logger.h
Auto-merging extensions/stackdriver/log/logger.cc
CONFLICT (content): Merge conflict in extensions/stackdriver/log/logger.cc
Auto-merging extensions/common/context.h
Auto-merging extensions/common/context.cc
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Initial audit log implementation
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Details

In response to this:

/cherry-pick release-1.7

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.

istio-testing · 2020-09-14T18:20:37Z

@elfinhe: new issue could not be created for failed cherrypick: status code 410 not one of [201], body: {"message":"Issues are disabled for this repo","documentation_url":"https://docs.github.com/v3/issues/"}

Details

In response to this:

/cherry-pick release-1.7

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 added 2 commits August 2, 2020 06:00

Initial auditEntry implementation

1f9ca46

Signed-off-by: davidraskin <draskin@google.com>

Initial audit log implementation

4078b7f

Signed-off-by: davidraskin <draskin@google.com>

davidraskin requested review from a team, gargnupur and kyessenov August 6, 2020 17:14

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

istio-testing added needs-rebase Indicates a PR needs to be rebased before being merged size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 6, 2020

Merge conflict

a6fc504

Signed-off-by: davidraskin <draskin@google.com>

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Aug 6, 2020

davidraskin marked this pull request as draft August 6, 2020 17:24

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Aug 6, 2020

Format

d300d24

Signed-off-by: davidraskin <draskin@google.com>

davidraskin marked this pull request as ready for review August 6, 2020 17:42

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Aug 6, 2020

douglas-reid reviewed Aug 6, 2020

View reviewed changes

gargnupur reviewed Aug 6, 2020

View reviewed changes

bianpengyuan reviewed Aug 6, 2020

View reviewed changes

davidraskin added 4 commits August 11, 2020 05:41

Use LogEntryType and map for WriteLogEntriesRequest

ca0f229

Signed-off-by: davidraskin <draskin@google.com>

Merge remote-tracking branch 'upstream/master'

c139a02

Add function to initialize logEntriesRequests

fac3b3b

Signed-off-by: davidraskin <draskin@google.com>

Format

eea4b00

Signed-off-by: davidraskin <draskin@google.com>

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Aug 11, 2020

davidraskin added 2 commits August 12, 2020 03:49

Merge with master

c8f5d2d

Signed-off-by: davidraskin <draskin@google.com>

Switched to using bool for outbound and bool for audit

b35ef06

Signed-off-by: davidraskin <draskin@google.com>

Documentation of config

6783506

Signed-off-by: davidraskin <draskin@google.com>

bianpengyuan reviewed Aug 18, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master'

da50b42

gargnupur reviewed Aug 18, 2020

View reviewed changes

Integration test + address comments

1f05c1d

Signed-off-by: davidraskin <draskin@google.com>

davidraskin requested a review from a team August 19, 2020 19:51

istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 19, 2020

Format

551a1f4

Signed-off-by: davidraskin <draskin@google.com>

test multiple auditentries + format comment

0fed677

Signed-off-by: davidraskin <draskin@google.com>

gargnupur reviewed Aug 19, 2020

View reviewed changes

gargnupur approved these changes Aug 19, 2020

View reviewed changes

gargnupur added the do-not-merge Block automatic merging of a PR. label Aug 19, 2020

bianpengyuan approved these changes Aug 20, 2020

View reviewed changes

Remove test case list + add test to inventory

d9a3d20

Signed-off-by: davidraskin <draskin@google.com>

gargnupur removed the do-not-merge Block automatic merging of a PR. label Aug 20, 2020

istio-testing merged commit 40c3904 into istio:master Aug 20, 2020

Conversation

davidraskin commented Aug 6, 2020

Uh oh!

istio-policy-bot commented Aug 6, 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!

bianpengyuan commented Aug 6, 2020

Uh oh!

davidraskin commented Aug 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bianpengyuan commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidraskin commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

davidraskin commented Aug 18, 2020

Uh oh!

bianpengyuan left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gargnupur commented Aug 18, 2020

Uh oh!

liminw commented Aug 19, 2020

Uh oh!

bianpengyuan commented Aug 19, 2020

Uh oh!

davidraskin commented Aug 19, 2020

Uh oh!

bianpengyuan commented Aug 19, 2020

Uh oh!

liminw commented Aug 19, 2020

Uh oh!

davidraskin commented Aug 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gargnupur left a comment

Choose a reason for hiding this comment

Uh oh!

elfinhe commented Sep 14, 2020

Uh oh!

istio-testing commented Sep 14, 2020

Uh oh!

istio-testing commented Sep 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

bianpengyuan commented Aug 6, 2020 •

edited

Loading

davidraskin commented Aug 6, 2020 •

edited

Loading

bianpengyuan left a comment •

edited

Loading