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

authz: add conversion of json to RBAC Audit Logging config #6192

Merged
merged 25 commits into from Apr 26, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
8b9f59a
Add conversion of json to RBAC Audit Logging config
gtcooke94 Apr 6, 2023
56ab1cd
Swap to passing around references instead of copied values
gtcooke94 Apr 14, 2023
0f3b6e7
Change v3 to v3corepb
gtcooke94 Apr 14, 2023
6bff58b
go mod tidy compat=1.17
gtcooke94 Apr 14, 2023
9b3ab47
go mod tidy in examples
gtcooke94 Apr 14, 2023
bbafb89
mod tidy
gtcooke94 Apr 14, 2023
5dad66e
go mod tidy compat=1.17
gtcooke94 Apr 14, 2023
73b1390
more tidy
gtcooke94 Apr 14, 2023
27cfe85
replace panic with t.fatal
gtcooke94 Apr 17, 2023
c8751d3
return nil when error
gtcooke94 Apr 17, 2023
441fb5c
lowercase start of error message
gtcooke94 Apr 17, 2023
2326e9f
remove redundant change
gtcooke94 Apr 17, 2023
abcb93f
Fix test logging hard-coding problem
gtcooke94 Apr 17, 2023
3f57e69
Changed behavior of missing configs per PR discussion
gtcooke94 Apr 18, 2023
b7863ee
Change parser to use map from generated proto go file
gtcooke94 Apr 18, 2023
48a875e
ALLOW and DENY filter should get separate audit logger config
gtcooke94 Apr 18, 2023
edf40f1
Small change remove and else
gtcooke94 Apr 18, 2023
551ced9
Construct and allow and denyproto from the beginning
gtcooke94 Apr 19, 2023
1718c45
Merge branch 'master' into AuditLoggingRBACTranslator
gtcooke94 Apr 19, 2023
dad7293
compat 1.17 in examples
gtcooke94 Apr 19, 2023
7b9e96e
more 1.17 compat
gtcooke94 Apr 19, 2023
1f4c0c0
Address PR comments
gtcooke94 Apr 20, 2023
ec7b6bd
Swap so we have to hav an empty custom config, no nil allowed
gtcooke94 Apr 21, 2023
392b6b1
Change variable names
gtcooke94 Apr 25, 2023
601be9a
Remove newline
gtcooke94 Apr 25, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
77 changes: 70 additions & 7 deletions authz/rbac_translator.go
Expand Up @@ -28,9 +28,12 @@ import (
"fmt"
"strings"

v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
v3rbacpb "github.com/envoyproxy/go-control-plane/envoy/config/rbac/v3"
v3routepb "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
v3matcherpb "github.com/envoyproxy/go-control-plane/envoy/type/matcher/v3"
"google.golang.org/protobuf/types/known/anypb"
"google.golang.org/protobuf/types/known/structpb"
)

type header struct {
Expand All @@ -53,11 +56,23 @@ type rule struct {
Request request
}

type auditLogger struct {
Name string
Config structpb.Struct
gtcooke94 marked this conversation as resolved.
Show resolved Hide resolved
IsOptional bool `json:"is_optional"`
}

type auditLoggingOptions struct {
AuditCondition string `json:"audit_condition"`
AuditLoggers []auditLogger `json:"audit_loggers"`
}

// Represents the SDK authorization policy provided by user.
type authorizationPolicy struct {
Name string
DenyRules []rule `json:"deny_rules"`
AllowRules []rule `json:"allow_rules"`
Name string
DenyRules []rule `json:"deny_rules"`
AllowRules []rule `json:"allow_rules"`
AuditLoggingOptions auditLoggingOptions `json:"audit_logging_options"`
}

func principalOr(principals []*v3rbacpb.Principal) *v3rbacpb.Principal {
Expand Down Expand Up @@ -266,6 +281,48 @@ func parseRules(rules []rule, prefixName string) (map[string]*v3rbacpb.Policy, e
return policies, nil
}

func parseAuditLoggingOptions(options auditLoggingOptions) (*v3rbacpb.RBAC_AuditLoggingOptions, error) {
optionsRbac := v3rbacpb.RBAC_AuditLoggingOptions{}
if options.AuditCondition == "" && len(options.AuditLoggers) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Audit condition should default to none if not present. No loggers should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed in other thread, keeping open to track

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL

// No audit logger parsed - fine
return &optionsRbac, nil
} else if options.AuditCondition == "" {
return &optionsRbac, fmt.Errorf("AuditLogger config present but missing AuditCondition")
} else if len(options.AuditLoggers) == 0 {
return &optionsRbac, fmt.Errorf("AuditCondition present but missing AuditLogger config")
}

switch options.AuditCondition {
case "NONE":
gtcooke94 marked this conversation as resolved.
Show resolved Hide resolved
optionsRbac.AuditCondition = v3rbacpb.RBAC_AuditLoggingOptions_NONE
case "ON_DENY":
optionsRbac.AuditCondition = v3rbacpb.RBAC_AuditLoggingOptions_ON_DENY
case "ON_ALLOW":
optionsRbac.AuditCondition = v3rbacpb.RBAC_AuditLoggingOptions_ON_ALLOW
case "ON_DENY_AND_ALLOW":
optionsRbac.AuditCondition = v3rbacpb.RBAC_AuditLoggingOptions_ON_DENY_AND_ALLOW
default:
return &optionsRbac, fmt.Errorf("Failed to parse AuditCondition %v. Allowed values {NONE, ON_DENY, ON_ALLOW, ON_DENY_AND_ALLOW}", options.AuditCondition)
rockspore marked this conversation as resolved.
Show resolved Hide resolved
}

for i := range options.AuditLoggers {
gtcooke94 marked this conversation as resolved.
Show resolved Hide resolved
config := &options.AuditLoggers[i]
customConfig, err := anypb.New(&config.Config)
if err != nil {
return &optionsRbac, fmt.Errorf("Error parsing custom audit logger config: %v", err)
rockspore marked this conversation as resolved.
Show resolved Hide resolved
}
logger := &v3corepb.TypedExtensionConfig{Name: config.Name, TypedConfig: customConfig}
rbacConfig := v3rbacpb.RBAC_AuditLoggingOptions_AuditLoggerConfig{
IsOptional: config.IsOptional,
AuditLogger: logger,
}
optionsRbac.LoggerConfigs = append(optionsRbac.LoggerConfigs, &rbacConfig)
}

return &optionsRbac, nil

gtcooke94 marked this conversation as resolved.
Show resolved Hide resolved
}

// translatePolicy translates SDK authorization policy in JSON format to two
// Envoy RBAC polices (deny followed by allow policy) or only one Envoy RBAC
// allow policy. If the input policy cannot be parsed or is invalid, an error
Expand All @@ -283,22 +340,28 @@ func translatePolicy(policyStr string) ([]*v3rbacpb.RBAC, error) {
if len(policy.AllowRules) == 0 {
return nil, fmt.Errorf(`"allow_rules" is not present`)
}
auditLoggers, err := parseAuditLoggingOptions(policy.AuditLoggingOptions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a mapping from this audit condition to the conditions of deny and allow RBACs. Because we only want the same RPC to be logged at most once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite get what you wrote here and how it applies to this line, can you go into more detail?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, you cannot use the audit condition from the policy in both RBACs, because we need to make sure one RPC can at most be logged one time.

There is a table in the gRFC. As an example, if we want to audit on allow, then the deny RBAC should have no audit condition (I just realized right now that in fact, it doesn't even need to hold any loggers in this case), and the allow RBAC will have ON_ALLOW. The evaluation order is always deny -> allow, and short-circuited if applicable.

Certainly you don't have to change this line to fix it. But this was where I realized the problem.

Copy link
Contributor Author

@gtcooke94 gtcooke94 Apr 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I just need some additional logic here that constructs two separate audit logging configs to add to each RBAC with different conditions per the table and the input AuditCondition?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I've changed to this, it was a sizeable code change in the tests because I made every test have both an ALLOW and DENY filter to be able to test that they are mapped correctly

if err != nil {
return nil, err
}
rbacs := make([]*v3rbacpb.RBAC, 0, 2)
if len(policy.DenyRules) > 0 {
denyPolicies, err := parseRules(policy.DenyRules, policy.Name)
if err != nil {
return nil, fmt.Errorf(`"deny_rules" %v`, err)
}
denyRBAC := &v3rbacpb.RBAC{
Action: v3rbacpb.RBAC_DENY,
Policies: denyPolicies,
Action: v3rbacpb.RBAC_DENY,
Policies: denyPolicies,
AuditLoggingOptions: auditLoggers,
}
rbacs = append(rbacs, denyRBAC)
}
allowPolicies, err := parseRules(policy.AllowRules, policy.Name)
if err != nil {
return nil, fmt.Errorf(`"allow_rules" %v`, err)
}
allowRBAC := &v3rbacpb.RBAC{Action: v3rbacpb.RBAC_ALLOW, Policies: allowPolicies}
return append(rbacs, allowRBAC), nil
allowRBAC := &v3rbacpb.RBAC{Action: v3rbacpb.RBAC_ALLOW, Policies: allowPolicies, AuditLoggingOptions: auditLoggers}
rbacs = append(rbacs, allowRBAC)
rockspore marked this conversation as resolved.
Show resolved Hide resolved
return rbacs, nil
}