create RequestBody struct, implement it using requests, add requestbody into Attribute for auditing #5124

bowenxia · 2023-03-01T02:03:06Z

What changed?
create FilteredRequestBody struct,
implement it using requests,
add FilteredRequestBody into Attribute
add a log tag for request body
add a email tag for actor emails

Why?
create FilteredRequestBody struct -> as an interface
implement it using requests -> we can store different requests as FilteredRequestBody, and can use a SerializeForLogging method
to transfer a struct into string. So that it is easier to log
add FilteredRequestBody into Attribute -> auditing manual accesses by logging the request body

How did you test it?
unit test

Potential risks
Leaking PII? -> But have already manually filtered fields to be logged.

Release notes

Documentation Changes

This reverts commit fbe72b3.

…o Attribute to use this field for auditing manual accesses

… auto generated code

coveralls · 2023-03-01T03:27:27Z

Pull Request Test Coverage Report for Build 0186b857-7512-4cad-ad94-15da898f9b53

105 of 496 (21.17%) changed or added relevant lines in 6 files are covered.
54 unchanged lines in 14 files lost coverage.
Overall coverage decreased (-0.1%) to 57.084%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
common/log/tag/tags.go	0	6	0.0%
common/types/replicator.go	0	18	0.0%
common/types/admin.go	0	27	0.0%
service/frontend/accessControlledHandler.go	101	135	74.81%
service/frontend/accessControlledAdminHandler.go	4	100	4.0%
common/types/shared.go	0	210	0.0%

Files with Coverage Reduction	New Missed Lines	%
service/history/queue/timer_queue_processor_base.go	1	77.26%
client/history/client.go	2	38.1%
client/history/metricClient.go	2	45.3%
common/task/weightedRoundRobinTaskScheduler.go	2	88.6%
service/history/handler.go	2	47.15%
common/cache/lru.go	3	90.73%
common/task/fifoTaskScheduler.go	3	84.54%
service/history/queue/timer_gate.go	3	95.83%
service/history/task/fetcher.go	3	91.75%
service/frontend/workflowHandler.go	4	59.86%

Totals
Change from base Build 0186b663-3828-4fa7-95a9-b5b2060c7cc3:	-0.1%
Covered Lines:	85198
Relevant Lines:	149251

💛 - Coveralls

… into Attribute

demirkayaender · 2023-03-02T15:30:33Z

service/frontend/accessControlledHandler.go

+func isManual(ctx context.Context) bool {
+	callContext := yarpc.CallFromContext(ctx)
+	caller := callContext.Caller()
+	return caller == "cadence-cli" || caller == "cadence-web" || caller == "cadence-ui"


Could you add a comment above that sometime in the future we should also validate the caller.

demirkayaender · 2023-03-02T15:33:14Z

common/types/shared.go

@@ -5572,6 +5688,50 @@ type SignalWithStartWorkflowExecutionRequest struct {
 	JitterStartSeconds                  *int32                 `json:"jitterStartSeconds,omitempty"`
 }

+func (v *SignalWithStartWorkflowExecutionRequest) BodyToString() string {
+	cloneRequest := SignalWithStartWorkflowExecutionRequest{


Add a comment here why you are manually assigning attributes. You are trying to prevent logging workflow params, i.e. user inputs.

demirkayaender · 2023-03-02T15:33:23Z

common/types/shared.go

+		RequestID:    v.RequestID,
+		CronSchedule: v.CronSchedule,
+	}
+	// deep copy the value within pointers


demirkayaender · 2023-03-02T15:35:43Z

common/authorization/authorizer.go

@@ -58,6 +58,7 @@ type (
 		WorkflowType *types.WorkflowType
 		TaskList     *types.TaskList
 		Permission   Permission
+		RequestBody  ManualRequestBody


Maybe let's call ManualRequestBody as FilteredRequestBody and add a comment next to it saying "request object except for data inputs"

demirkayaender · 2023-03-02T15:37:43Z

common/types/shared.go

+		RequestID:    v.RequestID,
+		CronSchedule: v.CronSchedule,
+	}
+	// deep copy the value within pointers


same here, explain why you are doing this manually in a sentence

demirkayaender · 2023-03-02T15:38:04Z

common/types/shared.go

+		Identity:   v.Identity,
+		RequestID:  v.RequestID,
+	}
+	// deep copy the value within a pointer


demirkayaender · 2023-03-02T15:42:51Z

service/frontend/accessControlledHandler.go

@@ -706,6 +818,11 @@ func (a *AccessControlledWorkflowHandler) StartWorkflowExecution(
 		Permission:   authorization.PermissionWrite,
 		WorkflowType: request.WorkflowType,
 	}
+
+	if isManual(ctx) {
+		attr.RequestBody = request


For all the requests that can contain PII, add a comment here that the authorizer plugin should use the new function you introduced while logging requests to avoid revealing private information.

demirkayaender · 2023-03-02T15:46:29Z

common/types/admin.go

@@ -26,6 +26,10 @@ type AddSearchAttributeRequest struct {
 	SecurityToken   string                      `json:"securityToken,omitempty"`
 }

+func (v *AddSearchAttributeRequest) BodyToString() string {


The naming here is a bit misleading. It's not really stringifying the struct, it's outputting a string for logging. You probably rename this to something like "SerializeForLogging". Same for the util function.

Shaddoll · 2023-03-03T17:59:45Z

common/types/shared.go

@@ -5572,6 +5688,53 @@ type SignalWithStartWorkflowExecutionRequest struct {
 	JitterStartSeconds                  *int32                 `json:"jitterStartSeconds,omitempty"`
 }

+func (v *SignalWithStartWorkflowExecutionRequest) SerializeForLogging() string {
+	// making a deep copy of the struct for manually assigning attributes
+	cloneRequest := SignalWithStartWorkflowExecutionRequest{


This is not a good pattern. These logic is hard to maintain.
Consider using json encoding to serialize the structs.

Shaddoll · 2023-03-03T21:36:39Z

common/types/shared.go

+
+// SerializeRequest Serialize an arbitrary request for logging
+func SerializeRequest(request interface{}) string {
+	res, _ := json.Marshal(request)


It's not good to ignore the error here.

…zeForLogging function

Shaddoll · 2023-03-03T23:31:46Z

common/types/shared.go

@@ -5925,6 +6109,10 @@ type RestartWorkflowExecutionRequest struct {
 	Identity          string             `json:"identity,omitempty"`
 }

+func (v *RestartWorkflowExecutionRequest) SerializeForLogging() (string, error) {
+	return SerializeRequest(*v)


you can pass the pointer as the parameter, which will save a deep copy

Shaddoll · 2023-03-03T23:34:32Z

service/frontend/accessControlledAdminHandler.go

@@ -251,6 +306,11 @@ func (a *AccessControlledWorkflowAdminHandler) PurgeDLQMessages(ctx context.Cont
 		APIName:    "PurgeDLQMessages",
 		Permission: authorization.PermissionAdmin,
 	}
+
+	if isManual(ctx) {


it seems that it's worth moving isManual check into isAuthorized

Since we change to store a pointer instead of a struct in Attributes, as we discussed, we don't need to check if it is manual any more. Because a pointer won't occupy too much space.

…equestbody into Attribute for auditing (uber#5124)" This reverts commit c462ec0.

…equestbody into Attribute for auditing (#5124)" (#5145) This reverts commit c462ec0.

bowenxia added 9 commits February 22, 2023 16:40

Revert "CDNC-2088 (uber#5094) Add attempt-count to task processing logs"

b98c07b

This reverts commit fbe72b3.

experiment on adding audit info (has errorsgit status)

0a1b61f

Merge branch 'uber:master' into CDNC-2946-Add-Audit-Info

1953ffe

move AuditInfo into Attr

acc3fed

implement requestBody Struct, implement it using requests, add it int…

4a05932

…o Attribute to use this field for auditing manual accesses

Merge branch 'master' into CDNC-2946-Add-Audit-Info

e3289ec

change struct name to be ManualRequestBody to avoid conflict

f202aa1

change any to interface to solve a compiling error

24c792b

rerun make go-generate && make fmt && make lint && make copyright for…

8939918

… auto generated code

bowenxia added 4 commits March 1, 2023 14:20

change the PII-filtered request to still be a requset instead of a map

5a0894b

add a utility function to check if access is manual, add request body…

839db23

… into Attribute

Merge branch 'master' into CDNC-2946-Add-Audit-Info

80df291

update with make go-generate && make fmt && make lint && make copyright

b7006fd

demirkayaender reviewed Mar 2, 2023

View reviewed changes

update comments and naming

e57f774

Shaddoll reviewed Mar 3, 2023

View reviewed changes

change Sprintf to marshal

2e120ea

Shaddoll reviewed Mar 3, 2023

View reviewed changes

bowenxia added 3 commits March 3, 2023 14:16

add error handling for json.marshal, pass it to the caller of Seriali…

2e5d4d7

…zeForLogging function

add a new log tag for actor email address

234e3c6

update interface function error handling

fcc5429

Shaddoll reviewed Mar 3, 2023

View reviewed changes

bowenxia added 3 commits March 5, 2023 22:49

pass in a pointer for serialization instead of a struct

6428a9e

Merge branch 'master' into CDNC-2946-Add-Audit-Info

f891ee7

delete the isManual checks when creating Attributes

263d9b9

Shaddoll approved these changes Mar 6, 2023

View reviewed changes

bowenxia merged commit c462ec0 into uber:master Mar 6, 2023

bowenxia deleted the CDNC-2946-Add-Audit-Info branch March 6, 2023 20:26

bowenxia added a commit to bowenxia/cadence that referenced this pull request Mar 10, 2023

Revert "create RequestBody struct, implement it using requests, add r…

2f6321c

…equestbody into Attribute for auditing (uber#5124)" This reverts commit c462ec0.

bowenxia mentioned this pull request Mar 10, 2023

Revert "create RequestBody struct, implement it using requests, add r… #5145

Merged

bowenxia added a commit that referenced this pull request Mar 10, 2023

Revert "create RequestBody struct, implement it using requests, add r…

434bcee

…equestbody into Attribute for auditing (#5124)" (#5145) This reverts commit c462ec0.

bowenxia mentioned this pull request Mar 10, 2023

Adding request body into Attributes for auditing purpose with PII fields are filtered #5151

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create RequestBody struct, implement it using requests, add requestbody into Attribute for auditing #5124

create RequestBody struct, implement it using requests, add requestbody into Attribute for auditing #5124

bowenxia commented Mar 1, 2023 •

edited

Loading

coveralls commented Mar 1, 2023 •

edited

Loading

demirkayaender Mar 2, 2023

bowenxia Mar 2, 2023

demirkayaender Mar 2, 2023

bowenxia Mar 2, 2023

demirkayaender Mar 2, 2023

bowenxia Mar 2, 2023

demirkayaender Mar 2, 2023 •

edited

Loading

bowenxia Mar 2, 2023

demirkayaender Mar 2, 2023 •

edited

Loading

bowenxia Mar 2, 2023

demirkayaender Mar 2, 2023

bowenxia Mar 2, 2023

demirkayaender Mar 2, 2023 •

edited

Loading

bowenxia Mar 2, 2023

demirkayaender Mar 2, 2023

bowenxia Mar 2, 2023

Shaddoll Mar 3, 2023

bowenxia Mar 3, 2023

Shaddoll Mar 3, 2023

Shaddoll Mar 3, 2023

bowenxia Mar 6, 2023

Shaddoll Mar 3, 2023

bowenxia Mar 6, 2023

create RequestBody struct, implement it using requests, add requestbody into Attribute for auditing #5124

create RequestBody struct, implement it using requests, add requestbody into Attribute for auditing #5124

Conversation

bowenxia commented Mar 1, 2023 • edited Loading

coveralls commented Mar 1, 2023 • edited Loading

Pull Request Test Coverage Report for Build 0186b857-7512-4cad-ad94-15da898f9b53

💛 - Coveralls

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

demirkayaender Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demirkayaender Mar 2, 2023 • 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

demirkayaender Mar 2, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowenxia commented Mar 1, 2023 •

edited

Loading

coveralls commented Mar 1, 2023 •

edited

Loading

demirkayaender Mar 2, 2023 •

edited

Loading

demirkayaender Mar 2, 2023 •

edited

Loading

demirkayaender Mar 2, 2023 •

edited

Loading