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.

Sign up for GitHub

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

security/authorization: add integration tests #3809

Merged
merged 11 commits into from
Aug 26, 2020
Merged

security/authorization: add integration tests #3809

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6117518
Added source.principal to cel.Env declaration
Aug 12, 2020
7a03062
Added engine integration tests + helper functions + mock classes
Aug 12, 2020
fb90c15
Addressed PR comments: moved env variable, removed wantErr from testi…
Aug 18, 2020
e71fa22
Rebased branch to use dreidenbach's work
Aug 20, 2020
18e4bcd
Changed testing to use grpctest, changed t.Errorf to t.Fatalf, moved …
Aug 20, 2020
0e5a6a8
Addressed cosmetic PR comments
Aug 21, 2020
b648f74
Check error occurrence rather than exact error string, uses cmp.Equal…
Aug 21, 2020
1960f7a
Remove 'tc := tc' lines
Aug 21, 2020
8cbd26a
Changed test error messages to be more descriptive
Aug 21, 2020
9c9313a
Removed zero values from table-driven tests
Aug 22, 2020
e19d28e
Replaced programMock with fakeProgram
Aug 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions security/authorization/engine/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ import (
"strconv"

pb "github.com/envoyproxy/go-control-plane/envoy/config/rbac/v2"
cel "github.com/google/cel-go/cel"
"github.com/google/cel-go/cel"
"github.com/google/cel-go/checker/decls"
types "github.com/google/cel-go/common/types"
interpreter "github.com/google/cel-go/interpreter"
"github.com/google/cel-go/common/types"
"github.com/google/cel-go/interpreter"
expr "google.golang.org/genproto/googleapis/api/expr/v1alpha1"
"google.golang.org/grpc/grpclog"
"google.golang.org/grpc/metadata"
Expand Down Expand Up @@ -324,6 +324,7 @@ func NewAuthorizationEngine(allow, deny *pb.RBAC) (*AuthorizationEngine, error)
decls.NewVar("destination.address", decls.String),
decls.NewVar("destination.port", decls.Int),
decls.NewVar("connection.uri_san_peer_certificate", decls.String),
decls.NewVar("source.principal", decls.String),
),
)
if err != nil {
Expand Down
216 changes: 187 additions & 29 deletions security/authorization/engine/engine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,23 @@ import (
"testing"

pb "github.com/envoyproxy/go-control-plane/envoy/config/rbac/v2"
cel "github.com/google/cel-go/cel"
"github.com/google/cel-go/cel"
"github.com/google/cel-go/checker/decls"
wflms20110333 marked this conversation as resolved.
Show resolved Hide resolved
"github.com/google/cel-go/common/types"
"github.com/google/cel-go/common/types/ref"
interpreter "github.com/google/cel-go/interpreter"
"github.com/google/cel-go/interpreter"
"github.com/google/go-cmp/cmp"
expr "google.golang.org/genproto/googleapis/api/expr/v1alpha1"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/internal/grpctest"
"google.golang.org/grpc/peer"
"google.golang.org/grpc/status"
)

type s struct {
grpctest.Tester
}

type programMock struct {
wflms20110333 marked this conversation as resolved.
Show resolved Hide resolved
out ref.Val
err error
Expand Down Expand Up @@ -66,6 +75,18 @@ func (mock valMock) Value() interface{} {
return mock.val
}

type addrMock struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to use a net.IPAddr instead? I don't see this mock addr adding much value here.

addr string
}

func (mock addrMock) Network() string {
return "tcp"
}

func (mock addrMock) String() string {
return mock.addr
}

var (
emptyActivation = interpreter.EmptyActivation()
unsuccessfulProgram = programMock{out: nil, err: status.Errorf(codes.InvalidArgument, "Unsuccessful program evaluation")}
Expand All @@ -92,59 +113,60 @@ var (
}}
)

func TestNewAuthorizationEngine(t *testing.T) {
func Test(t *testing.T) {
grpctest.RunSubTests(t, s{})
}

func (s) TestNewAuthorizationEngine(t *testing.T) {
tests := map[string]struct {
allow *pb.RBAC
deny *pb.RBAC
wantErr string
wantErr bool
errStr string
}{
"too few rbacs": {
allow: nil,
deny: nil,
wantErr: "at least one of allow, deny must be non-nil",
wantErr: true,
errStr: "Expected error: at least one of allow, deny must be non-nil",
},
"one rbac allow": {
allow: &pb.RBAC{Action: pb.RBAC_ALLOW},
deny: nil,
wantErr: "",
wantErr: false,
wflms20110333 marked this conversation as resolved.
Show resolved Hide resolved
errStr: "Expected 1 ALLOW RBAC to be successful",
},
"one rbac deny": {
allow: nil,
deny: &pb.RBAC{Action: pb.RBAC_DENY},
wantErr: "",
wantErr: false,
errStr: "Expected 1 DENY RBAC to be successful",
},
"two rbacs": {
allow: &pb.RBAC{Action: pb.RBAC_ALLOW},
deny: &pb.RBAC{Action: pb.RBAC_DENY},
wantErr: "",
wantErr: false,
errStr: "Expected 2 RBACs (DENY + ALLOW) to be successful",
},
"wrong rbac actions": {
allow: &pb.RBAC{Action: pb.RBAC_DENY},
deny: nil,
wantErr: "allow must have action ALLOW, deny must have action DENY",
wantErr: true,
errStr: "Expected error: allow must have action ALLOW, deny must have action DENY",
},
}

for name, tc := range tests {
t.Run(name, func(t *testing.T) {
_, gotErr := NewAuthorizationEngine(tc.allow, tc.deny)
if tc.wantErr == "" && gotErr == nil {
return
}
if gotErr == nil || gotErr.Error() != tc.wantErr {
t.Errorf(tc.errStr)
if (gotErr != nil) != tc.wantErr {
t.Fatal(tc.errStr)
}
})
}
}

func TestGetDecision(t *testing.T) {
func (s) TestGetDecision(t *testing.T) {
tests := map[string]struct {
engine *policyEngine
match bool
Expand Down Expand Up @@ -175,13 +197,13 @@ func TestGetDecision(t *testing.T) {
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
if got := getDecision(tc.engine, tc.match); got != tc.want {
t.Errorf("Expected %v, instead got %v", tc.want, got)
t.Fatalf("getDecision(%v, %v) = (%v), want (%v)", tc.engine, tc.match, got, tc.want)
}
})
}
}

func TestPolicyEngineEvaluate(t *testing.T) {
func (s) TestPolicyEngineEvaluate(t *testing.T) {
tests := map[string]struct {
engine *policyEngine
activation interpreter.Activation
Expand Down Expand Up @@ -218,54 +240,190 @@ func TestPolicyEngineEvaluate(t *testing.T) {
t.Run(name, func(t *testing.T) {
gotDecision, gotPolicyNames := tc.engine.evaluate(tc.activation)
sort.Strings(gotPolicyNames)
if gotDecision != tc.wantDecision || !reflect.DeepEqual(gotPolicyNames, tc.wantPolicyNames) {
t.Errorf("Expected (%v, %v), instead got (%v, %v)", tc.wantDecision, tc.wantPolicyNames, gotDecision, gotPolicyNames)
if gotDecision != tc.wantDecision || !cmp.Equal(gotPolicyNames, tc.wantPolicyNames) {
t.Fatalf("policyEngine.evaluate(%v, %v) = (%v, %v), want (%v, %v)", tc.engine, tc.activation, gotDecision, gotPolicyNames, tc.wantDecision, tc.wantPolicyNames)
}
})
}
}

func TestAuthorizationEngineEvaluate(t *testing.T) {
func (s) TestAuthorizationEngineEvaluate(t *testing.T) {
tests := map[string]struct {
engine *AuthorizationEngine
authArgs *AuthorizationArgs
wantAuthDecision *AuthorizationDecision
wantErr error
}{
"allow match": {
engine: &AuthorizationEngine{allow: allowMatchEngine},
authArgs: &AuthorizationArgs{},
wantAuthDecision: &AuthorizationDecision{decision: DecisionAllow, policyNames: []string{"allow match policy2"}},
wantErr: nil,
},
"deny fail": {
engine: &AuthorizationEngine{deny: denyFailEngine},
authArgs: &AuthorizationArgs{},
wantAuthDecision: &AuthorizationDecision{decision: DecisionAllow, policyNames: []string{}},
wantErr: nil,
},
"first engine unknown": {
engine: &AuthorizationEngine{allow: allowMatchEngine, deny: denyUnknownEngine},
authArgs: &AuthorizationArgs{},
wantAuthDecision: &AuthorizationDecision{decision: DecisionUnknown, policyNames: []string{"deny unknown policy2", "deny unknown policy3"}},
wantErr: nil,
},
"second engine match": {
engine: &AuthorizationEngine{allow: allowMatchEngine, deny: denyFailEngine},
authArgs: &AuthorizationArgs{},
wantAuthDecision: &AuthorizationDecision{decision: DecisionAllow, policyNames: []string{"allow match policy2"}},
wantErr: nil,
},
}

for name, tc := range tests {
t.Run(name, func(t *testing.T) {
gotAuthDecision, gotErr := tc.engine.Evaluate(tc.authArgs)
sort.Strings(gotAuthDecision.policyNames)
if tc.wantErr != nil && (gotErr == nil || gotErr.Error() != tc.wantErr.Error()) {
t.Errorf("Expected error to be %v, instead got %v", tc.wantErr, gotErr)
} else if tc.wantErr == nil && (gotErr != nil || gotAuthDecision.decision != tc.wantAuthDecision.decision || !reflect.DeepEqual(gotAuthDecision.policyNames, tc.wantAuthDecision.policyNames)) {
t.Errorf("Expected authorization decision to be (%v, %v), instead got (%v, %v)", tc.wantAuthDecision.decision, tc.wantAuthDecision.policyNames, gotAuthDecision.decision, gotAuthDecision.policyNames)
if gotErr != nil || gotAuthDecision.decision != tc.wantAuthDecision.decision || !cmp.Equal(gotAuthDecision.policyNames, tc.wantAuthDecision.policyNames) {
t.Fatalf("AuthorizationEngine.Evaluate(%v, %v) = (%v, %v), want (%v, %v)", tc.engine, tc.authArgs, gotAuthDecision, gotErr, tc.wantAuthDecision, nil)
}
})
}
}

func (s) TestIntegration(t *testing.T) {
declarations := []*expr.Decl{
decls.NewVar("request.url_path", decls.String),
decls.NewVar("request.host", decls.String),
decls.NewVar("request.method", decls.String),
decls.NewVar("request.headers", decls.NewMapType(decls.String, decls.String)),
decls.NewVar("source.address", decls.String),
decls.NewVar("source.port", decls.Int),
decls.NewVar("destination.address", decls.String),
decls.NewVar("destination.port", decls.Int),
decls.NewVar("connection.uri_san_peer_certificate", decls.String),
decls.NewVar("source.principal", decls.String),
}

tests := map[string]struct {
allow *pb.RBAC
deny *pb.RBAC
authArgs *AuthorizationArgs
wantAuthDecision *AuthorizationDecision
}{
"ALLOW engine: DecisionAllow": {
allow: &pb.RBAC{Action: pb.RBAC_ALLOW, Policies: map[string]*pb.Policy{
"url_path starts with": {Condition: compileStringToExpr("request.url_path.startsWith('/pkg.service/test')", declarations)},
}},
deny: nil,
wflms20110333 marked this conversation as resolved.
Show resolved Hide resolved
authArgs: &AuthorizationArgs{fullMethod: "/pkg.service/test/method"},
wantAuthDecision: &AuthorizationDecision{decision: DecisionAllow, policyNames: []string{"url_path starts with"}},
},
"ALLOW engine: DecisionUnknown": {
allow: &pb.RBAC{Action: pb.RBAC_ALLOW, Policies: map[string]*pb.Policy{
"url_path and uri_san_peer_certificate": {Condition: compileStringToExpr("request.url_path == '/pkg.service/test' && connection.uri_san_peer_certificate == 'cluster/ns/default/sa/admin'", declarations)},
"source port": {Condition: compileStringToExpr("source.port == 8080", declarations)},
}},
deny: nil,
authArgs: &AuthorizationArgs{peerInfo: &peer.Peer{Addr: addrMock{addr: "192.0.2.1:25"}}, fullMethod: "/pkg.service/test"},
wantAuthDecision: &AuthorizationDecision{decision: DecisionUnknown, policyNames: []string{"url_path and uri_san_peer_certificate"}},
},
"ALLOW engine: DecisionDeny": {
allow: &pb.RBAC{Action: pb.RBAC_ALLOW, Policies: map[string]*pb.Policy{
"url_path": {Condition: compileStringToExpr("request.url_path == '/pkg.service/test'", declarations)},
}},
deny: nil,
authArgs: &AuthorizationArgs{fullMethod: "/pkg.service/test/method"},
wantAuthDecision: &AuthorizationDecision{decision: DecisionDeny, policyNames: []string{}},
},
"DENY engine: DecisionAllow": {
allow: nil,
deny: &pb.RBAC{Action: pb.RBAC_DENY, Policies: map[string]*pb.Policy{
"url_path and uri_san_peer_certificate": {Condition: compileStringToExpr("request.url_path == '/pkg.service/test' && connection.uri_san_peer_certificate == 'cluster/ns/default/sa/admin'", declarations)},
}},
authArgs: &AuthorizationArgs{fullMethod: "/pkg.service/test/method"},
wantAuthDecision: &AuthorizationDecision{decision: DecisionAllow, policyNames: []string{}},
},
"DENY engine: DecisionUnknown": {
allow: nil,
deny: &pb.RBAC{Action: pb.RBAC_DENY, Policies: map[string]*pb.Policy{
"destination address": {Condition: compileStringToExpr("destination.address == '192.0.3.1'", declarations)},
"source port": {Condition: compileStringToExpr("source.port == 8080", declarations)},
}},
authArgs: &AuthorizationArgs{peerInfo: &peer.Peer{Addr: addrMock{addr: "192.0.2.1:25"}}, fullMethod: "/pkg.service/test"},
wantAuthDecision: &AuthorizationDecision{decision: DecisionUnknown, policyNames: []string{"destination address"}},
},
"DENY engine: DecisionDeny": {
allow: nil,
deny: &pb.RBAC{Action: pb.RBAC_DENY, Policies: map[string]*pb.Policy{
"destination address": {Condition: compileStringToExpr("destination.address == '192.0.3.1'", declarations)},
"source address or source port": {Condition: compileStringToExpr("source.address == '192.0.4.1' || source.port == 8080", declarations)},
}},
authArgs: &AuthorizationArgs{peerInfo: &peer.Peer{Addr: addrMock{addr: "192.0.2.1:8080"}}, fullMethod: "/pkg.service/test"},
wantAuthDecision: &AuthorizationDecision{decision: DecisionDeny, policyNames: []string{"source address or source port"}},
},
"DENY ALLOW engine: DecisionDeny from DENY policy": {
allow: &pb.RBAC{Action: pb.RBAC_ALLOW, Policies: map[string]*pb.Policy{
"url_path starts with": {Condition: compileStringToExpr("request.url_path.startsWith('/pkg.service/test')", declarations)},
}},
deny: &pb.RBAC{Action: pb.RBAC_DENY, Policies: map[string]*pb.Policy{
"destination address": {Condition: compileStringToExpr("destination.address == '192.0.3.1'", declarations)},
"source address or source port": {Condition: compileStringToExpr("source.address == '192.0.4.1' || source.port == 8080", declarations)},
}},
authArgs: &AuthorizationArgs{peerInfo: &peer.Peer{Addr: addrMock{addr: "192.0.2.1:8080"}}, fullMethod: "/pkg.service/test"},
wantAuthDecision: &AuthorizationDecision{decision: DecisionDeny, policyNames: []string{"source address or source port"}},
},
"DENY ALLOW engine: DecisionUnknown from DENY policy": {
allow: &pb.RBAC{Action: pb.RBAC_ALLOW, Policies: map[string]*pb.Policy{
"url_path starts with": {Condition: compileStringToExpr("request.url_path.startsWith('/pkg.service/test')", declarations)},
}},
deny: &pb.RBAC{Action: pb.RBAC_DENY, Policies: map[string]*pb.Policy{
"destination address": {Condition: compileStringToExpr("destination.address == '192.0.3.1'", declarations)},
"source port and destination port": {Condition: compileStringToExpr("source.port == 8080 && destination.port == 1234", declarations)},
}},
authArgs: &AuthorizationArgs{peerInfo: &peer.Peer{Addr: addrMock{addr: "192.0.2.1:8080"}}, fullMethod: "/pkg.service/test/method"},
wantAuthDecision: &AuthorizationDecision{decision: DecisionUnknown, policyNames: []string{"destination address", "source port and destination port"}},
},
"DENY ALLOW engine: DecisionAllow from ALLOW policy": {
allow: &pb.RBAC{Action: pb.RBAC_ALLOW, Policies: map[string]*pb.Policy{
"method or url_path starts with": {Condition: compileStringToExpr("request.method == 'POST' || request.url_path.startsWith('/pkg.service/test')", declarations)},
}},
deny: &pb.RBAC{Action: pb.RBAC_DENY, Policies: map[string]*pb.Policy{
"source address": {Condition: compileStringToExpr("source.address == '192.0.3.1'", declarations)},
"source port and url_path": {Condition: compileStringToExpr("source.port == 8080 && request.url_path == 'pkg.service/test'", declarations)},
}},
authArgs: &AuthorizationArgs{peerInfo: &peer.Peer{Addr: addrMock{addr: "192.0.2.1:8080"}}, fullMethod: "/pkg.service/test/method"},
wantAuthDecision: &AuthorizationDecision{decision: DecisionAllow, policyNames: []string{"method or url_path starts with"}},
},
"DENY ALLOW engine: DecisionUnknown from ALLOW policy": {
allow: &pb.RBAC{Action: pb.RBAC_ALLOW, Policies: map[string]*pb.Policy{
"url_path starts with and method": {Condition: compileStringToExpr("request.url_path.startsWith('/pkg.service/test') && request.method == 'POST'", declarations)},
}},
deny: &pb.RBAC{Action: pb.RBAC_DENY, Policies: map[string]*pb.Policy{
"source address": {Condition: compileStringToExpr("source.address == '192.0.3.1'", declarations)},
"source port and url_path": {Condition: compileStringToExpr("source.port == 8080 && request.url_path == 'pkg.service/test'", declarations)},
}},
authArgs: &AuthorizationArgs{peerInfo: &peer.Peer{Addr: addrMock{addr: "192.0.2.1:8080"}}, fullMethod: "/pkg.service/test/method"},
wantAuthDecision: &AuthorizationDecision{decision: DecisionUnknown, policyNames: []string{"url_path starts with and method"}},
},
"DENY ALLOW engine: DecisionDeny from ALLOW policy": {
allow: &pb.RBAC{Action: pb.RBAC_ALLOW, Policies: map[string]*pb.Policy{
"url_path starts with and source port": {Condition: compileStringToExpr("request.url_path.startsWith('/pkg.service/test') && source.port == 1234", declarations)},
}},
deny: &pb.RBAC{Action: pb.RBAC_DENY, Policies: map[string]*pb.Policy{
"source address": {Condition: compileStringToExpr("source.address == '192.0.3.1'", declarations)},
"source port and url_path": {Condition: compileStringToExpr("source.port == 8080 && request.url_path == 'pkg.service/test'", declarations)},
}},
authArgs: &AuthorizationArgs{peerInfo: &peer.Peer{Addr: addrMock{addr: "192.0.2.1:8080"}}, fullMethod: "/pkg.service/test/method"},
wantAuthDecision: &AuthorizationDecision{decision: DecisionDeny, policyNames: []string{}},
},
}

for name, tc := range tests {
t.Run(name, func(t *testing.T) {
wflms20110333 marked this conversation as resolved.
Show resolved Hide resolved
engine, err := NewAuthorizationEngine(tc.allow, tc.deny)
if err != nil {
t.Fatalf("Error constructing authorization engine: %v", err)
}
gotAuthDecision, gotErr := engine.Evaluate(tc.authArgs)
sort.Strings(gotAuthDecision.policyNames)
if gotErr != nil || gotAuthDecision.decision != tc.wantAuthDecision.decision || !cmp.Equal(gotAuthDecision.policyNames, tc.wantAuthDecision.policyNames) {
t.Fatalf("NewAuthorizationEngine(%v, %v).Evaluate(%v) = (%v, %v), want (%v, %v)", tc.allow, tc.deny, tc.authArgs, gotAuthDecision, gotErr, tc.wantAuthDecision, nil)
}
})
}
Expand Down
8 changes: 8 additions & 0 deletions security/authorization/util/util.go → security/authorization/engine/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,11 @@ func compileStringToCheckedExpr(expr string, declarations []*expr.Decl) (*expr.C
}
return checkedExpr, nil
}

func compileStringToExpr(expr string, declarations []*expr.Decl) *expr.Expr {
checkedExpr, err := compileStringToCheckedExpr(expr, declarations)
if err != nil {
logger.Fatalf("error encountered when compiling string to expression: %v", err)
}
return checkedExpr.Expr
}
9 changes: 0 additions & 9 deletions security/authorization/util/util_test.go → security/authorization/engine/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,20 +22,11 @@ import (
"testing"

expr "google.golang.org/genproto/googleapis/api/expr/v1alpha1"
"google.golang.org/grpc/internal/grpctest"

"github.com/google/cel-go/cel"
"github.com/google/cel-go/checker/decls"
)

type s struct {
grpctest.Tester
}

func Test(t *testing.T) {
grpctest.RunSubTests(t, s{})
}

func (s) TestStringConvert(t *testing.T) {
declarations := []*expr.Decl{
decls.NewIdent("request.url_path", decls.String, nil),
Expand Down
Loading