From 6201082d038984e7fe8ff033756cee486ff6679f Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 30 Jun 2021 11:47:56 -0400 Subject: [PATCH 01/36] Finished helper function --- internal/xds/rbac/rbac_engine.go | 91 ++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index 29c96d9fcf20..69d27a04a0fb 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -19,7 +19,12 @@ package rbac import ( + "context" + "crypto/x509" + "errors" + "google.golang.org/grpc/credentials" "net" + "strconv" v3rbacpb "github.com/envoyproxy/go-control-plane/envoy/config/rbac/v3" "google.golang.org/grpc/metadata" @@ -48,6 +53,92 @@ func NewEngine(policy *v3rbacpb.RBAC) (*Engine, error) { return &Engine{policies: policies}, nil } +// Helper functions here which put connection in and out of the context. +// This is used for authorization interceptors, so do we put it here or in authorization package? +type connectionKey struct{} + +func getConnection(ctx context.Context) net.Conn { + conn, _ := ctx.Value(connectionKey{}).(net.Conn) + return conn +} + +// SetConnection adds the connection to the context to be able to get +// information about the destination ip and port for an incoming RPC. +func SetConnection(ctx context.Context, conn net.Conn) context.Context { + return context.WithValue(ctx, connectionKey{}, conn) +} + +// NewRPCData takes a incoming context (should be a context representing state +// needed for server RPC Call with headers and connection piped into it) and the +// method name of the Service being called server side and populates an RPCData +// struct ready to be passed to the RBAC Engine to find a matching policy. +func NewRPCData(ctx context.Context, fullMethod string) (*RPCData, error) { + md, ok := metadata.FromIncomingContext(ctx) + if !ok { + return nil, errors.New("error retrieving metadata from incoming ctx") + } + + pi, ok := peer.FromContext(ctx) + if !ok { + return nil, errors.New("error retrieving peer info from incoming ctx") + } + + // You need the connection in order to find the destination address and port + // of the incoming RPC Call. + conn := getConnection(ctx) + _, dPort, err := net.SplitHostPort(conn.LocalAddr().String()) + if err != nil { + return nil, err + } + dp, err := strconv.ParseUint(dPort, 10, 32) + + tlsState := pi.AuthInfo.(credentials.TLSInfo).State // TODO: Handle errors on type conversion? + pName, err := findPrincipalNameFromCerts(tlsState.PeerCertificates) + if err != nil { + return nil, err + } + + return &RPCData{ + MD: md, + PeerInfo: pi, + FullMethod: fullMethod, + DestinationPort: uint32(dp), + DestinationAddr: conn.LocalAddr(), + PrincipalName: pName, + }, nil +} + +// findPrincipalNameFromCerts extracts the principal name from the TLS +// Certificates according to the RBAC configuration logic - "The URI SAN or DNS +// SAN in that order is used from the certificate, otherwise the subject field +// is used." +func findPrincipalNameFromCerts(certs []*x509.Certificate) (string, error) { + if len(certs) == 0 { + return "", errors.New("there are no certificates present") + } + // Loop through and find URI SAN if present. + for _, cert := range certs { + for _, uriSAN := range cert.URIs { + return uriSAN.String(), nil + } + } + + // Loop through and find DNS SAN if present. + for _, cert := range certs { + for _, dnsSAN := range cert.DNSNames { + return dnsSAN, nil + } + } + + // If neither URI SAN or DNS SAN were present in the certificates, we can + // just return the subject field. + for _, cert := range certs { + return cert.Subject.String(), nil + } + + return "", errors.New("the URI SAN, DNS SAN, or subject field was not found in the certificates") +} + // RPCData wraps data pulled from an incoming RPC that the RBAC engine needs to // find a matching policy. type RPCData struct { From 3345452aac639a7cf6bba5487ce6cfa2c4e0752e Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 30 Jun 2021 11:54:02 -0400 Subject: [PATCH 02/36] Vet --- internal/xds/rbac/rbac_engine.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index 69d27a04a0fb..3719116e3691 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -99,12 +99,12 @@ func NewRPCData(ctx context.Context, fullMethod string) (*RPCData, error) { } return &RPCData{ - MD: md, - PeerInfo: pi, - FullMethod: fullMethod, + MD: md, + PeerInfo: pi, + FullMethod: fullMethod, DestinationPort: uint32(dp), DestinationAddr: conn.LocalAddr(), - PrincipalName: pName, + PrincipalName: pName, }, nil } From c6f6c7c0b69df2292b3030c0f3fd7cb25c2b06eb Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 30 Jun 2021 11:56:01 -0400 Subject: [PATCH 03/36] Vet --- internal/xds/rbac/rbac_engine.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index 3719116e3691..3685c72233ce 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -22,11 +22,11 @@ import ( "context" "crypto/x509" "errors" - "google.golang.org/grpc/credentials" "net" "strconv" v3rbacpb "github.com/envoyproxy/go-control-plane/envoy/config/rbac/v3" + "google.golang.org/grpc/credentials" "google.golang.org/grpc/metadata" "google.golang.org/grpc/peer" ) From d2fdd851ed087cfec556b083cadc6a9477d9d564 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 30 Jun 2021 11:59:01 -0400 Subject: [PATCH 04/36] Vet --- internal/xds/rbac/rbac_engine.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index 3685c72233ce..b60e409f4358 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -91,6 +91,9 @@ func NewRPCData(ctx context.Context, fullMethod string) (*RPCData, error) { return nil, err } dp, err := strconv.ParseUint(dPort, 10, 32) + if err != nil { + return nil, err + } tlsState := pi.AuthInfo.(credentials.TLSInfo).State // TODO: Handle errors on type conversion? pName, err := findPrincipalNameFromCerts(tlsState.PeerCertificates) From 31301be1b4006752cf10fe0a53839b2f53132836 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Thu, 1 Jul 2021 12:24:03 -0400 Subject: [PATCH 05/36] Save progress --- internal/xds/rbac/matchers.go | 62 +++++++++++++++++++++++++-- internal/xds/rbac/rbac_engine.go | 48 ++++++++++++++++++--- internal/xds/rbac/rbac_engine_test.go | 50 +++++++++++++++++++++ 3 files changed, 151 insertions(+), 9 deletions(-) diff --git a/internal/xds/rbac/matchers.go b/internal/xds/rbac/matchers.go index 47be35c1d0d7..c9acc4226810 100644 --- a/internal/xds/rbac/matchers.go +++ b/internal/xds/rbac/matchers.go @@ -370,17 +370,73 @@ func (pm *portMatcher) match(data *RPCData) bool { // subject field is used. If unset, it applies to any user that is // authenticated. authenticatedMatcher implements the matcher interface. type authenticatedMatcher struct { - stringMatcher internalmatcher.StringMatcher + stringMatcher *internalmatcher.StringMatcher } +// Two things of logic here: config can be null, in which case you don't persist a string matcher and it matches any principal name func newAuthenticatedMatcher(authenticatedMatcherConfig *v3rbacpb.Principal_Authenticated) (*authenticatedMatcher, error) { + // Represents this line in the RBAC documentation = "If unset, it applies to + // any user that is authenticated". + if authenticatedMatcherConfig.PrincipalName == nil { + return &authenticatedMatcher{}, nil + } + // This will be split to branching logic, how is that represented in GoLang? stringMatcher, err := internalmatcher.StringMatcherFromProto(authenticatedMatcherConfig.PrincipalName) if err != nil { return nil, err } - return &authenticatedMatcher{stringMatcher: stringMatcher}, nil + return &authenticatedMatcher{stringMatcher: &stringMatcher}, nil } +// This should take TLS Certs and then loop through them... + +// Similar loop to previous: first loop through URI SAN, then loop through DNS SAN, then subject name func (am *authenticatedMatcher) match(data *RPCData) bool { - return am.stringMatcher.Match(data.PrincipalName) + // Represents this line in the RBAC documentation = "If unset, it applies to + // any user that is authenticated". Thus, if a user is authenticated user should + // match. An authenticated user will have a certificate provided. + if am.stringMatcher == nil { + // TODO: Is this correct? If a TLS Certificate is provided, that certificate means + // that that individual was logically authenticated with a key. + if len(data.Certs) != 0 { + return true + } + } + + // Loop through URI SAN's (list will be persisted in data) + for _, cert := range data.Certs { + for _, uriSAN := range cert.URIs { + if am.stringMatcher.Match(uriSAN.String()) { + return true + } + } + } + // Loop through DNS SAN's (list will be persisted in data) + for _, cert := range data.Certs { + for _, dnsSAN := range cert.DNSNames { + if am.stringMatcher.Match(dnsSAN) { + return true + } + } + } + + // Check against subject names + for _, cert := range data.Certs { + if am.stringMatcher.Match(cert.Subject.String()) { + return true + } + } + return false } + +// Tasks +// Authenticated matcher has wrong logic (Done I think nope) +// Change API Layer, which will be a two liner (Done I think... nope) + +// Testing Tasks, will depend on ^^^ +// Unit test helper function to RPC Data (This should be first test written, as might change API Layer of what you need to pass into it which will break VVV) +// This ^^^ unit test will be implicitly tested though if we change API Layer, it'll hit that codepath (NewRPCData helper method) internally implicitly +// API changed (takes in context) +// Authenticated matcher is different now + +// ^^^ All part of one PR or split into three? diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index b60e409f4358..54ce6874d552 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -22,6 +22,7 @@ import ( "context" "crypto/x509" "errors" + "google.golang.org/grpc/codes" "net" "strconv" @@ -29,6 +30,7 @@ import ( "google.golang.org/grpc/credentials" "google.golang.org/grpc/metadata" "google.golang.org/grpc/peer" + "google.golang.org/grpc/status" ) // Engine is used for matching incoming RPCs to policies. @@ -72,7 +74,7 @@ func SetConnection(ctx context.Context, conn net.Conn) context.Context { // needed for server RPC Call with headers and connection piped into it) and the // method name of the Service being called server side and populates an RPCData // struct ready to be passed to the RBAC Engine to find a matching policy. -func NewRPCData(ctx context.Context, fullMethod string) (*RPCData, error) { +func NewRPCData(ctx context.Context, fullMethod string) (*RPCData, error) { // *Big question*: Thought I just had: For this function on an error case, should it really return an error in certain situations, as it doesn't really all 6 fields... md, ok := metadata.FromIncomingContext(ctx) if !ok { return nil, errors.New("error retrieving metadata from incoming ctx") @@ -96,7 +98,7 @@ func NewRPCData(ctx context.Context, fullMethod string) (*RPCData, error) { } tlsState := pi.AuthInfo.(credentials.TLSInfo).State // TODO: Handle errors on type conversion? - pName, err := findPrincipalNameFromCerts(tlsState.PeerCertificates) + // pName, err := findPrincipalNameFromCerts(tlsState.PeerCertificates) if err != nil { return nil, err } @@ -107,7 +109,7 @@ func NewRPCData(ctx context.Context, fullMethod string) (*RPCData, error) { FullMethod: fullMethod, DestinationPort: uint32(dp), DestinationAddr: conn.LocalAddr(), - PrincipalName: pName, + Certs: tlsState.PeerCertificates, }, nil } @@ -144,7 +146,7 @@ func findPrincipalNameFromCerts(certs []*x509.Certificate) (string, error) { // RPCData wraps data pulled from an incoming RPC that the RBAC engine needs to // find a matching policy. -type RPCData struct { +type RPCData struct { // <- unexport this struct // MD is the HTTP Headers that are present in the incoming RPC. MD metadata.MD // PeerInfo is information about the downstream peer. @@ -160,13 +162,46 @@ type RPCData struct { // SAN or DNS SAN in that order is used from the certificate, otherwise the // subject field is used. If unset, it applies to any user that is // authenticated. - PrincipalName string + // These certs will be used to find the PrincipalName + Certs []*x509.Certificate } +type RBACData struct { + // This ctx is what is going to be pre populated with things + ctx context.Context + methodName string +} + +var ErrPolicyNotFound = errors.New("a matching policy was not found") + +// FindMatchingPolicy determines if an incoming RPC matches a policy. On a +// successful match, it returns the name of the matching policy and a true +// boolean to specify that there was a matching policy found. It returns +// an error in the case of ctx passed into it not having the correct data +// inside it. +func (r *Engine) FindMatchingPolicy(data RBACData) (string, error) { + // Convert this generic data about an incoming RPC on server side to data that can be passed around RBAC - RPCData + // This needs to return an error... + rpcData, err := NewRPCData(data.ctx, data.methodName) + if err != nil { + // Status error? + return "", status.Error(codes.InvalidArgument, "data could not be converted") + } + // What to do about error handling here? + for policy, matcher := range r.policies { + if matcher.match(rpcData) { + return policy, nil + } + } + return "", ErrPolicyNotFound // I can either make this return a bool (false), or scale up returns to handle error +} + +/* // FindMatchingPolicy determines if an incoming RPC matches a policy. On a // successful match, it returns the name of the matching policy and a true // boolean to specify that there was a matching policy found. -func (r *Engine) FindMatchingPolicy(data *RPCData) (string, bool) { +func (r *Engine) FindMatchingPolicy(data *RPCData) (string, bool) { // <- switch this RPC Data into a context and method name, and convert that to RPC Data + // Convert here. for policy, matcher := range r.policies { if matcher.match(data) { return policy, true @@ -174,3 +209,4 @@ func (r *Engine) FindMatchingPolicy(data *RPCData) (string, bool) { } return "", false } +*/ diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index 47ed5a1342e0..28b116850b34 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -41,6 +41,56 @@ type addr struct { ipAddress string } +// "Getters don't make sense" +// So keep this function here, but the only thing RBAC defines in the API layer is the context, but will call this function + +// Errors combine + +// How does the logic in c core work {1, 2, 3, 4, 5, 6} <- does each field need to be populated with data, does this make sense? +// Right now this function has to have everything +// Could instantiate it with ctx, pull stuff out of context as needed (will return errors there) +// Instantiate with ctx +// Pull stuff out of that generic data as needed...with getters +// Or helperfunciton(ctx) data prepopulated... + +// TestNewRPCData tests the helper function which populates an RPCData struct. OR, have this implicitly tested VVV by taking a context at first +// then passing that into RBAC Engine...how should we switch them around +func (s) TestNewRPCData(t *testing.T) { + tests := []struct { + name string + // Test a context embedded with both metadata and connection (for dest port etc.) + wantErr bool + wantRPCData *RPCData + }{ + // Basic case tests a basic conversion with headers + // Will also test if context makes sense without anything but data + { + name: "basic-case", + wantErr: false, + wantRPCData: &RPCData{ + MD: map[string][]string{ + ":path": {"localhost-fan-page"}, + }, + }, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + /*if _, err := NewEngine(test.rbacConfig); (err != nil) != test.wantErr { + t.Fatalf("NewEngine(%+v) returned err: %v, wantErr: %v", test.rbacConfig, err, test.wantErr) + }*/ + rpcData, error := NewRPCData(ctx, fullMethod) + + }) + } + // Test the error case as well + rpcData, error := NewRPCData() +} + +// It first constructs a context representing the context of an incoming RPC on the server side + +// And then calls into the helepr function and validates if it correctly was built + func (addr) Network() string { return "" } func (a *addr) String() string { return a.ipAddress } From 73d9c40c123b9a6f080fd6ad3eefcc2a4e56a738 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Thu, 1 Jul 2021 13:34:16 -0400 Subject: [PATCH 06/36] Save progress --- internal/xds/rbac/matchers.go | 45 ++++++++------- internal/xds/rbac/rbac_engine.go | 99 +++++++++++++------------------- 2 files changed, 64 insertions(+), 80 deletions(-) diff --git a/internal/xds/rbac/matchers.go b/internal/xds/rbac/matchers.go index c9acc4226810..67037b1219d4 100644 --- a/internal/xds/rbac/matchers.go +++ b/internal/xds/rbac/matchers.go @@ -32,7 +32,7 @@ import ( // matcher is an interface that takes data about incoming RPC's and returns // whether it matches with whatever matcher implements this interface. type matcher interface { - match(data *RPCData) bool + match(data *rpcData) bool } // policyMatcher helps determine whether an incoming RPC call matches a policy. @@ -63,7 +63,7 @@ func newPolicyMatcher(policy *v3rbacpb.Policy) (*policyMatcher, error) { }, nil } -func (pm *policyMatcher) match(data *RPCData) bool { +func (pm *policyMatcher) match(data *rpcData) bool { // A policy matches if and only if at least one of its permissions match the // action taking place AND at least one if its principals match the // downstream peer. @@ -202,7 +202,7 @@ type orMatcher struct { matchers []matcher } -func (om *orMatcher) match(data *RPCData) bool { +func (om *orMatcher) match(data *rpcData) bool { // Range through child matchers and pass in data about incoming RPC, and // only one child matcher has to match to be logically successful. for _, m := range om.matchers { @@ -219,7 +219,7 @@ type andMatcher struct { matchers []matcher } -func (am *andMatcher) match(data *RPCData) bool { +func (am *andMatcher) match(data *rpcData) bool { for _, m := range am.matchers { if !m.match(data) { return false @@ -234,7 +234,7 @@ func (am *andMatcher) match(data *RPCData) bool { type alwaysMatcher struct { } -func (am *alwaysMatcher) match(data *RPCData) bool { +func (am *alwaysMatcher) match(data *rpcData) bool { return true } @@ -244,7 +244,7 @@ type notMatcher struct { matcherToNot matcher } -func (nm *notMatcher) match(data *RPCData) bool { +func (nm *notMatcher) match(data *rpcData) bool { return !nm.matcherToNot.match(data) } @@ -284,8 +284,8 @@ func newHeaderMatcher(headerMatcherConfig *v3route_componentspb.HeaderMatcher) ( return &headerMatcher{matcher: m}, nil } -func (hm *headerMatcher) match(data *RPCData) bool { - return hm.matcher.Match(data.MD) +func (hm *headerMatcher) match(data *rpcData) bool { + return hm.matcher.Match(data.md) } // urlPathMatcher matches on the URL Path of the incoming RPC. In gRPC, this @@ -303,8 +303,8 @@ func newURLPathMatcher(pathMatcher *v3matcherpb.PathMatcher) (*urlPathMatcher, e return &urlPathMatcher{stringMatcher: stringMatcher}, nil } -func (upm *urlPathMatcher) match(data *RPCData) bool { - return upm.stringMatcher.Match(data.FullMethod) +func (upm *urlPathMatcher) match(data *rpcData) bool { + return upm.stringMatcher.Match(data.fullMethod) } // sourceIPMatcher and destinationIPMatcher both are matchers that match against @@ -329,8 +329,8 @@ func newSourceIPMatcher(cidrRange *v3corepb.CidrRange) (*sourceIPMatcher, error) return &sourceIPMatcher{ipNet: ipNet}, nil } -func (sim *sourceIPMatcher) match(data *RPCData) bool { - return sim.ipNet.Contains(net.IP(net.ParseIP(data.PeerInfo.Addr.String()))) +func (sim *sourceIPMatcher) match(data *rpcData) bool { + return sim.ipNet.Contains(net.IP(net.ParseIP(data.peerInfo.Addr.String()))) } type destinationIPMatcher struct { @@ -346,8 +346,8 @@ func newDestinationIPMatcher(cidrRange *v3corepb.CidrRange) (*destinationIPMatch return &destinationIPMatcher{ipNet: ipNet}, nil } -func (dim *destinationIPMatcher) match(data *RPCData) bool { - return dim.ipNet.Contains(net.IP(net.ParseIP(data.DestinationAddr.String()))) +func (dim *destinationIPMatcher) match(data *rpcData) bool { + return dim.ipNet.Contains(net.IP(net.ParseIP(data.destinationAddr.String()))) } // portMatcher matches on whether the destination port of the RPC matches the @@ -361,8 +361,8 @@ func newPortMatcher(destinationPort uint32) *portMatcher { return &portMatcher{destinationPort: destinationPort} } -func (pm *portMatcher) match(data *RPCData) bool { - return data.DestinationPort == pm.destinationPort +func (pm *portMatcher) match(data *rpcData) bool { + return data.destinationPort == pm.destinationPort } // authenticatedMatcher matches on the name of the Principal. If set, the URI @@ -391,20 +391,20 @@ func newAuthenticatedMatcher(authenticatedMatcherConfig *v3rbacpb.Principal_Auth // This should take TLS Certs and then loop through them... // Similar loop to previous: first loop through URI SAN, then loop through DNS SAN, then subject name -func (am *authenticatedMatcher) match(data *RPCData) bool { +func (am *authenticatedMatcher) match(data *rpcData) bool { // Represents this line in the RBAC documentation = "If unset, it applies to // any user that is authenticated". Thus, if a user is authenticated user should // match. An authenticated user will have a certificate provided. if am.stringMatcher == nil { // TODO: Is this correct? If a TLS Certificate is provided, that certificate means // that that individual was logically authenticated with a key. - if len(data.Certs) != 0 { + if len(data.certs) != 0 { return true } } // Loop through URI SAN's (list will be persisted in data) - for _, cert := range data.Certs { + for _, cert := range data.certs { for _, uriSAN := range cert.URIs { if am.stringMatcher.Match(uriSAN.String()) { return true @@ -412,7 +412,7 @@ func (am *authenticatedMatcher) match(data *RPCData) bool { } } // Loop through DNS SAN's (list will be persisted in data) - for _, cert := range data.Certs { + for _, cert := range data.certs { for _, dnsSAN := range cert.DNSNames { if am.stringMatcher.Match(dnsSAN) { return true @@ -421,7 +421,7 @@ func (am *authenticatedMatcher) match(data *RPCData) bool { } // Check against subject names - for _, cert := range data.Certs { + for _, cert := range data.certs { if am.stringMatcher.Match(cert.Subject.String()) { return true } @@ -440,3 +440,6 @@ func (am *authenticatedMatcher) match(data *RPCData) bool { // Authenticated matcher is different now // ^^^ All part of one PR or split into three? + +// Cleanup +// Start testing diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index 54ce6874d552..8cddc5a0f285 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -55,8 +55,6 @@ func NewEngine(policy *v3rbacpb.RBAC) (*Engine, error) { return &Engine{policies: policies}, nil } -// Helper functions here which put connection in and out of the context. -// This is used for authorization interceptors, so do we put it here or in authorization package? type connectionKey struct{} func getConnection(ctx context.Context) net.Conn { @@ -70,11 +68,11 @@ func SetConnection(ctx context.Context, conn net.Conn) context.Context { return context.WithValue(ctx, connectionKey{}, conn) } -// NewRPCData takes a incoming context (should be a context representing state +// newRPCData takes a incoming context (should be a context representing state // needed for server RPC Call with headers and connection piped into it) and the // method name of the Service being called server side and populates an RPCData // struct ready to be passed to the RBAC Engine to find a matching policy. -func NewRPCData(ctx context.Context, fullMethod string) (*RPCData, error) { // *Big question*: Thought I just had: For this function on an error case, should it really return an error in certain situations, as it doesn't really all 6 fields... +func newRPCData(ctx context.Context, fullMethod string) (*rpcData, error) { // *Big question*: Thought I just had: For this function on an error case, should it really return an error in certain situations, as it doesn't really need all 6 fields... md, ok := metadata.FromIncomingContext(ctx) if !ok { return nil, errors.New("error retrieving metadata from incoming ctx") @@ -98,18 +96,17 @@ func NewRPCData(ctx context.Context, fullMethod string) (*RPCData, error) { // * } tlsState := pi.AuthInfo.(credentials.TLSInfo).State // TODO: Handle errors on type conversion? - // pName, err := findPrincipalNameFromCerts(tlsState.PeerCertificates) if err != nil { return nil, err } - return &RPCData{ - MD: md, - PeerInfo: pi, - FullMethod: fullMethod, - DestinationPort: uint32(dp), - DestinationAddr: conn.LocalAddr(), - Certs: tlsState.PeerCertificates, + return &rpcData{ + md: md, + peerInfo: pi, + fullMethod: fullMethod, + destinationPort: uint32(dp), + destinationAddr: conn.LocalAddr(), + certs: tlsState.PeerCertificates, }, nil } @@ -144,69 +141,53 @@ func findPrincipalNameFromCerts(certs []*x509.Certificate) (string, error) { return "", errors.New("the URI SAN, DNS SAN, or subject field was not found in the certificates") } -// RPCData wraps data pulled from an incoming RPC that the RBAC engine needs to +// rpcData wraps data pulled from an incoming RPC that the RBAC engine needs to // find a matching policy. -type RPCData struct { // <- unexport this struct - // MD is the HTTP Headers that are present in the incoming RPC. - MD metadata.MD - // PeerInfo is information about the downstream peer. - PeerInfo *peer.Peer - // FullMethod is the method name being called on the upstream service. - FullMethod string - // DestinationPort is the port that the RPC is being sent to on the +type rpcData struct { + // md is the HTTP Headers that are present in the incoming RPC. + md metadata.MD + // peerInfo is information about the downstream peer. + peerInfo *peer.Peer + // fullMethod is the method name being called on the upstream service. + fullMethod string + // destinationPort is the port that the RPC is being sent to on the // server. - DestinationPort uint32 - // DestinationAddr is the address that the RPC is being sent to. - DestinationAddr net.Addr - // PrincipalName is the name of the downstream principal. If set, the URI - // SAN or DNS SAN in that order is used from the certificate, otherwise the - // subject field is used. If unset, it applies to any user that is - // authenticated. - // These certs will be used to find the PrincipalName - Certs []*x509.Certificate + destinationPort uint32 + // destinationAddr is the address that the RPC is being sent to. + destinationAddr net.Addr + // certs will be used for authenticated matcher. + certs []*x509.Certificate } -type RBACData struct { +// Data represents the generic data about incoming RPC's that must be passed +// into the RBAC Engine in order to try and find a matching policy or not. The +// ctx passed in must have metadata, peerinfo (used for source ip/port and TLS +// information) and connection (used for destination ip/port) embedded within +// it. +type Data struct { // This ctx is what is going to be pre populated with things - ctx context.Context - methodName string + Ctx context.Context + MethodName string } var ErrPolicyNotFound = errors.New("a matching policy was not found") // FindMatchingPolicy determines if an incoming RPC matches a policy. On a -// successful match, it returns the name of the matching policy and a true -// boolean to specify that there was a matching policy found. It returns -// an error in the case of ctx passed into it not having the correct data -// inside it. -func (r *Engine) FindMatchingPolicy(data RBACData) (string, error) { - // Convert this generic data about an incoming RPC on server side to data that can be passed around RBAC - RPCData - // This needs to return an error... - rpcData, err := NewRPCData(data.ctx, data.methodName) +// successful match, it returns the name of the matching policy and a nil error +// to specify that there was a matching policy found. It returns an error in +// the case of ctx passed into function not having the correct data inside it or +// not finding a matching policy. +func (r *Engine) FindMatchingPolicy(data Data) (string, error) { + // Convert passed in generic data into something easily passable around the + // RBAC Engine. + rpcData, err := newRPCData(data.Ctx, data.MethodName) if err != nil { - // Status error? return "", status.Error(codes.InvalidArgument, "data could not be converted") } - // What to do about error handling here? for policy, matcher := range r.policies { if matcher.match(rpcData) { return policy, nil } } - return "", ErrPolicyNotFound // I can either make this return a bool (false), or scale up returns to handle error + return "", ErrPolicyNotFound } - -/* -// FindMatchingPolicy determines if an incoming RPC matches a policy. On a -// successful match, it returns the name of the matching policy and a true -// boolean to specify that there was a matching policy found. -func (r *Engine) FindMatchingPolicy(data *RPCData) (string, bool) { // <- switch this RPC Data into a context and method name, and convert that to RPC Data - // Convert here. - for policy, matcher := range r.policies { - if matcher.match(data) { - return policy, true - } - } - return "", false -} -*/ From 11acf73477e5e298bd37b6d1f04638aefa4140fa Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Thu, 1 Jul 2021 17:49:08 -0400 Subject: [PATCH 07/36] Save progress before cleanup --- internal/xds/rbac/matchers.go | 3 -- internal/xds/rbac/rbac_engine.go | 79 +++++++++++++++++++++++++++++++- 2 files changed, 78 insertions(+), 4 deletions(-) diff --git a/internal/xds/rbac/matchers.go b/internal/xds/rbac/matchers.go index 67037b1219d4..b6661f90cd75 100644 --- a/internal/xds/rbac/matchers.go +++ b/internal/xds/rbac/matchers.go @@ -388,9 +388,6 @@ func newAuthenticatedMatcher(authenticatedMatcherConfig *v3rbacpb.Principal_Auth return &authenticatedMatcher{stringMatcher: &stringMatcher}, nil } -// This should take TLS Certs and then loop through them... - -// Similar loop to previous: first loop through URI SAN, then loop through DNS SAN, then subject name func (am *authenticatedMatcher) match(data *rpcData) bool { // Represents this line in the RBAC documentation = "If unset, it applies to // any user that is authenticated". Thus, if a user is authenticated user should diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index 8cddc5a0f285..ddccac861c78 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -33,9 +33,85 @@ import ( "google.golang.org/grpc/status" ) +// This will be called by an interceptor, so rather than instantiate it every time with a config, instantiate it once with +// a config and continue. + +// ChainedRBACEngine represents a chain of RBAC Engines, which will be used in order to determine the status of incoming RPC's according +// to the actions of each engine. +type ChainedRBACEngine struct { + chainedEngines []*Engine +} + +func NewChainEngine(policies []*v3rbacpb.RBAC) (*ChainedRBACEngine, error) { + // Loop through that list and convert it to RBAC stuff + // Now RBAC will have to persist actions + // Convert from one slice to another slice i.e. policies -> ChainedRBACEngine.chainedEngines + var chainedEngines []*Engine + for _, policy := range policies { + rbacEngine, err := NewEngine(policy) + if err != nil { + return nil, err + } + chainedEngines = append(chainedEngines, rbacEngine) + } + return &ChainedRBACEngine{chainedEngines: chainedEngines}, nil +} + +// DetermineStatus takes in data about incoming RPC's and returns a status +// representing whether the RPC should be denied or not (i.e. PermissionDenied) +// based on the full list of RBAC Engines and their associated actions. +func (cre *ChainedRBACEngine) DetermineStatus(data Data) (codes.Code, error) { // Should I combine these into one <-? + // Convert Data into RPCData + rpcData, err := newRPCData(data.Ctx, data.MethodName) + if err != nil { + // Return should be a status error though. Should I combine codes.Code and error into one thing or no? + } + // Loop through RBAC Engines, logic here + // allow Engine <- RPCData, spits out result handle it + // deny Engine <- RPCData, spits out matching policy name handle it using the Action persisted in the RBAC Engine + for _, engine := range cre.chainedEngines { + // What do I do now with this matchingPolicyName? + matchingPolicyName, err := engine.FindMatchingPolicy(rpcData) // change this layer back + // Two stoppers (return PermissionDenied), matchingPolicy + Deny, and also non matching Policy + allow + + // If the engine type was allow and a matching policy was not found, this RPC should be denied. + if engine.action == allow && err = ErrPolicyNotFound { + return codes.PermissionDenied, nil // This should be combine into one + } + + // If the engine type was deny and also a matching policy was found, this RPC should be denied. + if engine.action == deny && err != ErrPolicyNotFound { + return codes.PermissionDenied, nil // This should be combine into one + } + } + + // Return status code here about whether it's permission denied or not + return codes.OK, nil //<- combine into one? +} + +// Add another layer here in regards to function calls +func chainPolicies(policies []*v3rbacpb.RBAC, data Data) /*Status code such as permission denied*/ { + // Handle the logic here about instantiating RBAC Engines in a list + + // Convert Data into RPCData (only has to happen once), then pass that to the list of engines + // allow <- RPCData, spits out result handle it + // deny <- RPCData, spits out result handle it + + // Return status code here about whether it's permission denied or not +} + +type action int + +const ( + allow action = iota + deny +) + // Engine is used for matching incoming RPCs to policies. type Engine struct { policies map[string]*policyMatcher + // Persist something here that represents action, don't return it, have caller handle the logic used to call this Engine + action action } // NewEngine creates an RBAC Engine based on the contents of policy. If the @@ -44,6 +120,7 @@ type Engine struct { // present in the policy, and will leave up to caller to handle the action that // is attached to the config. func NewEngine(policy *v3rbacpb.RBAC) (*Engine, error) { + // Persist action - any verification needed on action i.e. what to do on action.LOG? policies := make(map[string]*policyMatcher) for name, config := range policy.Policies { matcher, err := newPolicyMatcher(config) @@ -177,7 +254,7 @@ var ErrPolicyNotFound = errors.New("a matching policy was not found") // to specify that there was a matching policy found. It returns an error in // the case of ctx passed into function not having the correct data inside it or // not finding a matching policy. -func (r *Engine) FindMatchingPolicy(data Data) (string, error) { +func (r *Engine) FindMatchingPolicy(data Data) (string, error) { // Convert this back to old definition // Convert passed in generic data into something easily passable around the // RBAC Engine. rpcData, err := newRPCData(data.Ctx, data.MethodName) From 153fbb66eccfa95f7b00fa2e86f42864f71d2411 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Fri, 2 Jul 2021 09:31:25 -0400 Subject: [PATCH 08/36] Cleanup --- internal/xds/rbac/rbac_engine.go | 133 +++++++++++-------------------- 1 file changed, 47 insertions(+), 86 deletions(-) diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index ddccac861c78..004db4155939 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -22,30 +22,28 @@ import ( "context" "crypto/x509" "errors" - "google.golang.org/grpc/codes" "net" "strconv" v3rbacpb "github.com/envoyproxy/go-control-plane/envoy/config/rbac/v3" + "google.golang.org/grpc/codes" "google.golang.org/grpc/credentials" "google.golang.org/grpc/metadata" "google.golang.org/grpc/peer" "google.golang.org/grpc/status" ) -// This will be called by an interceptor, so rather than instantiate it every time with a config, instantiate it once with -// a config and continue. - -// ChainedRBACEngine represents a chain of RBAC Engines, which will be used in order to determine the status of incoming RPC's according -// to the actions of each engine. +// ChainedRBACEngine represents a chain of RBAC Engines, which will be used in +// order to determine whether to allow incoming RPC's to proceed according to +// the actions of each engine. type ChainedRBACEngine struct { chainedEngines []*Engine } +// NewChainEngine returns a new Chain of RBAC Engines to query if incoming RPC's +// are allowed to proceed or not. This function returns an error in the case +// that one of the policies is invalid. func NewChainEngine(policies []*v3rbacpb.RBAC) (*ChainedRBACEngine, error) { - // Loop through that list and convert it to RBAC stuff - // Now RBAC will have to persist actions - // Convert from one slice to another slice i.e. policies -> ChainedRBACEngine.chainedEngines var chainedEngines []*Engine for _, policy := range policies { rbacEngine, err := NewEngine(policy) @@ -57,47 +55,36 @@ func NewChainEngine(policies []*v3rbacpb.RBAC) (*ChainedRBACEngine, error) { return &ChainedRBACEngine{chainedEngines: chainedEngines}, nil } -// DetermineStatus takes in data about incoming RPC's and returns a status -// representing whether the RPC should be denied or not (i.e. PermissionDenied) +// DetermineStatus takes in data about incoming RPC's and returns a status error +// representing whether the RPC should be denied or not (i.e. PermissionDenied or OK) // based on the full list of RBAC Engines and their associated actions. -func (cre *ChainedRBACEngine) DetermineStatus(data Data) (codes.Code, error) { // Should I combine these into one <-? - // Convert Data into RPCData +func (cre *ChainedRBACEngine) DetermineStatus(data Data) error { // Should I combine these into one <-? + // This conversion step (i.e. pulling things out of generic ctx) can be done + // once, and then be used for the whole chain of RBAC Engines. rpcData, err := newRPCData(data.Ctx, data.MethodName) if err != nil { - // Return should be a status error though. Should I combine codes.Code and error into one thing or no? + return status.Error(codes.InvalidArgument, "passed in data did not have enough information representing incoming rpc") } - // Loop through RBAC Engines, logic here - // allow Engine <- RPCData, spits out result handle it - // deny Engine <- RPCData, spits out matching policy name handle it using the Action persisted in the RBAC Engine for _, engine := range cre.chainedEngines { - // What do I do now with this matchingPolicyName? - matchingPolicyName, err := engine.FindMatchingPolicy(rpcData) // change this layer back - // Two stoppers (return PermissionDenied), matchingPolicy + Deny, and also non matching Policy + allow + // TODO: What do I do now with this matchingPolicyName? + _, err := engine.findMatchingPolicy(rpcData) - // If the engine type was allow and a matching policy was not found, this RPC should be denied. - if engine.action == allow && err = ErrPolicyNotFound { - return codes.PermissionDenied, nil // This should be combine into one + // If the engine action type was allow and a matching policy was not + // found, this RPC should be denied. + if engine.action == allow && err == ErrPolicyNotFound { + return status.Error(codes.PermissionDenied, "incoming RPC did not match an allow policy") } - // If the engine type was deny and also a matching policy was found, this RPC should be denied. + // If the engine type was deny and also a matching policy was found, + // this RPC should be denied. if engine.action == deny && err != ErrPolicyNotFound { - return codes.PermissionDenied, nil // This should be combine into one + return status.Error(codes.PermissionDenied, "incoming RPC matched a deny policy") } } - - // Return status code here about whether it's permission denied or not - return codes.OK, nil //<- combine into one? -} - -// Add another layer here in regards to function calls -func chainPolicies(policies []*v3rbacpb.RBAC, data Data) /*Status code such as permission denied*/ { - // Handle the logic here about instantiating RBAC Engines in a list - - // Convert Data into RPCData (only has to happen once), then pass that to the list of engines - // allow <- RPCData, spits out result handle it - // deny <- RPCData, spits out result handle it - - // Return status code here about whether it's permission denied or not + // If the incoming RPC gets through all of the engines successfully (i.e. + // doesn't not match an allow or match a deny engine), the RPC is authorized + // to proceed. + return status.Error(codes.OK, "rpc is ok to proceed") } type action int @@ -120,7 +107,16 @@ type Engine struct { // present in the policy, and will leave up to caller to handle the action that // is attached to the config. func NewEngine(policy *v3rbacpb.RBAC) (*Engine, error) { - // Persist action - any verification needed on action i.e. what to do on action.LOG? + var action action + switch *policy.Action.Enum() { + case v3rbacpb.RBAC_ALLOW: + action = allow + case v3rbacpb.RBAC_DENY: + action = deny + case v3rbacpb.RBAC_LOG: + return nil, errors.New("unsupported action") + } + policies := make(map[string]*policyMatcher) for name, config := range policy.Policies { matcher, err := newPolicyMatcher(config) @@ -129,7 +125,10 @@ func NewEngine(policy *v3rbacpb.RBAC) (*Engine, error) { } policies[name] = matcher } - return &Engine{policies: policies}, nil + return &Engine{ + policies: policies, + action: action, + }, nil } type connectionKey struct{} @@ -172,9 +171,9 @@ func newRPCData(ctx context.Context, fullMethod string) (*rpcData, error) { // * return nil, err } - tlsState := pi.AuthInfo.(credentials.TLSInfo).State // TODO: Handle errors on type conversion? - if err != nil { - return nil, err + tlsInfo, ok := pi.AuthInfo.(credentials.TLSInfo) + if !ok { + return nil, errors.New("wrong credentials provided, need to be tls") } return &rpcData{ @@ -183,41 +182,10 @@ func newRPCData(ctx context.Context, fullMethod string) (*rpcData, error) { // * fullMethod: fullMethod, destinationPort: uint32(dp), destinationAddr: conn.LocalAddr(), - certs: tlsState.PeerCertificates, + certs: tlsInfo.State.PeerCertificates, }, nil } -// findPrincipalNameFromCerts extracts the principal name from the TLS -// Certificates according to the RBAC configuration logic - "The URI SAN or DNS -// SAN in that order is used from the certificate, otherwise the subject field -// is used." -func findPrincipalNameFromCerts(certs []*x509.Certificate) (string, error) { - if len(certs) == 0 { - return "", errors.New("there are no certificates present") - } - // Loop through and find URI SAN if present. - for _, cert := range certs { - for _, uriSAN := range cert.URIs { - return uriSAN.String(), nil - } - } - - // Loop through and find DNS SAN if present. - for _, cert := range certs { - for _, dnsSAN := range cert.DNSNames { - return dnsSAN, nil - } - } - - // If neither URI SAN or DNS SAN were present in the certificates, we can - // just return the subject field. - for _, cert := range certs { - return cert.Subject.String(), nil - } - - return "", errors.New("the URI SAN, DNS SAN, or subject field was not found in the certificates") -} - // rpcData wraps data pulled from an incoming RPC that the RBAC engine needs to // find a matching policy. type rpcData struct { @@ -249,18 +217,11 @@ type Data struct { var ErrPolicyNotFound = errors.New("a matching policy was not found") -// FindMatchingPolicy determines if an incoming RPC matches a policy. On a +// findMatchingPolicy determines if an incoming RPC matches a policy. On a // successful match, it returns the name of the matching policy and a nil error // to specify that there was a matching policy found. It returns an error in -// the case of ctx passed into function not having the correct data inside it or -// not finding a matching policy. -func (r *Engine) FindMatchingPolicy(data Data) (string, error) { // Convert this back to old definition - // Convert passed in generic data into something easily passable around the - // RBAC Engine. - rpcData, err := newRPCData(data.Ctx, data.MethodName) - if err != nil { - return "", status.Error(codes.InvalidArgument, "data could not be converted") - } +// the case of not finding a matching policy. +func (r *Engine) findMatchingPolicy(rpcData *rpcData) (string, error) { for policy, matcher := range r.policies { if matcher.match(rpcData) { return policy, nil From 16002a5151ba7bffbe0fe3864ae2e58a2a35e99b Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Fri, 2 Jul 2021 09:45:17 -0400 Subject: [PATCH 09/36] More cleanup --- internal/xds/rbac/matchers.go | 24 +++------------ internal/xds/rbac/rbac_engine.go | 52 +++++++++++++++----------------- 2 files changed, 29 insertions(+), 47 deletions(-) diff --git a/internal/xds/rbac/matchers.go b/internal/xds/rbac/matchers.go index b6661f90cd75..0ea589d6d4c3 100644 --- a/internal/xds/rbac/matchers.go +++ b/internal/xds/rbac/matchers.go @@ -373,14 +373,12 @@ type authenticatedMatcher struct { stringMatcher *internalmatcher.StringMatcher } -// Two things of logic here: config can be null, in which case you don't persist a string matcher and it matches any principal name func newAuthenticatedMatcher(authenticatedMatcherConfig *v3rbacpb.Principal_Authenticated) (*authenticatedMatcher, error) { // Represents this line in the RBAC documentation = "If unset, it applies to // any user that is authenticated". if authenticatedMatcherConfig.PrincipalName == nil { return &authenticatedMatcher{}, nil } - // This will be split to branching logic, how is that represented in GoLang? stringMatcher, err := internalmatcher.StringMatcherFromProto(authenticatedMatcherConfig.PrincipalName) if err != nil { return nil, err @@ -400,7 +398,8 @@ func (am *authenticatedMatcher) match(data *rpcData) bool { } } - // Loop through URI SAN's (list will be persisted in data) + // The precedence according to documentation specifies to use URI SANs at + // first if present. for _, cert := range data.certs { for _, uriSAN := range cert.URIs { if am.stringMatcher.Match(uriSAN.String()) { @@ -408,7 +407,7 @@ func (am *authenticatedMatcher) match(data *rpcData) bool { } } } - // Loop through DNS SAN's (list will be persisted in data) + // The next thing to check against if present DNS SANs, if present. for _, cert := range data.certs { for _, dnsSAN := range cert.DNSNames { if am.stringMatcher.Match(dnsSAN) { @@ -417,7 +416,7 @@ func (am *authenticatedMatcher) match(data *rpcData) bool { } } - // Check against subject names + // The last thing to check is the subject field. for _, cert := range data.certs { if am.stringMatcher.Match(cert.Subject.String()) { return true @@ -425,18 +424,3 @@ func (am *authenticatedMatcher) match(data *rpcData) bool { } return false } - -// Tasks -// Authenticated matcher has wrong logic (Done I think nope) -// Change API Layer, which will be a two liner (Done I think... nope) - -// Testing Tasks, will depend on ^^^ -// Unit test helper function to RPC Data (This should be first test written, as might change API Layer of what you need to pass into it which will break VVV) -// This ^^^ unit test will be implicitly tested though if we change API Layer, it'll hit that codepath (NewRPCData helper method) internally implicitly -// API changed (takes in context) -// Authenticated matcher is different now - -// ^^^ All part of one PR or split into three? - -// Cleanup -// Start testing diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index 004db4155939..fe723684827d 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -37,14 +37,14 @@ import ( // order to determine whether to allow incoming RPC's to proceed according to // the actions of each engine. type ChainedRBACEngine struct { - chainedEngines []*Engine + chainedEngines []*engine } // NewChainEngine returns a new Chain of RBAC Engines to query if incoming RPC's // are allowed to proceed or not. This function returns an error in the case // that one of the policies is invalid. func NewChainEngine(policies []*v3rbacpb.RBAC) (*ChainedRBACEngine, error) { - var chainedEngines []*Engine + var chainedEngines []*engine for _, policy := range policies { rbacEngine, err := NewEngine(policy) if err != nil { @@ -58,10 +58,10 @@ func NewChainEngine(policies []*v3rbacpb.RBAC) (*ChainedRBACEngine, error) { // DetermineStatus takes in data about incoming RPC's and returns a status error // representing whether the RPC should be denied or not (i.e. PermissionDenied or OK) // based on the full list of RBAC Engines and their associated actions. -func (cre *ChainedRBACEngine) DetermineStatus(data Data) error { // Should I combine these into one <-? +func (cre *ChainedRBACEngine) DetermineStatus(data Data) error { // This conversion step (i.e. pulling things out of generic ctx) can be done // once, and then be used for the whole chain of RBAC Engines. - rpcData, err := newRPCData(data.Ctx, data.MethodName) + rpcData, err := newRPCData(data) if err != nil { return status.Error(codes.InvalidArgument, "passed in data did not have enough information representing incoming rpc") } @@ -94,11 +94,10 @@ const ( deny ) -// Engine is used for matching incoming RPCs to policies. -type Engine struct { +// engine is used for matching incoming RPCs to policies. +type engine struct { policies map[string]*policyMatcher - // Persist something here that represents action, don't return it, have caller handle the logic used to call this Engine - action action + action action } // NewEngine creates an RBAC Engine based on the contents of policy. If the @@ -106,7 +105,7 @@ type Engine struct { // will return an error. This created RBAC Engine will not persist the action // present in the policy, and will leave up to caller to handle the action that // is attached to the config. -func NewEngine(policy *v3rbacpb.RBAC) (*Engine, error) { +func NewEngine(policy *v3rbacpb.RBAC) (*engine, error) { var action action switch *policy.Action.Enum() { case v3rbacpb.RBAC_ALLOW: @@ -125,7 +124,7 @@ func NewEngine(policy *v3rbacpb.RBAC) (*Engine, error) { } policies[name] = matcher } - return &Engine{ + return &engine{ policies: policies, action: action, }, nil @@ -144,24 +143,34 @@ func SetConnection(ctx context.Context, conn net.Conn) context.Context { return context.WithValue(ctx, connectionKey{}, conn) } +// Data represents the generic data about incoming RPC's that must be passed +// into the RBAC Engine in order to try and find a matching policy or not. The +// ctx passed in must have metadata, peerinfo (used for source ip/port and TLS +// information) and connection (used for destination ip/port) embedded within +// it. +type Data struct { + Ctx context.Context + MethodName string +} + // newRPCData takes a incoming context (should be a context representing state // needed for server RPC Call with headers and connection piped into it) and the // method name of the Service being called server side and populates an RPCData // struct ready to be passed to the RBAC Engine to find a matching policy. -func newRPCData(ctx context.Context, fullMethod string) (*rpcData, error) { // *Big question*: Thought I just had: For this function on an error case, should it really return an error in certain situations, as it doesn't really need all 6 fields... - md, ok := metadata.FromIncomingContext(ctx) +func newRPCData(data Data) (*rpcData, error) { // *Big question*: Thought I just had: For this function on an error case, should it really return an error in certain situations, as it doesn't really need all 6 fields... + md, ok := metadata.FromIncomingContext(data.Ctx) if !ok { return nil, errors.New("error retrieving metadata from incoming ctx") } - pi, ok := peer.FromContext(ctx) + pi, ok := peer.FromContext(data.Ctx) if !ok { return nil, errors.New("error retrieving peer info from incoming ctx") } // You need the connection in order to find the destination address and port // of the incoming RPC Call. - conn := getConnection(ctx) + conn := getConnection(data.Ctx) _, dPort, err := net.SplitHostPort(conn.LocalAddr().String()) if err != nil { return nil, err @@ -179,7 +188,7 @@ func newRPCData(ctx context.Context, fullMethod string) (*rpcData, error) { // * return &rpcData{ md: md, peerInfo: pi, - fullMethod: fullMethod, + fullMethod: data.MethodName, destinationPort: uint32(dp), destinationAddr: conn.LocalAddr(), certs: tlsInfo.State.PeerCertificates, @@ -204,24 +213,13 @@ type rpcData struct { certs []*x509.Certificate } -// Data represents the generic data about incoming RPC's that must be passed -// into the RBAC Engine in order to try and find a matching policy or not. The -// ctx passed in must have metadata, peerinfo (used for source ip/port and TLS -// information) and connection (used for destination ip/port) embedded within -// it. -type Data struct { - // This ctx is what is going to be pre populated with things - Ctx context.Context - MethodName string -} - var ErrPolicyNotFound = errors.New("a matching policy was not found") // findMatchingPolicy determines if an incoming RPC matches a policy. On a // successful match, it returns the name of the matching policy and a nil error // to specify that there was a matching policy found. It returns an error in // the case of not finding a matching policy. -func (r *Engine) findMatchingPolicy(rpcData *rpcData) (string, error) { +func (r *engine) findMatchingPolicy(rpcData *rpcData) (string, error) { for policy, matcher := range r.policies { if matcher.match(rpcData) { return policy, nil From 72311a2761f97001a5d645fa6d284eb00ccd457a Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Fri, 2 Jul 2021 14:06:40 -0400 Subject: [PATCH 10/36] Save progress --- internal/xds/rbac/rbac_engine_test.go | 368 ++++++++++++++++++++++++++ 1 file changed, 368 insertions(+) diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index 28b116850b34..99d80288641c 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -654,3 +654,371 @@ func (s) TestRBACEngine(t *testing.T) { }) } } + +// Function here for configuration (chaining) +// Take singular configurations from previous +func (s) TestChainedRBACEngineConstruction(t *testing.T) { + tests := []struct { + name string + rbacConfigs []*v3rbacpb.RBAC + wantErr bool + }{ + { + name: "TestSuccessCaseAnyMatchSingular", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Action: v3rbacpb.RBAC_ALLOW, + Policies: map[string]*v3rbacpb.Policy{ + "anyone": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Any{Any: true}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Any{Any: true}}, + }, + }, + }, + }, + }, + }, + { + name: "TestSuccessCaseAnyMatchMultiple", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Action: v3rbacpb.RBAC_ALLOW, + Policies: map[string]*v3rbacpb.Policy{ + "anyone": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Any{Any: true}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Any{Any: true}}, + }, + }, + }, + }, + { + Action: v3rbacpb.RBAC_DENY, + Policies: map[string]*v3rbacpb.Policy{ + "anyone": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Any{Any: true}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Any{Any: true}}, + }, + }, + }, + }, + }, + }, + { + name: "TestSuccessCaseSimplePolicySingular", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Action: v3rbacpb.RBAC_ALLOW, + Policies: map[string]*v3rbacpb.Policy{ + "localhost-fan": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 8080}}, + {Rule: &v3rbacpb.Permission_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "localhost-fan-page"}}}}}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Any{Any: true}}, + }, + }, + }, + }, + }, + }, + { + name: "TestSuccessCaseSimplePolicyMultiple", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Action: v3rbacpb.RBAC_ALLOW, + Policies: map[string]*v3rbacpb.Policy{ + "localhost-fan": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 8080}}, + {Rule: &v3rbacpb.Permission_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "localhost-fan-page"}}}}}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Any{Any: true}}, + }, + }, + }, + }, + { + Action: v3rbacpb.RBAC_DENY, + Policies: map[string]*v3rbacpb.Policy{ + "localhost-fan": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 8080}}, + {Rule: &v3rbacpb.Permission_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "localhost-fan-page"}}}}}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Any{Any: true}}, + }, + }, + }, + }, + }, + }, + { + name: "TestSuccessCaseEnvoyExampleSingular", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Action: v3rbacpb.RBAC_ALLOW, + Policies: map[string]*v3rbacpb.Policy{ + "service-admin": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Any{Any: true}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Authenticated_{Authenticated: &v3rbacpb.Principal_Authenticated{PrincipalName: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "cluster.local/ns/default/sa/admin"}}}}}, + {Identifier: &v3rbacpb.Principal_Authenticated_{Authenticated: &v3rbacpb.Principal_Authenticated{PrincipalName: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "cluster.local/ns/default/sa/superuser"}}}}}, + }, + }, + "product-viewer": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_AndRules{AndRules: &v3rbacpb.Permission_Set{ + Rules: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_ExactMatch{ExactMatch: "GET"}}}}, + {Rule: &v3rbacpb.Permission_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Prefix{Prefix: "/products"}}}}}}, + {Rule: &v3rbacpb.Permission_OrRules{OrRules: &v3rbacpb.Permission_Set{ + Rules: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 80}}, + {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 443}}, + }, + }}}, + }, + }, + }, + }, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Any{Any: true}}, + }, + }, + }, + }, + }, + }, + { + name: "TestSourceIpMatcherSuccessSingular", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Action: v3rbacpb.RBAC_ALLOW, + Policies: map[string]*v3rbacpb.Policy{ + "certain-source-ip": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Any{Any: true}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_DirectRemoteIp{DirectRemoteIp: &v3corepb.CidrRange{AddressPrefix: "0.0.0.0", PrefixLen: &wrapperspb.UInt32Value{Value: uint32(10)}}}}, + }, + }, + }, + }, + }, + }, + { + name: "TestSourceIpMatcherFailureSingular", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Action: v3rbacpb.RBAC_ALLOW, + Policies: map[string]*v3rbacpb.Policy{ + "certain-source-ip": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Any{Any: true}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_DirectRemoteIp{DirectRemoteIp: &v3corepb.CidrRange{AddressPrefix: "not a correct address", PrefixLen: &wrapperspb.UInt32Value{Value: uint32(10)}}}}, + }, + }, + }, + }, + }, + wantErr: true, + }, + { + name: "TestDestinationIpMatcherSuccess", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Action: v3rbacpb.RBAC_ALLOW, + Policies: map[string]*v3rbacpb.Policy{ + "certain-destination-ip": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_DestinationIp{DestinationIp: &v3corepb.CidrRange{AddressPrefix: "0.0.0.0", PrefixLen: &wrapperspb.UInt32Value{Value: uint32(10)}}}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Any{Any: true}}, + }, + }, + }, + }, + }, + }, + { + name: "TestDestinationIpMatcherFailure", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Action: v3rbacpb.RBAC_ALLOW, + Policies: map[string]*v3rbacpb.Policy{ + "certain-destination-ip": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_DestinationIp{DestinationIp: &v3corepb.CidrRange{AddressPrefix: "not a correct address", PrefixLen: &wrapperspb.UInt32Value{Value: uint32(10)}}}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Any{Any: true}}, + }, + }, + }, + }, + }, + }, + { + name: "TestMatcherToNotPolicy", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Action: v3rbacpb.RBAC_ALLOW, + Policies: map[string]*v3rbacpb.Policy{ + "not-secret-content": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_NotRule{NotRule: &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Prefix{Prefix: "/secret-content"}}}}}}}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Any{Any: true}}, + }, + }, + }, + }, + }, + }, + { + name: "TestMatcherToNotPrinicipal", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Action: v3rbacpb.RBAC_ALLOW, + Policies: map[string]*v3rbacpb.Policy{ + "not-from-certain-ip": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Any{Any: true}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_NotId{NotId: &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_DirectRemoteIp{DirectRemoteIp: &v3corepb.CidrRange{AddressPrefix: "0.0.0.0", PrefixLen: &wrapperspb.UInt32Value{Value: uint32(10)}}}}}}, + }, + }, + }, + }, + }, + }, + { + name: "TestPrincipalProductViewer", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Action: v3rbacpb.RBAC_ALLOW, + Policies: map[string]*v3rbacpb.Policy{ + "product-viewer": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Any{Any: true}}, + }, + Principals: []*v3rbacpb.Principal{ + { + Identifier: &v3rbacpb.Principal_AndIds{AndIds: &v3rbacpb.Principal_Set{Ids: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_ExactMatch{ExactMatch: "GET"}}}}, + {Identifier: &v3rbacpb.Principal_OrIds{OrIds: &v3rbacpb.Principal_Set{ + Ids: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Prefix{Prefix: "/books"}}}}}}, + {Identifier: &v3rbacpb.Principal_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Prefix{Prefix: "/cars"}}}}}}, + }, + }}}, + }}}, + }, + }, + }, + }, + }, + }, + }, + { + name: "TestCertainHeaders", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Policies: map[string]*v3rbacpb.Policy{ + "certain-headers": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Any{Any: true}}, + }, + Principals: []*v3rbacpb.Principal{ + { + Identifier: &v3rbacpb.Principal_OrIds{OrIds: &v3rbacpb.Principal_Set{Ids: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_ExactMatch{ExactMatch: "GET"}}}}, + {Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_SafeRegexMatch{SafeRegexMatch: &v3matcherpb.RegexMatcher{Regex: "GET"}}}}}, + {Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_RangeMatch{RangeMatch: &v3typepb.Int64Range{ + Start: 0, + End: 64, + }}}}}, + {Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PresentMatch{PresentMatch: true}}}}, + {Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PrefixMatch{PrefixMatch: "GET"}}}}, + {Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_SuffixMatch{SuffixMatch: "GET"}}}}, + {Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_ContainsMatch{ContainsMatch: "GET"}}}}, + }}}, + }, + }, + }, + }, + }, + }, + }, + // Test LOG and also an action not specified + { + name: "TestLogAction", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Action: v3rbacpb.RBAC_LOG, + Policies: map[string]*v3rbacpb.Policy{ + "anyone": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Any{Any: true}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Any{Any: true}}, + }, + }, + }, + }, + }, + wantErr: true, + }, + { + name: "TestActionNotSpecified", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Policies: map[string]*v3rbacpb.Policy{ + "anyone": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Any{Any: true}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Any{Any: true}}, + }, + }, + }, + }, + }, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if _, err := NewChainEngine(test.rbacConfigs); (err != nil) != test.wantErr { + t.Fatalf("NewChainEngine(%+v) returned err: %v, wantErr: %v", test.rbacConfigs, err, test.wantErr) + } + }) + } +} + +// Function here for once configured and instantiated, check incoming RPC's against it +// Will check conversion function + chaining logic of whether incoming RPC's should be allowed + +// After writing all these tests, also need to check code coverage From d60b45b5de7522ac250bb28a4be73eee09903368 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Fri, 2 Jul 2021 14:12:49 -0400 Subject: [PATCH 11/36] Working instantiation test --- internal/xds/rbac/rbac_engine_test.go | 45 +++------------------------ 1 file changed, 5 insertions(+), 40 deletions(-) diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index 99d80288641c..f3f927f8bd7a 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -26,7 +26,6 @@ import ( v3typepb "github.com/envoyproxy/go-control-plane/envoy/type/v3" wrapperspb "github.com/golang/protobuf/ptypes/wrappers" "google.golang.org/grpc/internal/grpctest" - "google.golang.org/grpc/peer" ) type s struct { @@ -53,44 +52,7 @@ type addr struct { // Pull stuff out of that generic data as needed...with getters // Or helperfunciton(ctx) data prepopulated... -// TestNewRPCData tests the helper function which populates an RPCData struct. OR, have this implicitly tested VVV by taking a context at first -// then passing that into RBAC Engine...how should we switch them around -func (s) TestNewRPCData(t *testing.T) { - tests := []struct { - name string - // Test a context embedded with both metadata and connection (for dest port etc.) - wantErr bool - wantRPCData *RPCData - }{ - // Basic case tests a basic conversion with headers - // Will also test if context makes sense without anything but data - { - name: "basic-case", - wantErr: false, - wantRPCData: &RPCData{ - MD: map[string][]string{ - ":path": {"localhost-fan-page"}, - }, - }, - }, - } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - /*if _, err := NewEngine(test.rbacConfig); (err != nil) != test.wantErr { - t.Fatalf("NewEngine(%+v) returned err: %v, wantErr: %v", test.rbacConfig, err, test.wantErr) - }*/ - rpcData, error := NewRPCData(ctx, fullMethod) - - }) - } - // Test the error case as well - rpcData, error := NewRPCData() -} - -// It first constructs a context representing the context of an incoming RPC on the server side - -// And then calls into the helepr function and validates if it correctly was built - +/* func (addr) Network() string { return "" } func (a *addr) String() string { return a.ipAddress } @@ -653,7 +615,7 @@ func (s) TestRBACEngine(t *testing.T) { } }) } -} +}*/ // Function here for configuration (chaining) // Take singular configurations from previous @@ -876,6 +838,7 @@ func (s) TestChainedRBACEngineConstruction(t *testing.T) { }, }, }, + wantErr: true, }, { name: "TestMatcherToNotPolicy", @@ -1021,4 +984,6 @@ func (s) TestChainedRBACEngineConstruction(t *testing.T) { // Function here for once configured and instantiated, check incoming RPC's against it // Will check conversion function + chaining logic of whether incoming RPC's should be allowed +// Thus, this needs to construct a context at the beginning full of the things that represent data + // After writing all these tests, also need to check code coverage From cbd8112be5b0b7a82e152d4b7f1f7f63150ce2a8 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Tue, 6 Jul 2021 08:30:21 -0400 Subject: [PATCH 12/36] Save progress --- internal/xds/rbac/rbac_engine.go | 6 +- internal/xds/rbac/rbac_engine_test.go | 138 ++++++++++++++++++++++++++ 2 files changed, 142 insertions(+), 2 deletions(-) diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index fe723684827d..04aef107642d 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -158,6 +158,7 @@ type Data struct { // method name of the Service being called server side and populates an RPCData // struct ready to be passed to the RBAC Engine to find a matching policy. func newRPCData(data Data) (*rpcData, error) { // *Big question*: Thought I just had: For this function on an error case, should it really return an error in certain situations, as it doesn't really need all 6 fields... + // Will we leave it up to caller to pipe these three pieces of data into context? Right now, as expected, it's failing. md, ok := metadata.FromIncomingContext(data.Ctx) if !ok { return nil, errors.New("error retrieving metadata from incoming ctx") @@ -180,12 +181,13 @@ func newRPCData(data Data) (*rpcData, error) { // *Big question*: Thought I just return nil, err } - tlsInfo, ok := pi.AuthInfo.(credentials.TLSInfo) + tlsInfo, ok := pi.AuthInfo.(credentials.TLSInfo) // Does this have to be TLS? if !ok { return nil, errors.New("wrong credentials provided, need to be tls") } - return &rpcData{ + // I think we should require all 3 except TLS Info might be questionable + return &rpcData{ // Big question: What to do when data is not specified...? Should we require all 3? md: md, peerInfo: pi, fullMethod: data.MethodName, diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index f3f927f8bd7a..f377846befd3 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -17,6 +17,12 @@ package rbac import ( + "context" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/metadata" + "google.golang.org/grpc/peer" + "google.golang.org/grpc/status" + "net" "testing" v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" @@ -40,6 +46,8 @@ type addr struct { ipAddress string } +// Actually I might keep these... + // "Getters don't make sense" // So keep this function here, but the only thing RBAC defines in the API layer is the context, but will call this function @@ -619,6 +627,12 @@ func (s) TestRBACEngine(t *testing.T) { // Function here for configuration (chaining) // Take singular configurations from previous + +// TestChainedRBACEngineConstruction tests the construction of the +// ChainedRBACEngine. Due to some types of RBAC configuration being logically +// wrong and returning an error rather than successfully constructing the RBAC +// Engine, this test tests both RBAC Configurations deemed successful and also +// RBAC Configurations that will raise errors. func (s) TestChainedRBACEngineConstruction(t *testing.T) { tests := []struct { name string @@ -986,4 +1000,128 @@ func (s) TestChainedRBACEngineConstruction(t *testing.T) { // Thus, this needs to construct a context at the beginning full of the things that represent data +// TestChainedRBACEngine tests the chain of RBAC Engines by configuring the +// chain of engines in a certain way in different scenarios. After configuring +// the chain of engines in a certain way, this test pings the chain of engines +// with different types of data representing incoming RPC's (usually data piped +// into a context), and verifies that it works as expected. +func (s) TestChainedRBACEngine(t *testing.T) { + tests := []struct { + name string + rbacConfigs []*v3rbacpb.RBAC // Scaled up to handle long list + rbacQueries []struct { + /*Previously: + (What you send into API, What you get back from API) + rpcData *RPCData + wantMatchingPolicyName string + wantMatch bool + */ + // DetermineStatus(data Data) error <- will have information about what to do about the RPC + /*Now: You send in generic data to API, you get a status code without matching policy name there*/ + // Generic Data in context (Metadata, PeerInfo, Conn) that will get pulled out + // Why not literally have the four data points: Metadata, PeerInfo, and Connection and MethodName + // Pipe first three into context, call the function with fourth as well + md metadata.MD + pi *peer.Peer + conn net.Conn + methodName string + wantStatusCode codes.Code + } + }{ + // TestSuccessCaseAnyMatch tests a single RBAC Engine instantiated with + // a config with a policy with any rules for both permissions and + // principals, meaning that any data about incoming RPC's that the RBAC + // Engine is queried with should match that policy. + { + name: "TestSuccessCaseAnyMatch", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Policies: map[string]*v3rbacpb.Policy{ + "anyone": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Any{Any: true}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Any{Any: true}}, + }, + }, + }, + }, + }, + rbacQueries: []struct { + md metadata.MD + pi *peer.Peer + conn net.Conn + methodName string + wantStatusCode codes.Code + }{ + { + // If you unspecify the three, this will logically represent the same thing as previous. What will happen in this scenario? + methodName: "some method", + wantStatusCode: codes.OK, + }, + }, + }, + // TestSuccessCaseSimplePolicy is a test that tests a single policy + // that only allows an rpc to proceed if the rpc is calling with a certain + // path and port. + /*{ + name: "TestSuccessCaseSimplePolicy", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Policies: map[string]*v3rbacpb.Policy{ + "localhost-fan": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 8080}}, + {Rule: &v3rbacpb.Permission_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "localhost-fan-page"}}}}}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Any{Any: true}}, + }, + }, + }, + }, + }, + rbacQueries: []struct { + // Why + }, + },*/ + } + + // Also check what happens if stuff isn't already in context (i.e. metadata, peerinfo, conn) + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + // Instantiate the chainedRBACEngine with different configurations that are + // interesting to test and to query. + cre, err := NewChainEngine(test.rbacConfigs) + if err != nil { + t.Fatalf("Error constructing RBAC Engine: %v", err) + } + // Query the created chain of RBAC Engines with different args to see + // if the chain of RBAC Engines configured as such works as intended. + for _, data := range test.rbacQueries { // What about specifying the RPC Data to send around, pulling stuff off that RPC Data to construct three basic...? + // Construct the context with three data points that have enough + // information to represent incoming RPC's. This will be how a + // user uses this API. + ctx := metadata.NewIncomingContext(context.Background(), data.md) + // data.conn is used for local addr + port + ctx = SetConnection(ctx, data.conn) // The RPC has to happen on a connection, so this will never be unspecified... + // data.pi is used for remote addr + port + ctx = peer.NewContext(ctx, data.pi) + data.pi. + err := cre.DetermineStatus(Data{ + Ctx: ctx, + MethodName: data.methodName, + }) + + // I think a nil status represents a OK code, perhaps return a nil error instead? + status, _ := status.FromError(err) + if data.wantStatusCode != status.Code() { + t.Fatalf("DetermineStatus() returned (), want()") + } + } + }) + } +} + // After writing all these tests, also need to check code coverage From 9ff32559561df225f4b6f8f1e32e9f8958964144 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Tue, 6 Jul 2021 15:26:51 -0400 Subject: [PATCH 13/36] Save progress --- internal/xds/rbac/rbac_engine.go | 14 ++++-- internal/xds/rbac/rbac_engine_test.go | 72 ++++++++++++++++++++++----- 2 files changed, 69 insertions(+), 17 deletions(-) diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index 04aef107642d..a3485f7cb695 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -172,7 +172,8 @@ func newRPCData(data Data) (*rpcData, error) { // *Big question*: Thought I just // You need the connection in order to find the destination address and port // of the incoming RPC Call. conn := getConnection(data.Ctx) - _, dPort, err := net.SplitHostPort(conn.LocalAddr().String()) + _, dPort, err := net.SplitHostPort(conn.LocalAddr().String()) // Nil panic here, although this will never in practice, because conn must be there + // "Behavior is undefined if pass in something with nil conn" if err != nil { return nil, err } @@ -181,9 +182,12 @@ func newRPCData(data Data) (*rpcData, error) { // *Big question*: Thought I just return nil, err } - tlsInfo, ok := pi.AuthInfo.(credentials.TLSInfo) // Does this have to be TLS? - if !ok { - return nil, errors.New("wrong credentials provided, need to be tls") + // What happens if this is nil? Will this typecast work? + tlsInfo, ok := pi.AuthInfo.(credentials.TLSInfo) // Does this have to be TLS? If grpc has to specify HTTPS, then it should, if can be just HTTP + var peerCertificates []*x509.Certificate + if ok { + // return nil, errors.New("wrong credentials provided, need to be tls") // Perhaps scale this error back + peerCertificates = tlsInfo.State.PeerCertificates } // I think we should require all 3 except TLS Info might be questionable @@ -193,7 +197,7 @@ func newRPCData(data Data) (*rpcData, error) { // *Big question*: Thought I just fullMethod: data.MethodName, destinationPort: uint32(dp), destinationAddr: conn.LocalAddr(), - certs: tlsInfo.State.PeerCertificates, + certs: peerCertificates, // Set this to nil if not there, have a nil check in authenticated matcher }, nil } diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index f377846befd3..d32cafa266ae 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -18,6 +18,7 @@ package rbac import ( "context" + "fmt" "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" "google.golang.org/grpc/peer" @@ -1009,6 +1010,8 @@ func (s) TestChainedRBACEngine(t *testing.T) { tests := []struct { name string rbacConfigs []*v3rbacpb.RBAC // Scaled up to handle long list + // Actually, rather than defining the three things you pipe into context, define RPC Data, + // and pull stuff off, build md, pi and conn, put that into context, send that to chain rbacQueries []struct { /*Previously: (What you send into API, What you get back from API) @@ -1021,10 +1024,12 @@ func (s) TestChainedRBACEngine(t *testing.T) { // Generic Data in context (Metadata, PeerInfo, Conn) that will get pulled out // Why not literally have the four data points: Metadata, PeerInfo, and Connection and MethodName // Pipe first three into context, call the function with fourth as well - md metadata.MD + /*md metadata.MD pi *peer.Peer conn net.Conn methodName string + wantStatusCode codes.Code*/ + rpcData *rpcData wantStatusCode codes.Code } }{ @@ -1049,15 +1054,15 @@ func (s) TestChainedRBACEngine(t *testing.T) { }, }, rbacQueries: []struct { - md metadata.MD - pi *peer.Peer - conn net.Conn - methodName string + rpcData *rpcData wantStatusCode codes.Code }{ { // If you unspecify the three, this will logically represent the same thing as previous. What will happen in this scenario? - methodName: "some method", + rpcData: &rpcData{ + fullMethod: "some method", + // The rest default i.e. make connection, but otherwise have it default + }, wantStatusCode: codes.OK, }, }, @@ -1103,15 +1108,58 @@ func (s) TestChainedRBACEngine(t *testing.T) { // Construct the context with three data points that have enough // information to represent incoming RPC's. This will be how a // user uses this API. - ctx := metadata.NewIncomingContext(context.Background(), data.md) + ctx := metadata.NewIncomingContext(context.Background(), data.rpcData.md) // This stays the same + + // Make a TCP connection with a certain destination port. The + // address/port of this connection will be used to populate the + // destination ip/port in RPCData struct. This represents what + // the user of ChainedRBACEngine will have to place into + // context, as this is only way to get destination ip and port. + lis, err := net.Listen("tcp", "localhost:"+fmt.Sprint(data.rpcData.destinationPort)) + var conn net.Conn + go func() { + conn, err = lis.Accept() + if err != nil { + t.Fatalf("Error accepting connection: %v", err) + return + } + }() + // I feel like what I have to do is this: + // Client: Dial, Server: net.Listen (listening on a configurable port), dest addr + port + // ==, now have a conn connecting client and server + // the thing is, we're testing that the conversion works, so can't mock or hook into RPCData passed + // into chain of engines + net.Dial("tcp", lis.Addr().String()) + + // Construct connection to put in context here + // Connection needs to have a certain IP and a certain PORT (dest ip + dest port) + // How do you construct a connection like that? + // net.Listen up there? Pass that in? - I think this might be best option + // conn := /*Mock Connection here...*/ // PROBLEM STATEMENT: HOW DOES (IF EVEN...) THE CODEBASE DEFINE A MOCK CONNECTION? + // The only thing that this connection needs is a way to get IP Address string it will split into address + port + + // If we construct PeerInfo from net.Listen, how would that work? + // Construct PeerInfo to put in context here + // PeerInfo has authinfo + addr + // if authinfo isn't tls, unauthenticated user + // addr is something easily mockable, take addr defined in RPCData specified and put it inside PeerInfo + // Like we had before + // pi := &peer.Peer{ + // Addr: /*Construct an address based on rpc specification, also have this default*/, + // AuthInfo: // If the test specifies list of credentials, have this thing have tls auth info, + // } Outside of Connection, PeerInfo is still defined in RPCData... + + // These three things -> RPCData, define RPCData, use that to build three things, like how user of API will use + // data.conn is used for local addr + port - ctx = SetConnection(ctx, data.conn) // The RPC has to happen on a connection, so this will never be unspecified... + ctx = SetConnection(ctx, conn) // The RPC has to happen on a connection, so this will never be unspecified..., how do we have a connection with a mock address // data.pi is used for remote addr + port - ctx = peer.NewContext(ctx, data.pi) - data.pi. - err := cre.DetermineStatus(Data{ + // Set this to TLS Credentials if specified... + ctx = peer.NewContext(ctx, data.rpcData.peerInfo) // This has auth info (new thing) + addr, define this in test case + // Default should be present, but when looks into it it pulls off nil value... + err = cre.DetermineStatus(Data{ Ctx: ctx, - MethodName: data.methodName, + MethodName: data.rpcData.fullMethod, }) // I think a nil status represents a OK code, perhaps return a nil error instead? From 4fe746b3a96c170592848d5678b4378ec66e45e0 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 7 Jul 2021 09:48:35 -0400 Subject: [PATCH 14/36] Save progress, almost done --- internal/xds/rbac/rbac_engine.go | 12 +- internal/xds/rbac/rbac_engine_test.go | 406 +++++++++++++++++++++++--- 2 files changed, 367 insertions(+), 51 deletions(-) diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index a3485f7cb695..6d62d8c5537c 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -182,12 +182,14 @@ func newRPCData(data Data) (*rpcData, error) { // *Big question*: Thought I just return nil, err } - // What happens if this is nil? Will this typecast work? - tlsInfo, ok := pi.AuthInfo.(credentials.TLSInfo) // Does this have to be TLS? If grpc has to specify HTTPS, then it should, if can be just HTTP var peerCertificates []*x509.Certificate - if ok { - // return nil, errors.New("wrong credentials provided, need to be tls") // Perhaps scale this error back - peerCertificates = tlsInfo.State.PeerCertificates + // What happens if this is nil? Will this typecast work? + if pi.AuthInfo != nil { + tlsInfo, ok := pi.AuthInfo.(credentials.TLSInfo) // Does this have to be TLS? If grpc has to specify HTTPS, then it should, if can be just HTTP + if ok { + // return nil, errors.New("wrong credentials provided, need to be tls") // Perhaps scale this error back + peerCertificates = tlsInfo.State.PeerCertificates + } } // I think we should require all 3 except TLS Info might be questionable diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index d32cafa266ae..da7c334c91e3 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -47,6 +47,9 @@ type addr struct { ipAddress string } +func (addr) Network() string { return "" } +func (a *addr) String() string { return a.ipAddress } + // Actually I might keep these... // "Getters don't make sense" @@ -62,8 +65,6 @@ type addr struct { // Or helperfunciton(ctx) data prepopulated... /* -func (addr) Network() string { return "" } -func (a *addr) String() string { return a.ipAddress } // TestRBACEngineConstruction tests the construction of the RBAC Engine. Due to // some types of RBAC configuration being logically wrong and returning an error @@ -999,8 +1000,6 @@ func (s) TestChainedRBACEngineConstruction(t *testing.T) { // Function here for once configured and instantiated, check incoming RPC's against it // Will check conversion function + chaining logic of whether incoming RPC's should be allowed -// Thus, this needs to construct a context at the beginning full of the things that represent data - // TestChainedRBACEngine tests the chain of RBAC Engines by configuring the // chain of engines in a certain way in different scenarios. After configuring // the chain of engines in a certain way, this test pings the chain of engines @@ -1062,6 +1061,9 @@ func (s) TestChainedRBACEngine(t *testing.T) { rpcData: &rpcData{ fullMethod: "some method", // The rest default i.e. make connection, but otherwise have it default + peerInfo: &peer.Peer{ + Addr: &addr{ipAddress: "0.0.0.0"}, + }, }, wantStatusCode: codes.OK, }, @@ -1070,7 +1072,7 @@ func (s) TestChainedRBACEngine(t *testing.T) { // TestSuccessCaseSimplePolicy is a test that tests a single policy // that only allows an rpc to proceed if the rpc is calling with a certain // path and port. - /*{ + { name: "TestSuccessCaseSimplePolicy", rbacConfigs: []*v3rbacpb.RBAC{ { @@ -1088,9 +1090,342 @@ func (s) TestChainedRBACEngine(t *testing.T) { }, }, rbacQueries: []struct { - // Why + rpcData *rpcData + wantStatusCode codes.Code + }{ + // This RPC should match with the local host fan policy. Thus, + // this RPC should be allowed to proceed. + { + rpcData: &rpcData{ + md: map[string][]string{ + ":path": {"localhost-fan-page"}, + }, + destinationPort: 8080, + peerInfo: &peer.Peer{ + Addr: &addr{ipAddress: "0.0.0.0"}, + }, + }, + wantStatusCode: codes.OK, + }, + + // This RPC shouldn't match with the local host fan policy. Thus, + // this rpc shouldn't be allowed to proceed. + { + rpcData: &rpcData{ + destinationPort: 8000, + peerInfo: &peer.Peer{ + Addr: &addr{ipAddress: "0.0.0.0"}, + }, + }, + wantStatusCode: codes.PermissionDenied, + }, + }, + }, + // TestSuccessCaseEnvoyExample is a test based on the example provided + // in the EnvoyProxy docs. The RBAC Config contains two policies, + // service admin and product viewer, that provides an example of a real + // RBAC Config that might be configured for a given for a given backend + // service. + { + name: "TestSuccessCaseEnvoyExample", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Policies: map[string]*v3rbacpb.Policy{ + "service-admin": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Any{Any: true}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Authenticated_{Authenticated: &v3rbacpb.Principal_Authenticated{PrincipalName: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "cluster.local/ns/default/sa/admin"}}}}}, + {Identifier: &v3rbacpb.Principal_Authenticated_{Authenticated: &v3rbacpb.Principal_Authenticated{PrincipalName: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "cluster.local/ns/default/sa/superuser"}}}}}, + }, + }, + "product-viewer": { + Permissions: []*v3rbacpb.Permission{ + { + Rule: &v3rbacpb.Permission_AndRules{AndRules: &v3rbacpb.Permission_Set{ + Rules: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_ExactMatch{ExactMatch: "GET"}}}}, + {Rule: &v3rbacpb.Permission_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Prefix{Prefix: "/products"}}}}}}, + {Rule: &v3rbacpb.Permission_OrRules{OrRules: &v3rbacpb.Permission_Set{ + Rules: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 80}}, + {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 443}}, + }, + }}}, + }, + }, + }, + }, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Any{Any: true}}, + }, + }, + }, + }, + }, + rbacQueries: []struct { + rpcData *rpcData + wantStatusCode codes.Code + }{ + // This incoming RPC Call should match with the service admin + // policy. + { + rpcData: &rpcData{ + fullMethod: "some method", + peerInfo: &peer.Peer{ + Addr: &addr{ipAddress: "0.0.0.0"}, + // Auth Info logically representing something with TLS Certs + // here. From the TLS Certs, the principal name should be: "cluster.local/ns/default/sa/admin" + }, + }, + wantStatusCode: codes.OK, + }, + // These incoming RPC calls should not match any policy. + { + rpcData: &rpcData{ + destinationPort: 8000, + peerInfo: &peer.Peer{ + Addr: &addr{ipAddress: "0.0.0.0"}, + }, + }, + wantStatusCode: codes.PermissionDenied, + }, + { + rpcData: &rpcData{ + destinationPort: 8080, + fullMethod: "get-product-list", + peerInfo: &peer.Peer{ + Addr: &addr{ipAddress: "0.0.0.0"}, + }, + }, + wantStatusCode: codes.PermissionDenied, + }, + { + rpcData: &rpcData{ + destinationPort: 8080, + peerInfo: &peer.Peer{ + Addr: &addr{ipAddress: "0.0.0.0"}, + // Principal Name here - "localhost", will be done with Auth Info + }, + }, + wantStatusCode: codes.PermissionDenied, + }, + }, + }, + { + name: "TestNotMatcher", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Policies: map[string]*v3rbacpb.Policy{ + "not-secret-content": { + Permissions: []*v3rbacpb.Permission{ + { + Rule: &v3rbacpb.Permission_NotRule{ + NotRule: &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Prefix{Prefix: "/secret-content"}}}}}}, + }, + }, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Any{Any: true}}, + }, + }, + }, + }, + }, + rbacQueries: []struct { + rpcData *rpcData + wantStatusCode codes.Code + }{ + // This incoming RPC Call should match with the not-secret-content policy. + { + rpcData: &rpcData{ + fullMethod: "/regular-content", + peerInfo: &peer.Peer{ + Addr: &addr{ipAddress: "0.0.0.0"}, + }, + }, + wantStatusCode: codes.OK, + }, + // This incoming RPC Call shouldn't match with the not-secret-content-policy. + { + rpcData: &rpcData{ + fullMethod: "/secret-content", + peerInfo: &peer.Peer{ + Addr: &addr{ipAddress: "0.0.0.0"}, + }, + }, + wantStatusCode: codes.PermissionDenied, + }, }, - },*/ + }, + { + name: "TestSourceIpMatcher", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Policies: map[string]*v3rbacpb.Policy{ + "certain-source-ip": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Any{Any: true}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_DirectRemoteIp{DirectRemoteIp: &v3corepb.CidrRange{AddressPrefix: "0.0.0.0", PrefixLen: &wrapperspb.UInt32Value{Value: uint32(10)}}}}, + }, + }, + }, + }, + }, + rbacQueries: []struct { + rpcData *rpcData + wantStatusCode codes.Code + }{ + // This incoming RPC Call should match with the certain-source-ip policy. + { + rpcData: &rpcData{ + peerInfo: &peer.Peer{ + Addr: &addr{ipAddress: "0.0.0.0"}, + }, + }, + wantStatusCode: codes.OK, + }, + // This incoming RPC Call shouldn't match with the certain-source-ip policy. + { + rpcData: &rpcData{ + peerInfo: &peer.Peer{ + Addr: &addr{ipAddress: "10.0.0.0"}, + }, + }, + wantStatusCode: codes.PermissionDenied, + }, + }, + }, + // What we should do for this matcher is have it specify whatever address localhost is. + // Or perhaps just get rid of this one + { + name: "TestDestinationIpMatcher", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Policies: map[string]*v3rbacpb.Policy{ + "certain-destination-ip": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_DestinationIp{DestinationIp: &v3corepb.CidrRange{AddressPrefix: "0.0.0.0", PrefixLen: &wrapperspb.UInt32Value{Value: uint32(10)}}}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Any{Any: true}}, + }, + }, + }, + }, + }, + rbacQueries: []struct { + rpcData *rpcData + wantStatusCode codes.Code + }{ + // This incoming RPC Call shouldn't match with the + // certain-destination-ip policy, as the test listens on local + // host. + { + rpcData: &rpcData{ + peerInfo: &peer.Peer{ + Addr: &addr{ipAddress: "10.0.0.0"}, + }, + }, + wantStatusCode: codes.PermissionDenied, + }, + }, + }, + // TestAllowAndDenyPolicy tests a policy with an allow (on path) and + // deny (on port) policy chained together. This represents how a user + // configured interceptor would use this, and also is a potential + // configuration for a dynamic xds interceptor. + { + name: "TestAllowAndDenyPolicy", + rbacConfigs: []*v3rbacpb.RBAC{ + { + Policies: map[string]*v3rbacpb.Policy{ + "certain-source-ip": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Any{Any: true}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_DirectRemoteIp{DirectRemoteIp: &v3corepb.CidrRange{AddressPrefix: "0.0.0.0", PrefixLen: &wrapperspb.UInt32Value{Value: uint32(10)}}}}, + }, + }, + }, + Action: v3rbacpb.RBAC_ALLOW, + }, + { + Policies: map[string]*v3rbacpb.Policy{ + "localhost-fan": { + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 8080}}, + {Rule: &v3rbacpb.Permission_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "localhost-fan-page"}}}}}}, + }, + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Any{Any: true}}, + }, + }, + }, + Action: v3rbacpb.RBAC_DENY, + }, + }, + rbacQueries: []struct { + rpcData *rpcData + wantStatusCode codes.Code + }{ + // This RPC should match with the allow policy, and shouldn't + // match with the deny and thus should be allowed to proceed. + { + rpcData: &rpcData{ + destinationPort: 8000, + peerInfo: &peer.Peer{ + Addr: &addr{ipAddress: "0.0.0.0"}, + }, + }, + wantStatusCode: codes.OK, + }, + // This RPC should match with both the allow policy and deny policy + // and thus shouldn't be allowed to proceed as matched with deny. + { + rpcData: &rpcData{ + md: map[string][]string{ + ":path": {"localhost-fan-page"}, + }, + destinationPort: 8080, + peerInfo: &peer.Peer{ + Addr: &addr{ipAddress: "0.0.0.0"}, + }, + }, + wantStatusCode: codes.PermissionDenied, + }, + // This RPC shouldn't match with either policy, and thus + // shouldn't be allowed to proceed as didn't match with allow. + { + rpcData: &rpcData{ + destinationPort: 8000, + peerInfo: &peer.Peer{ + Addr: &addr{ipAddress: "10.0.0.0"}, + }, + }, + wantStatusCode: codes.PermissionDenied, + }, + // This RPC shouldn't match with allow, match with deny, and + // thus shouldn't be allowed to proceed. + { + rpcData: &rpcData{ + md: map[string][]string{ + ":path": {"localhost-fan-page"}, + }, + destinationPort: 8080, + peerInfo: &peer.Peer{ + Addr: &addr{ipAddress: "10.0.0.0"}, + }, + }, + wantStatusCode: codes.PermissionDenied, + }, + }, + }, } // Also check what happens if stuff isn't already in context (i.e. metadata, peerinfo, conn) @@ -1104,11 +1439,12 @@ func (s) TestChainedRBACEngine(t *testing.T) { } // Query the created chain of RBAC Engines with different args to see // if the chain of RBAC Engines configured as such works as intended. - for _, data := range test.rbacQueries { // What about specifying the RPC Data to send around, pulling stuff off that RPC Data to construct three basic...? + for _, data := range test.rbacQueries { // Construct the context with three data points that have enough // information to represent incoming RPC's. This will be how a - // user uses this API. - ctx := metadata.NewIncomingContext(context.Background(), data.rpcData.md) // This stays the same + // user uses this API. A user will have to put MD, PeerInfo, and + // the connection rpc is sent on in the context. + ctx := metadata.NewIncomingContext(context.Background(), data.rpcData.md) // Make a TCP connection with a certain destination port. The // address/port of this connection will be used to populate the @@ -1116,47 +1452,19 @@ func (s) TestChainedRBACEngine(t *testing.T) { // the user of ChainedRBACEngine will have to place into // context, as this is only way to get destination ip and port. lis, err := net.Listen("tcp", "localhost:"+fmt.Sprint(data.rpcData.destinationPort)) - var conn net.Conn + connCh := make(chan net.Conn, 1) go func() { - conn, err = lis.Accept() + conn, err := lis.Accept() if err != nil { t.Fatalf("Error accepting connection: %v", err) return } + connCh <- conn }() - // I feel like what I have to do is this: - // Client: Dial, Server: net.Listen (listening on a configurable port), dest addr + port - // ==, now have a conn connecting client and server - // the thing is, we're testing that the conversion works, so can't mock or hook into RPCData passed - // into chain of engines net.Dial("tcp", lis.Addr().String()) - - // Construct connection to put in context here - // Connection needs to have a certain IP and a certain PORT (dest ip + dest port) - // How do you construct a connection like that? - // net.Listen up there? Pass that in? - I think this might be best option - // conn := /*Mock Connection here...*/ // PROBLEM STATEMENT: HOW DOES (IF EVEN...) THE CODEBASE DEFINE A MOCK CONNECTION? - // The only thing that this connection needs is a way to get IP Address string it will split into address + port - - // If we construct PeerInfo from net.Listen, how would that work? - // Construct PeerInfo to put in context here - // PeerInfo has authinfo + addr - // if authinfo isn't tls, unauthenticated user - // addr is something easily mockable, take addr defined in RPCData specified and put it inside PeerInfo - // Like we had before - // pi := &peer.Peer{ - // Addr: /*Construct an address based on rpc specification, also have this default*/, - // AuthInfo: // If the test specifies list of credentials, have this thing have tls auth info, - // } Outside of Connection, PeerInfo is still defined in RPCData... - - // These three things -> RPCData, define RPCData, use that to build three things, like how user of API will use - - // data.conn is used for local addr + port - ctx = SetConnection(ctx, conn) // The RPC has to happen on a connection, so this will never be unspecified..., how do we have a connection with a mock address - // data.pi is used for remote addr + port - // Set this to TLS Credentials if specified... - ctx = peer.NewContext(ctx, data.rpcData.peerInfo) // This has auth info (new thing) + addr, define this in test case - // Default should be present, but when looks into it it pulls off nil value... + conn := <-connCh + ctx = SetConnection(ctx, conn) + ctx = peer.NewContext(ctx, data.rpcData.peerInfo) // This has auth info (new thing) + addr (was already there previously), define this in test case err = cre.DetermineStatus(Data{ Ctx: ctx, MethodName: data.rpcData.fullMethod, @@ -1165,11 +1473,17 @@ func (s) TestChainedRBACEngine(t *testing.T) { // I think a nil status represents a OK code, perhaps return a nil error instead? status, _ := status.FromError(err) if data.wantStatusCode != status.Code() { - t.Fatalf("DetermineStatus() returned (), want()") + t.Fatalf("DetermineStatus(%+v, %+v) returned (%+v), want(%+v)", ctx, data.rpcData.fullMethod, status.Code(), data.wantStatusCode) } + conn.Close() + lis.Close() } }) } } -// After writing all these tests, also need to check code coverage +// Task list: Fix Success Case envoy example (principal name logic) +// Scale up tests to allow + deny policy +// Cleanup, send out for PR :) + +// After writing all these tests, also need to check code coverage, especially for authenticated stuff From 9ea71035ede3988fd637f60ce07c070c814de03d Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 7 Jul 2021 10:05:35 -0400 Subject: [PATCH 15/36] Clean up --- internal/xds/rbac/rbac_engine.go | 38 +- internal/xds/rbac/rbac_engine_test.go | 627 +------------------------- 2 files changed, 27 insertions(+), 638 deletions(-) diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index 6d62d8c5537c..5eb588ec9b9f 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -46,7 +46,7 @@ type ChainedRBACEngine struct { func NewChainEngine(policies []*v3rbacpb.RBAC) (*ChainedRBACEngine, error) { var chainedEngines []*engine for _, policy := range policies { - rbacEngine, err := NewEngine(policy) + rbacEngine, err := newEngine(policy) if err != nil { return nil, err } @@ -100,12 +100,10 @@ type engine struct { action action } -// NewEngine creates an RBAC Engine based on the contents of policy. If the +// newEngine creates an RBAC Engine based on the contents of policy. If the // config is invalid (and fails to build underlying tree of matchers), NewEngine -// will return an error. This created RBAC Engine will not persist the action -// present in the policy, and will leave up to caller to handle the action that -// is attached to the config. -func NewEngine(policy *v3rbacpb.RBAC) (*engine, error) { +// will return an error. +func newEngine(policy *v3rbacpb.RBAC) (*engine, error) { var action action switch *policy.Action.Enum() { case v3rbacpb.RBAC_ALLOW: @@ -154,11 +152,13 @@ type Data struct { } // newRPCData takes a incoming context (should be a context representing state -// needed for server RPC Call with headers and connection piped into it) and the -// method name of the Service being called server side and populates an RPCData -// struct ready to be passed to the RBAC Engine to find a matching policy. -func newRPCData(data Data) (*rpcData, error) { // *Big question*: Thought I just had: For this function on an error case, should it really return an error in certain situations, as it doesn't really need all 6 fields... - // Will we leave it up to caller to pipe these three pieces of data into context? Right now, as expected, it's failing. +// needed for server RPC Call with headers, connection and peer info piped into +// it) and the method name of the Service being called server side and populates +// an rpcData struct ready to be passed to the RBAC Engine to find a matching +// policy. +func newRPCData(data Data) (*rpcData, error) { + // The caller should populate all of these fields (i.e. for empty headers, + // pipe an empty md into context). md, ok := metadata.FromIncomingContext(data.Ctx) if !ok { return nil, errors.New("error retrieving metadata from incoming ctx") @@ -169,11 +169,10 @@ func newRPCData(data Data) (*rpcData, error) { // *Big question*: Thought I just return nil, errors.New("error retrieving peer info from incoming ctx") } - // You need the connection in order to find the destination address and port - // of the incoming RPC Call. + // The connection is needed in order to find the destination address and + // port of the incoming RPC Call. conn := getConnection(data.Ctx) - _, dPort, err := net.SplitHostPort(conn.LocalAddr().String()) // Nil panic here, although this will never in practice, because conn must be there - // "Behavior is undefined if pass in something with nil conn" + _, dPort, err := net.SplitHostPort(conn.LocalAddr().String()) if err != nil { return nil, err } @@ -183,23 +182,20 @@ func newRPCData(data Data) (*rpcData, error) { // *Big question*: Thought I just } var peerCertificates []*x509.Certificate - // What happens if this is nil? Will this typecast work? if pi.AuthInfo != nil { - tlsInfo, ok := pi.AuthInfo.(credentials.TLSInfo) // Does this have to be TLS? If grpc has to specify HTTPS, then it should, if can be just HTTP + tlsInfo, ok := pi.AuthInfo.(credentials.TLSInfo) if ok { - // return nil, errors.New("wrong credentials provided, need to be tls") // Perhaps scale this error back peerCertificates = tlsInfo.State.PeerCertificates } } - // I think we should require all 3 except TLS Info might be questionable - return &rpcData{ // Big question: What to do when data is not specified...? Should we require all 3? + return &rpcData{ md: md, peerInfo: pi, fullMethod: data.MethodName, destinationPort: uint32(dp), destinationAddr: conn.LocalAddr(), - certs: peerCertificates, // Set this to nil if not there, have a nil check in authenticated matcher + certs: peerCertificates, }, nil } diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index da7c334c91e3..a5abfd752b12 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -19,10 +19,6 @@ package rbac import ( "context" "fmt" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/metadata" - "google.golang.org/grpc/peer" - "google.golang.org/grpc/status" "net" "testing" @@ -32,7 +28,11 @@ import ( v3matcherpb "github.com/envoyproxy/go-control-plane/envoy/type/matcher/v3" v3typepb "github.com/envoyproxy/go-control-plane/envoy/type/v3" wrapperspb "github.com/golang/protobuf/ptypes/wrappers" + "google.golang.org/grpc/codes" "google.golang.org/grpc/internal/grpctest" + "google.golang.org/grpc/metadata" + "google.golang.org/grpc/peer" + "google.golang.org/grpc/status" ) type s struct { @@ -50,586 +50,6 @@ type addr struct { func (addr) Network() string { return "" } func (a *addr) String() string { return a.ipAddress } -// Actually I might keep these... - -// "Getters don't make sense" -// So keep this function here, but the only thing RBAC defines in the API layer is the context, but will call this function - -// Errors combine - -// How does the logic in c core work {1, 2, 3, 4, 5, 6} <- does each field need to be populated with data, does this make sense? -// Right now this function has to have everything -// Could instantiate it with ctx, pull stuff out of context as needed (will return errors there) -// Instantiate with ctx -// Pull stuff out of that generic data as needed...with getters -// Or helperfunciton(ctx) data prepopulated... - -/* - -// TestRBACEngineConstruction tests the construction of the RBAC Engine. Due to -// some types of RBAC configuration being logically wrong and returning an error -// rather than successfully constructing the RBAC Engine, this test tests both -// RBAC Configurations deemed successful and also RBAC Configurations that will -// raise errors. -func (s) TestRBACEngineConstruction(t *testing.T) { - tests := []struct { - name string - rbacConfig *v3rbacpb.RBAC - wantErr bool - }{ - { - name: "TestSuccessCaseAnyMatch", - rbacConfig: &v3rbacpb.RBAC{ - Policies: map[string]*v3rbacpb.Policy{ - "anyone": { - Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_Any{Any: true}}, - }, - Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_Any{Any: true}}, - }, - }, - }, - }, - }, - { - name: "TestSuccessCaseSimplePolicy", - rbacConfig: &v3rbacpb.RBAC{ - Policies: map[string]*v3rbacpb.Policy{ - "localhost-fan": { - Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 8080}}, - {Rule: &v3rbacpb.Permission_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "localhost-fan-page"}}}}}}, - }, - Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_Any{Any: true}}, - }, - }, - }, - }, - }, - { - name: "TestSuccessCaseEnvoyExample", - rbacConfig: &v3rbacpb.RBAC{ - Policies: map[string]*v3rbacpb.Policy{ - "service-admin": { - Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_Any{Any: true}}, - }, - Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_Authenticated_{Authenticated: &v3rbacpb.Principal_Authenticated{PrincipalName: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "cluster.local/ns/default/sa/admin"}}}}}, - {Identifier: &v3rbacpb.Principal_Authenticated_{Authenticated: &v3rbacpb.Principal_Authenticated{PrincipalName: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "cluster.local/ns/default/sa/superuser"}}}}}, - }, - }, - "product-viewer": { - Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_AndRules{AndRules: &v3rbacpb.Permission_Set{ - Rules: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_ExactMatch{ExactMatch: "GET"}}}}, - {Rule: &v3rbacpb.Permission_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Prefix{Prefix: "/products"}}}}}}, - {Rule: &v3rbacpb.Permission_OrRules{OrRules: &v3rbacpb.Permission_Set{ - Rules: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 80}}, - {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 443}}, - }, - }}}, - }, - }, - }, - }, - }, - Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_Any{Any: true}}, - }, - }, - }, - }, - }, - { - name: "TestSourceIpMatcherSuccess", - rbacConfig: &v3rbacpb.RBAC{ - Policies: map[string]*v3rbacpb.Policy{ - "certain-source-ip": { - Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_Any{Any: true}}, - }, - Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_DirectRemoteIp{DirectRemoteIp: &v3corepb.CidrRange{AddressPrefix: "0.0.0.0", PrefixLen: &wrapperspb.UInt32Value{Value: uint32(10)}}}}, - }, - }, - }, - }, - }, - { - name: "TestSourceIpMatcherFailure", - rbacConfig: &v3rbacpb.RBAC{ - Policies: map[string]*v3rbacpb.Policy{ - "certain-source-ip": { - Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_Any{Any: true}}, - }, - Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_DirectRemoteIp{DirectRemoteIp: &v3corepb.CidrRange{AddressPrefix: "not a correct address", PrefixLen: &wrapperspb.UInt32Value{Value: uint32(10)}}}}, - }, - }, - }, - }, - wantErr: true, - }, - { - name: "TestDestinationIpMatcherSuccess", - rbacConfig: &v3rbacpb.RBAC{ - Policies: map[string]*v3rbacpb.Policy{ - "certain-destination-ip": { - Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_DestinationIp{DestinationIp: &v3corepb.CidrRange{AddressPrefix: "0.0.0.0", PrefixLen: &wrapperspb.UInt32Value{Value: uint32(10)}}}}, - }, - Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_Any{Any: true}}, - }, - }, - }, - }, - }, - { - name: "TestDestinationIpMatcherFailure", - rbacConfig: &v3rbacpb.RBAC{ - Policies: map[string]*v3rbacpb.Policy{ - "certain-destination-ip": { - Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_DestinationIp{DestinationIp: &v3corepb.CidrRange{AddressPrefix: "not a correct address", PrefixLen: &wrapperspb.UInt32Value{Value: uint32(10)}}}}, - }, - Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_Any{Any: true}}, - }, - }, - }, - }, - wantErr: true, - }, - { - name: "TestMatcherToNotPolicy", - rbacConfig: &v3rbacpb.RBAC{ - Policies: map[string]*v3rbacpb.Policy{ - "not-secret-content": { - Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_NotRule{NotRule: &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Prefix{Prefix: "/secret-content"}}}}}}}}, - }, - Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_Any{Any: true}}, - }, - }, - }, - }, - }, - { - name: "TestMatcherToNotPrincipal", - rbacConfig: &v3rbacpb.RBAC{ - Policies: map[string]*v3rbacpb.Policy{ - "not-from-certain-ip": { - Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_Any{Any: true}}, - }, - Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_NotId{NotId: &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_DirectRemoteIp{DirectRemoteIp: &v3corepb.CidrRange{AddressPrefix: "0.0.0.0", PrefixLen: &wrapperspb.UInt32Value{Value: uint32(10)}}}}}}, - }, - }, - }, - }, - }, - { - name: "TestPrincipalProductViewer", - rbacConfig: &v3rbacpb.RBAC{ - Policies: map[string]*v3rbacpb.Policy{ - "product-viewer": { - Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_Any{Any: true}}, - }, - Principals: []*v3rbacpb.Principal{ - { - Identifier: &v3rbacpb.Principal_AndIds{AndIds: &v3rbacpb.Principal_Set{Ids: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_ExactMatch{ExactMatch: "GET"}}}}, - {Identifier: &v3rbacpb.Principal_OrIds{OrIds: &v3rbacpb.Principal_Set{ - Ids: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Prefix{Prefix: "/books"}}}}}}, - {Identifier: &v3rbacpb.Principal_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Prefix{Prefix: "/cars"}}}}}}, - }, - }}}, - }}}, - }, - }, - }, - }, - }, - }, - { - name: "TestCertainHeaders", - rbacConfig: &v3rbacpb.RBAC{ - Policies: map[string]*v3rbacpb.Policy{ - "certain-headers": { - Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_Any{Any: true}}, - }, - Principals: []*v3rbacpb.Principal{ - { - Identifier: &v3rbacpb.Principal_OrIds{OrIds: &v3rbacpb.Principal_Set{Ids: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_ExactMatch{ExactMatch: "GET"}}}}, - {Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_SafeRegexMatch{SafeRegexMatch: &v3matcherpb.RegexMatcher{Regex: "GET"}}}}}, - {Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_RangeMatch{RangeMatch: &v3typepb.Int64Range{ - Start: 0, - End: 64, - }}}}}, - {Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PresentMatch{PresentMatch: true}}}}, - {Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PrefixMatch{PrefixMatch: "GET"}}}}, - {Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_SuffixMatch{SuffixMatch: "GET"}}}}, - {Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_ContainsMatch{ContainsMatch: "GET"}}}}, - }}}, - }, - }, - }, - }, - }, - }, - } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - if _, err := NewEngine(test.rbacConfig); (err != nil) != test.wantErr { - t.Fatalf("NewEngine(%+v) returned err: %v, wantErr: %v", test.rbacConfig, err, test.wantErr) - } - }) - } -} - -// TestRBACEngine tests the RBAC Engine by configuring the engine in different -// scenarios. After configuring the engine in a certain way, this test pings the -// engine with different kinds of data representing incoming RPC's, and verifies -// that it works as expected. -func (s) TestRBACEngine(t *testing.T) { - tests := []struct { - name string - rbacConfig *v3rbacpb.RBAC - rbacQueries []struct { - rpcData *RPCData - wantMatchingPolicyName string - wantMatch bool - } - }{ - // TestSuccessCaseAnyMatch tests an RBAC Engine instantiated with a - // config with a policy with any rules for both permissions and - // principals, meaning that any data about incoming RPC's that the RBAC - // Engine is queried with should match that policy. - { - name: "TestSuccessCaseAnyMatch", - rbacConfig: &v3rbacpb.RBAC{ - Policies: map[string]*v3rbacpb.Policy{ - "anyone": { - Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_Any{Any: true}}, - }, - Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_Any{Any: true}}, - }, - }, - }, - }, - rbacQueries: // Any incoming RPC should match with the anyone policy - []struct { - rpcData *RPCData - wantMatchingPolicyName string - wantMatch bool - }{ - { - rpcData: &RPCData{ - FullMethod: "some method", - }, - wantMatchingPolicyName: "anyone", - wantMatch: true, - }, - { - rpcData: &RPCData{ - DestinationPort: 100, - }, - wantMatchingPolicyName: "anyone", - wantMatch: true, - }, - }, - }, - // TestSuccessCaseSimplePolicy is a test that tests a simple policy that - // only allows an rpc to proceed if the rpc is calling a certain path - // and port. - { - name: "TestSuccessCaseSimplePolicy", - rbacConfig: &v3rbacpb.RBAC{ - Policies: map[string]*v3rbacpb.Policy{ - "localhost-fan": { - Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 8080}}, - {Rule: &v3rbacpb.Permission_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "localhost-fan-page"}}}}}}, - }, - Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_Any{Any: true}}, - }, - }, - }, - }, - rbacQueries: []struct { - rpcData *RPCData - wantMatchingPolicyName string - wantMatch bool - }{ - // This RPC should match with the local host fan policy. - { - rpcData: &RPCData{ - MD: map[string][]string{ - ":path": {"localhost-fan-page"}, - }, - DestinationPort: 8080, - }, - wantMatchingPolicyName: "localhost-fan", - wantMatch: true}, - // This RPC shouldn't match with the local host fan policy. - { - rpcData: &RPCData{ - DestinationPort: 100, - }, - wantMatchingPolicyName: ""}, - }, - }, - // TestSuccessCaseEnvoyExample is a test based on the example provided - // in the EnvoyProxy docs. The RBAC Config contains two policies, - // service admin and product viewer, that provides an example of a real - // RBAC Config that might be configured for a given for a given backend - // service. - { - name: "TestSuccessCaseEnvoyExample", - rbacConfig: &v3rbacpb.RBAC{ - Policies: map[string]*v3rbacpb.Policy{ - "service-admin": { - Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_Any{Any: true}}, - }, - Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_Authenticated_{Authenticated: &v3rbacpb.Principal_Authenticated{PrincipalName: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "cluster.local/ns/default/sa/admin"}}}}}, - {Identifier: &v3rbacpb.Principal_Authenticated_{Authenticated: &v3rbacpb.Principal_Authenticated{PrincipalName: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "cluster.local/ns/default/sa/superuser"}}}}}, - }, - }, - "product-viewer": { - Permissions: []*v3rbacpb.Permission{ - { - Rule: &v3rbacpb.Permission_AndRules{AndRules: &v3rbacpb.Permission_Set{ - Rules: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_ExactMatch{ExactMatch: "GET"}}}}, - {Rule: &v3rbacpb.Permission_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Prefix{Prefix: "/products"}}}}}}, - {Rule: &v3rbacpb.Permission_OrRules{OrRules: &v3rbacpb.Permission_Set{ - Rules: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 80}}, - {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 443}}, - }, - }}}, - }, - }, - }, - }, - }, - Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_Any{Any: true}}, - }, - }, - }, - }, - rbacQueries: []struct { - rpcData *RPCData - wantMatchingPolicyName string - wantMatch bool - }{ - // This incoming RPC Call should match with the service admin - // policy. - { - rpcData: &RPCData{ - FullMethod: "some method", - PrincipalName: "cluster.local/ns/default/sa/admin", - }, - wantMatchingPolicyName: "service-admin", - wantMatch: true}, - // This incoming RPC Call should match with the product - // viewer policy. - { - rpcData: &RPCData{ - DestinationPort: 80, - MD: map[string][]string{ - ":method": {"GET"}, - }, - FullMethod: "/products", - }, - wantMatchingPolicyName: "product-viewer", - wantMatch: true}, - // These incoming RPC calls should not match any policy - - // represented by an empty matching policy name and false being - // returned. - { - rpcData: &RPCData{ - DestinationPort: 100, - }, - wantMatchingPolicyName: ""}, - { - rpcData: &RPCData{ - FullMethod: "get-product-list", - DestinationPort: 8080, - }, - wantMatchingPolicyName: ""}, - { - rpcData: &RPCData{ - PrincipalName: "localhost", - DestinationPort: 8080, - }, - wantMatchingPolicyName: ""}, - }, - }, - { - name: "TestNotMatcher", - rbacConfig: &v3rbacpb.RBAC{ - Policies: map[string]*v3rbacpb.Policy{ - "not-secret-content": { - Permissions: []*v3rbacpb.Permission{ - { - Rule: &v3rbacpb.Permission_NotRule{ - NotRule: &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Prefix{Prefix: "/secret-content"}}}}}}, - }, - }, - }, - Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_Any{Any: true}}, - }, - }, - }, - }, - rbacQueries: []struct { - rpcData *RPCData - wantMatchingPolicyName string - wantMatch bool - }{ - // This incoming RPC Call should match with the not-secret-content policy. - { - rpcData: &RPCData{ - FullMethod: "/regular-content", - }, - wantMatchingPolicyName: "not-secret-content", - wantMatch: true, - }, - // This incoming RPC Call shouldn't match with the not-secret-content policy. - { - rpcData: &RPCData{ - FullMethod: "/secret-content", - }, - wantMatchingPolicyName: "", - wantMatch: false, - }, - }, - }, - { - name: "TestSourceIpMatcher", - rbacConfig: &v3rbacpb.RBAC{ - Policies: map[string]*v3rbacpb.Policy{ - "certain-source-ip": { - Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_Any{Any: true}}, - }, - Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_DirectRemoteIp{DirectRemoteIp: &v3corepb.CidrRange{AddressPrefix: "0.0.0.0", PrefixLen: &wrapperspb.UInt32Value{Value: uint32(10)}}}}, - }, - }, - }, - }, - rbacQueries: []struct { - rpcData *RPCData - wantMatchingPolicyName string - wantMatch bool - }{ - // This incoming RPC Call should match with the certain-source-ip policy. - { - rpcData: &RPCData{ - PeerInfo: &peer.Peer{ - Addr: &addr{ipAddress: "0.0.0.0"}, - }, - }, - wantMatchingPolicyName: "certain-source-ip", - wantMatch: true, - }, - // This incoming RPC Call shouldn't match with the certain-source-ip policy. - { - rpcData: &RPCData{ - PeerInfo: &peer.Peer{ - Addr: &addr{ipAddress: "10.0.0.0"}, - }, - }, - wantMatchingPolicyName: "", - wantMatch: false, - }, - }, - }, - { - name: "TestDestinationIpMatcher", - rbacConfig: &v3rbacpb.RBAC{ - Policies: map[string]*v3rbacpb.Policy{ - "certain-destination-ip": { - Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_DestinationIp{DestinationIp: &v3corepb.CidrRange{AddressPrefix: "0.0.0.0", PrefixLen: &wrapperspb.UInt32Value{Value: uint32(10)}}}}, - }, - Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_Any{Any: true}}, - }, - }, - }, - }, - rbacQueries: []struct { - rpcData *RPCData - wantMatchingPolicyName string - wantMatch bool - }{ - // This incoming RPC Call should match with the certain-destination-ip policy. - { - rpcData: &RPCData{ - DestinationAddr: &addr{ipAddress: "0.0.0.10"}, - }, - wantMatchingPolicyName: "certain-destination-ip", - wantMatch: true, - }, - // This incoming RPC Call shouldn't match with the certain-destination-ip policy. - { - rpcData: &RPCData{ - DestinationAddr: &addr{ipAddress: "10.0.0.0"}, - }, - wantMatchingPolicyName: "", - }, - }, - }, - } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - // Instantiate the rbacEngine with different configurations that - // interesting to test and to query. - rbacEngine, err := NewEngine(test.rbacConfig) - if err != nil { - t.Fatalf("Error constructing RBAC Engine: %v", err) - } - // Query that created RBAC Engine with different args to see if the - // RBAC Engine configured a certain way works as intended. - for _, queryToRBACEngine := range test.rbacQueries { - // The matchingPolicyName returned will be empty in the case of - // no matching policy. Thus, matchingPolicyName can also be used - // to test the "error" condition of no matching policies. - matchingPolicyName, matchingPolicyFound := rbacEngine.FindMatchingPolicy(queryToRBACEngine.rpcData) - if matchingPolicyFound != queryToRBACEngine.wantMatch || matchingPolicyName != queryToRBACEngine.wantMatchingPolicyName { - t.Errorf("FindMatchingPolicy(%+v) returned (%v, %v), want (%v, %v)", queryToRBACEngine.rpcData, matchingPolicyName, matchingPolicyFound, queryToRBACEngine.wantMatchingPolicyName, queryToRBACEngine.wantMatch) - } - } - }) - } -}*/ - -// Function here for configuration (chaining) -// Take singular configurations from previous - // TestChainedRBACEngineConstruction tests the construction of the // ChainedRBACEngine. Due to some types of RBAC configuration being logically // wrong and returning an error rather than successfully constructing the RBAC @@ -950,7 +370,6 @@ func (s) TestChainedRBACEngineConstruction(t *testing.T) { }, }, }, - // Test LOG and also an action not specified { name: "TestLogAction", rbacConfigs: []*v3rbacpb.RBAC{ @@ -997,37 +416,16 @@ func (s) TestChainedRBACEngineConstruction(t *testing.T) { } } -// Function here for once configured and instantiated, check incoming RPC's against it -// Will check conversion function + chaining logic of whether incoming RPC's should be allowed - // TestChainedRBACEngine tests the chain of RBAC Engines by configuring the // chain of engines in a certain way in different scenarios. After configuring // the chain of engines in a certain way, this test pings the chain of engines -// with different types of data representing incoming RPC's (usually data piped -// into a context), and verifies that it works as expected. +// with different types of data representing incoming RPC's (piped into a +// context), and verifies that it works as expected. func (s) TestChainedRBACEngine(t *testing.T) { tests := []struct { name string - rbacConfigs []*v3rbacpb.RBAC // Scaled up to handle long list - // Actually, rather than defining the three things you pipe into context, define RPC Data, - // and pull stuff off, build md, pi and conn, put that into context, send that to chain + rbacConfigs []*v3rbacpb.RBAC rbacQueries []struct { - /*Previously: - (What you send into API, What you get back from API) - rpcData *RPCData - wantMatchingPolicyName string - wantMatch bool - */ - // DetermineStatus(data Data) error <- will have information about what to do about the RPC - /*Now: You send in generic data to API, you get a status code without matching policy name there*/ - // Generic Data in context (Metadata, PeerInfo, Conn) that will get pulled out - // Why not literally have the four data points: Metadata, PeerInfo, and Connection and MethodName - // Pipe first three into context, call the function with fourth as well - /*md metadata.MD - pi *peer.Peer - conn net.Conn - methodName string - wantStatusCode codes.Code*/ rpcData *rpcData wantStatusCode codes.Code } @@ -1057,10 +455,8 @@ func (s) TestChainedRBACEngine(t *testing.T) { wantStatusCode codes.Code }{ { - // If you unspecify the three, this will logically represent the same thing as previous. What will happen in this scenario? rpcData: &rpcData{ fullMethod: "some method", - // The rest default i.e. make connection, but otherwise have it default peerInfo: &peer.Peer{ Addr: &addr{ipAddress: "0.0.0.0"}, }, @@ -1300,8 +696,6 @@ func (s) TestChainedRBACEngine(t *testing.T) { }, }, }, - // What we should do for this matcher is have it specify whatever address localhost is. - // Or perhaps just get rid of this one { name: "TestDestinationIpMatcher", rbacConfigs: []*v3rbacpb.RBAC{ @@ -1428,7 +822,6 @@ func (s) TestChainedRBACEngine(t *testing.T) { }, } - // Also check what happens if stuff isn't already in context (i.e. metadata, peerinfo, conn) for _, test := range tests { t.Run(test.name, func(t *testing.T) { // Instantiate the chainedRBACEngine with different configurations that are @@ -1443,7 +836,7 @@ func (s) TestChainedRBACEngine(t *testing.T) { // Construct the context with three data points that have enough // information to represent incoming RPC's. This will be how a // user uses this API. A user will have to put MD, PeerInfo, and - // the connection rpc is sent on in the context. + // the connection the RPC is sent on in the context. ctx := metadata.NewIncomingContext(context.Background(), data.rpcData.md) // Make a TCP connection with a certain destination port. The @@ -1464,13 +857,13 @@ func (s) TestChainedRBACEngine(t *testing.T) { net.Dial("tcp", lis.Addr().String()) conn := <-connCh ctx = SetConnection(ctx, conn) - ctx = peer.NewContext(ctx, data.rpcData.peerInfo) // This has auth info (new thing) + addr (was already there previously), define this in test case + ctx = peer.NewContext(ctx, data.rpcData.peerInfo) err = cre.DetermineStatus(Data{ Ctx: ctx, MethodName: data.rpcData.fullMethod, }) - // I think a nil status represents a OK code, perhaps return a nil error instead? + // TODO: I think a nil status represents a OK code, perhaps return a nil error instead? status, _ := status.FromError(err) if data.wantStatusCode != status.Code() { t.Fatalf("DetermineStatus(%+v, %+v) returned (%+v), want(%+v)", ctx, data.rpcData.fullMethod, status.Code(), data.wantStatusCode) From ae0e62c42373d98fdffb473fea330db4b9142b42 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 7 Jul 2021 10:07:17 -0400 Subject: [PATCH 16/36] vet --- internal/xds/rbac/rbac_engine_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index a5abfd752b12..a0b6560ad96d 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -849,7 +849,7 @@ func (s) TestChainedRBACEngine(t *testing.T) { go func() { conn, err := lis.Accept() if err != nil { - t.Fatalf("Error accepting connection: %v", err) + t.Errorf("Error accepting connection: %v", err) return } connCh <- conn From 8b9ebeeef09a353c9db846d0200ba34e08733a66 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 7 Jul 2021 10:09:00 -0400 Subject: [PATCH 17/36] Vet --- internal/xds/rbac/rbac_engine.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index 5eb588ec9b9f..c068961936fc 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -71,13 +71,13 @@ func (cre *ChainedRBACEngine) DetermineStatus(data Data) error { // If the engine action type was allow and a matching policy was not // found, this RPC should be denied. - if engine.action == allow && err == ErrPolicyNotFound { + if engine.action == allow && err == errPolicyNotFound { return status.Error(codes.PermissionDenied, "incoming RPC did not match an allow policy") } // If the engine type was deny and also a matching policy was found, // this RPC should be denied. - if engine.action == deny && err != ErrPolicyNotFound { + if engine.action == deny && err != errPolicyNotFound { return status.Error(codes.PermissionDenied, "incoming RPC matched a deny policy") } } @@ -217,7 +217,7 @@ type rpcData struct { certs []*x509.Certificate } -var ErrPolicyNotFound = errors.New("a matching policy was not found") +var errPolicyNotFound = errors.New("a matching policy was not found") // findMatchingPolicy determines if an incoming RPC matches a policy. On a // successful match, it returns the name of the matching policy and a nil error @@ -229,5 +229,5 @@ func (r *engine) findMatchingPolicy(rpcData *rpcData) (string, error) { return policy, nil } } - return "", ErrPolicyNotFound + return "", errPolicyNotFound } From 16273d71d0f98136150ab4c81f5f3ddb2a013cfa Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 7 Jul 2021 10:11:12 -0400 Subject: [PATCH 18/36] Vet --- internal/xds/rbac/rbac_engine_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index a0b6560ad96d..1c867dd799a3 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -845,6 +845,9 @@ func (s) TestChainedRBACEngine(t *testing.T) { // the user of ChainedRBACEngine will have to place into // context, as this is only way to get destination ip and port. lis, err := net.Listen("tcp", "localhost:"+fmt.Sprint(data.rpcData.destinationPort)) + if err != nil { + t.Fatalf("Error listening: %v", err) + } connCh := make(chan net.Conn, 1) go func() { conn, err := lis.Accept() From 0cb34555202d380986706a7bc0775ea8da144bf2 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 7 Jul 2021 10:24:04 -0400 Subject: [PATCH 19/36] Added Auth Info --- internal/xds/rbac/rbac_engine_test.go | 37 +++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index 1c867dd799a3..2bd39f805ed6 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -18,8 +18,12 @@ package rbac import ( "context" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" "fmt" "net" + "net/url" "testing" v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" @@ -29,6 +33,7 @@ import ( v3typepb "github.com/envoyproxy/go-control-plane/envoy/type/v3" wrapperspb "github.com/golang/protobuf/ptypes/wrappers" "google.golang.org/grpc/codes" + "google.golang.org/grpc/credentials" "google.golang.org/grpc/internal/grpctest" "google.golang.org/grpc/metadata" "google.golang.org/grpc/peer" @@ -532,8 +537,8 @@ func (s) TestChainedRBACEngine(t *testing.T) { {Rule: &v3rbacpb.Permission_Any{Any: true}}, }, Principals: []*v3rbacpb.Principal{ - {Identifier: &v3rbacpb.Principal_Authenticated_{Authenticated: &v3rbacpb.Principal_Authenticated{PrincipalName: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "cluster.local/ns/default/sa/admin"}}}}}, - {Identifier: &v3rbacpb.Principal_Authenticated_{Authenticated: &v3rbacpb.Principal_Authenticated{PrincipalName: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "cluster.local/ns/default/sa/superuser"}}}}}, + {Identifier: &v3rbacpb.Principal_Authenticated_{Authenticated: &v3rbacpb.Principal_Authenticated{PrincipalName: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "//cluster.local/ns/default/sa/admin"}}}}}, + {Identifier: &v3rbacpb.Principal_Authenticated_{Authenticated: &v3rbacpb.Principal_Authenticated{PrincipalName: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "//cluster.local/ns/default/sa/superuser"}}}}}, }, }, "product-viewer": { @@ -572,8 +577,20 @@ func (s) TestChainedRBACEngine(t *testing.T) { fullMethod: "some method", peerInfo: &peer.Peer{ Addr: &addr{ipAddress: "0.0.0.0"}, - // Auth Info logically representing something with TLS Certs - // here. From the TLS Certs, the principal name should be: "cluster.local/ns/default/sa/admin" + AuthInfo: credentials.TLSInfo{ + State: tls.ConnectionState{ + PeerCertificates: []*x509.Certificate{ + { + URIs: []*url.URL{ + { + Host: "cluster.local", + Path: "/ns/default/sa/admin", + }, + }, + }, + }, + }, + }, }, }, wantStatusCode: codes.OK, @@ -603,7 +620,17 @@ func (s) TestChainedRBACEngine(t *testing.T) { destinationPort: 8080, peerInfo: &peer.Peer{ Addr: &addr{ipAddress: "0.0.0.0"}, - // Principal Name here - "localhost", will be done with Auth Info + AuthInfo: credentials.TLSInfo{ + State: tls.ConnectionState{ + PeerCertificates: []*x509.Certificate{ + { + Subject: pkix.Name{ + CommonName: "localhost", + }, + }, + }, + }, + }, }, }, wantStatusCode: codes.PermissionDenied, From 2aad5fb82f9cc5bd0864307c5ae5a0163e59ae12 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Thu, 8 Jul 2021 11:33:17 -0400 Subject: [PATCH 20/36] Responded to Easwar's comments --- internal/xds/rbac/matchers.go | 14 ++- internal/xds/rbac/rbac_engine.go | 138 ++++++++++++-------------- internal/xds/rbac/rbac_engine_test.go | 41 +++----- 3 files changed, 85 insertions(+), 108 deletions(-) diff --git a/internal/xds/rbac/matchers.go b/internal/xds/rbac/matchers.go index 0ea589d6d4c3..d44232652aeb 100644 --- a/internal/xds/rbac/matchers.go +++ b/internal/xds/rbac/matchers.go @@ -375,7 +375,7 @@ type authenticatedMatcher struct { func newAuthenticatedMatcher(authenticatedMatcherConfig *v3rbacpb.Principal_Authenticated) (*authenticatedMatcher, error) { // Represents this line in the RBAC documentation = "If unset, it applies to - // any user that is authenticated". + // any user that is authenticated" (see package-level comments). if authenticatedMatcherConfig.PrincipalName == nil { return &authenticatedMatcher{}, nil } @@ -388,8 +388,9 @@ func newAuthenticatedMatcher(authenticatedMatcherConfig *v3rbacpb.Principal_Auth func (am *authenticatedMatcher) match(data *rpcData) bool { // Represents this line in the RBAC documentation = "If unset, it applies to - // any user that is authenticated". Thus, if a user is authenticated user should - // match. An authenticated user will have a certificate provided. + // any user that is authenticated" (see package-level comments). + // Thus, if a user is authenticated, the user should match. An authenticated + // user will have a certificate provided. if am.stringMatcher == nil { // TODO: Is this correct? If a TLS Certificate is provided, that certificate means // that that individual was logically authenticated with a key. @@ -398,8 +399,8 @@ func (am *authenticatedMatcher) match(data *rpcData) bool { } } - // The precedence according to documentation specifies to use URI SANs at - // first if present. + // The order of matching as per the RBAC documentation (see package-level comments) + // is as follows: URI SANs, DNS SANs, and then subject name. for _, cert := range data.certs { for _, uriSAN := range cert.URIs { if am.stringMatcher.Match(uriSAN.String()) { @@ -407,7 +408,6 @@ func (am *authenticatedMatcher) match(data *rpcData) bool { } } } - // The next thing to check against if present DNS SANs, if present. for _, cert := range data.certs { for _, dnsSAN := range cert.DNSNames { if am.stringMatcher.Match(dnsSAN) { @@ -415,8 +415,6 @@ func (am *authenticatedMatcher) match(data *rpcData) bool { } } } - - // The last thing to check is the subject field. for _, cert := range data.certs { if am.stringMatcher.Match(cert.Subject.String()) { return true diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index c068961936fc..a319ca2d140e 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -15,7 +15,9 @@ */ // Package rbac provides service-level and method-level access control for a -// service. +// service. See +// https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/rbac/v3/rbac.proto#role-based-access-control-rbac +// for documentation. package rbac import ( @@ -33,58 +35,53 @@ import ( "google.golang.org/grpc/status" ) -// ChainedRBACEngine represents a chain of RBAC Engines, which will be used in -// order to determine whether to allow incoming RPC's to proceed according to -// the actions of each engine. -type ChainedRBACEngine struct { +// ChainEngine represents a chain of RBAC Engines, used to make authorization +// decisions on incoming RPCs. +type ChainEngine struct { chainedEngines []*engine } -// NewChainEngine returns a new Chain of RBAC Engines to query if incoming RPC's -// are allowed to proceed or not. This function returns an error in the case -// that one of the policies is invalid. -func NewChainEngine(policies []*v3rbacpb.RBAC) (*ChainedRBACEngine, error) { - var chainedEngines []*engine +// NewChainEngine returns a chain of RBAC engines, used to make authorization +// decisions on incoming RPCs. Returns a non-nil error for invalid policies. +func NewChainEngine(policies []*v3rbacpb.RBAC) (*ChainEngine, error) { + var engines []*engine for _, policy := range policies { - rbacEngine, err := newEngine(policy) + engine, err := newEngine(policy) if err != nil { return nil, err } - chainedEngines = append(chainedEngines, rbacEngine) + engines = append(engines, engine) } - return &ChainedRBACEngine{chainedEngines: chainedEngines}, nil + return &ChainEngine{chainedEngines: engines}, nil } -// DetermineStatus takes in data about incoming RPC's and returns a status error -// representing whether the RPC should be denied or not (i.e. PermissionDenied or OK) -// based on the full list of RBAC Engines and their associated actions. -func (cre *ChainedRBACEngine) DetermineStatus(data Data) error { +// IsAuthorized determines if an incoming RPC is authorized based on the chain of RBAC +// engines and their associated actions. +// +// Errors returned by this function are compatible with the status package. +func (cre *ChainEngine) IsAuthorized(ctx context.Context, methodName string) error { // This conversion step (i.e. pulling things out of generic ctx) can be done // once, and then be used for the whole chain of RBAC Engines. - rpcData, err := newRPCData(data) + rpcData, err := newRPCData(ctx, methodName) if err != nil { - return status.Error(codes.InvalidArgument, "passed in data did not have enough information representing incoming rpc") + return status.Errorf(codes.InvalidArgument, "missing fields in ctx %+v: %v", ctx, err) } for _, engine := range cre.chainedEngines { // TODO: What do I do now with this matchingPolicyName? _, err := engine.findMatchingPolicy(rpcData) - // If the engine action type was allow and a matching policy was not - // found, this RPC should be denied. - if engine.action == allow && err == errPolicyNotFound { - return status.Error(codes.PermissionDenied, "incoming RPC did not match an allow policy") - } - - // If the engine type was deny and also a matching policy was found, - // this RPC should be denied. - if engine.action == deny && err != errPolicyNotFound { - return status.Error(codes.PermissionDenied, "incoming RPC matched a deny policy") + switch { + case engine.action == allow && err == errPolicyNotFound: + return status.Errorf(codes.PermissionDenied, "incoming RPC %v did not match an allow policy", rpcData) + case engine.action == deny && err != errPolicyNotFound: + return status.Errorf(codes.PermissionDenied, "incoming RPC %+v matched a deny policy", rpcData) + default: } } // If the incoming RPC gets through all of the engines successfully (i.e. // doesn't not match an allow or match a deny engine), the RPC is authorized // to proceed. - return status.Error(codes.OK, "rpc is ok to proceed") + return status.Error(codes.OK, "") } type action int @@ -100,9 +97,8 @@ type engine struct { action action } -// newEngine creates an RBAC Engine based on the contents of policy. If the -// config is invalid (and fails to build underlying tree of matchers), NewEngine -// will return an error. +// newEngine creates an RBAC Engine based on the contents of policy. Returns a +// non-nil error if the policy is invalid. func newEngine(policy *v3rbacpb.RBAC) (*engine, error) { var action action switch *policy.Action.Enum() { @@ -114,7 +110,7 @@ func newEngine(policy *v3rbacpb.RBAC) (*engine, error) { return nil, errors.New("unsupported action") } - policies := make(map[string]*policyMatcher) + policies := make(map[string]*policyMatcher, len(policy.Policies)) for name, config := range policy.Policies { matcher, err := newPolicyMatcher(config) if err != nil { @@ -128,50 +124,43 @@ func newEngine(policy *v3rbacpb.RBAC) (*engine, error) { }, nil } -type connectionKey struct{} - -func getConnection(ctx context.Context) net.Conn { - conn, _ := ctx.Value(connectionKey{}).(net.Conn) - return conn -} - -// SetConnection adds the connection to the context to be able to get -// information about the destination ip and port for an incoming RPC. -func SetConnection(ctx context.Context, conn net.Conn) context.Context { - return context.WithValue(ctx, connectionKey{}, conn) -} +var errPolicyNotFound = errors.New("a matching policy was not found") -// Data represents the generic data about incoming RPC's that must be passed -// into the RBAC Engine in order to try and find a matching policy or not. The -// ctx passed in must have metadata, peerinfo (used for source ip/port and TLS -// information) and connection (used for destination ip/port) embedded within -// it. -type Data struct { - Ctx context.Context - MethodName string +// findMatchingPolicy determines if an incoming RPC matches a policy. On a +// successful match, it returns the name of the matching policy and a nil error +// to specify that there was a matching policy found. It returns an error in +// the case of not finding a matching policy. +func (r *engine) findMatchingPolicy(rpcData *rpcData) (string, error) { + for policy, matcher := range r.policies { + if matcher.match(rpcData) { + return policy, nil + } + } + return "", errPolicyNotFound } // newRPCData takes a incoming context (should be a context representing state -// needed for server RPC Call with headers, connection and peer info piped into +// needed for server RPC Call with metadata, peer info (used for source ip/port +// and TLS information) and connection (used for destination ip/port) piped into // it) and the method name of the Service being called server side and populates // an rpcData struct ready to be passed to the RBAC Engine to find a matching // policy. -func newRPCData(data Data) (*rpcData, error) { +func newRPCData(ctx context.Context, methodName string) (*rpcData, error) { // The caller should populate all of these fields (i.e. for empty headers, // pipe an empty md into context). - md, ok := metadata.FromIncomingContext(data.Ctx) + md, ok := metadata.FromIncomingContext(ctx) if !ok { - return nil, errors.New("error retrieving metadata from incoming ctx") + return nil, errors.New("missing metadata in incoming context") } - pi, ok := peer.FromContext(data.Ctx) + pi, ok := peer.FromContext(ctx) if !ok { - return nil, errors.New("error retrieving peer info from incoming ctx") + return nil, errors.New("missing peer info in incoming context") } // The connection is needed in order to find the destination address and // port of the incoming RPC Call. - conn := getConnection(data.Ctx) + conn := getConnection(ctx) _, dPort, err := net.SplitHostPort(conn.LocalAddr().String()) if err != nil { return nil, err @@ -192,7 +181,7 @@ func newRPCData(data Data) (*rpcData, error) { return &rpcData{ md: md, peerInfo: pi, - fullMethod: data.MethodName, + fullMethod: methodName, destinationPort: uint32(dp), destinationAddr: conn.LocalAddr(), certs: peerCertificates, @@ -213,21 +202,20 @@ type rpcData struct { destinationPort uint32 // destinationAddr is the address that the RPC is being sent to. destinationAddr net.Addr - // certs will be used for authenticated matcher. + // certs are the certificates presented by the peer during a TLS + // handshake. certs []*x509.Certificate } -var errPolicyNotFound = errors.New("a matching policy was not found") +type connectionKey struct{} -// findMatchingPolicy determines if an incoming RPC matches a policy. On a -// successful match, it returns the name of the matching policy and a nil error -// to specify that there was a matching policy found. It returns an error in -// the case of not finding a matching policy. -func (r *engine) findMatchingPolicy(rpcData *rpcData) (string, error) { - for policy, matcher := range r.policies { - if matcher.match(rpcData) { - return policy, nil - } - } - return "", errPolicyNotFound +func getConnection(ctx context.Context) net.Conn { + conn, _ := ctx.Value(connectionKey{}).(net.Conn) + return conn +} + +// SetConnection adds the connection to the context to be able to get +// information about the destination ip and port for an incoming RPC. +func SetConnection(ctx context.Context, conn net.Conn) context.Context { + return context.WithValue(ctx, connectionKey{}, conn) } diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index 2bd39f805ed6..3b0330dc2444 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -55,12 +55,12 @@ type addr struct { func (addr) Network() string { return "" } func (a *addr) String() string { return a.ipAddress } -// TestChainedRBACEngineConstruction tests the construction of the -// ChainedRBACEngine. Due to some types of RBAC configuration being logically -// wrong and returning an error rather than successfully constructing the RBAC -// Engine, this test tests both RBAC Configurations deemed successful and also -// RBAC Configurations that will raise errors. -func (s) TestChainedRBACEngineConstruction(t *testing.T) { +// TestChainEngineConstruction tests the construction of the ChainEngine. Due to +// some types of RBAC configuration being logically wrong and returning an error +// rather than successfully constructing the RBAC Engine, this test tests both +// RBAC Configurations deemed successful and also RBAC Configurations that will +// raise errors. +func (s) TestChainEngineConstruction(t *testing.T) { tests := []struct { name string rbacConfigs []*v3rbacpb.RBAC @@ -421,12 +421,12 @@ func (s) TestChainedRBACEngineConstruction(t *testing.T) { } } -// TestChainedRBACEngine tests the chain of RBAC Engines by configuring the -// chain of engines in a certain way in different scenarios. After configuring -// the chain of engines in a certain way, this test pings the chain of engines -// with different types of data representing incoming RPC's (piped into a -// context), and verifies that it works as expected. -func (s) TestChainedRBACEngine(t *testing.T) { +// TestChainEngine tests the chain of RBAC Engines by configuring the chain of +// engines in a certain way in different scenarios. After configuring the chain +// of engines in a certain way, this test pings the chain of engines with +// different types of data representing incoming RPC's (piped into a context), +// and verifies that it works as expected. +func (s) TestChainEngine(t *testing.T) { tests := []struct { name string rbacConfigs []*v3rbacpb.RBAC @@ -869,7 +869,7 @@ func (s) TestChainedRBACEngine(t *testing.T) { // Make a TCP connection with a certain destination port. The // address/port of this connection will be used to populate the // destination ip/port in RPCData struct. This represents what - // the user of ChainedRBACEngine will have to place into + // the user of ChainEngine will have to place into // context, as this is only way to get destination ip and port. lis, err := net.Listen("tcp", "localhost:"+fmt.Sprint(data.rpcData.destinationPort)) if err != nil { @@ -888,25 +888,16 @@ func (s) TestChainedRBACEngine(t *testing.T) { conn := <-connCh ctx = SetConnection(ctx, conn) ctx = peer.NewContext(ctx, data.rpcData.peerInfo) - err = cre.DetermineStatus(Data{ - Ctx: ctx, - MethodName: data.rpcData.fullMethod, - }) - // TODO: I think a nil status represents a OK code, perhaps return a nil error instead? + err = cre.IsAuthorized(ctx, data.rpcData.fullMethod) status, _ := status.FromError(err) if data.wantStatusCode != status.Code() { - t.Fatalf("DetermineStatus(%+v, %+v) returned (%+v), want(%+v)", ctx, data.rpcData.fullMethod, status.Code(), data.wantStatusCode) + t.Fatalf("IsAuthorized(%+v, %+v) returned (%+v), want(%+v)", ctx, data.rpcData.fullMethod, status.Code(), data.wantStatusCode) } + conn.Close() lis.Close() } }) } } - -// Task list: Fix Success Case envoy example (principal name logic) -// Scale up tests to allow + deny policy -// Cleanup, send out for PR :) - -// After writing all these tests, also need to check code coverage, especially for authenticated stuff From 8b6ee2f1a3fcd55a5cdf4bd100a106ac263a9e86 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Thu, 8 Jul 2021 11:48:08 -0400 Subject: [PATCH 21/36] Missed one --- internal/xds/rbac/rbac_engine.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index a319ca2d140e..97bc5adbf13c 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -160,7 +160,10 @@ func newRPCData(ctx context.Context, methodName string) (*rpcData, error) { // The connection is needed in order to find the destination address and // port of the incoming RPC Call. - conn := getConnection(ctx) + conn, ok := getConnection(ctx) + if !ok { + return nil, errors.New("missing connection in incoming context") + } _, dPort, err := net.SplitHostPort(conn.LocalAddr().String()) if err != nil { return nil, err @@ -209,9 +212,9 @@ type rpcData struct { type connectionKey struct{} -func getConnection(ctx context.Context) net.Conn { - conn, _ := ctx.Value(connectionKey{}).(net.Conn) - return conn +func getConnection(ctx context.Context) (net.Conn, bool) { + conn, ok := ctx.Value(connectionKey{}).(net.Conn) + return conn, ok } // SetConnection adds the connection to the context to be able to get From e5c1f302f6940d9cadc83cbe54b30bdbd187c710 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Fri, 9 Jul 2021 11:56:35 -0400 Subject: [PATCH 22/36] Responded to Easwar's comments --- internal/xds/rbac/matchers.go | 11 +-- internal/xds/rbac/rbac_engine.go | 1 - internal/xds/rbac/rbac_engine_test.go | 113 ++++++++++++++------------ 3 files changed, 69 insertions(+), 56 deletions(-) diff --git a/internal/xds/rbac/matchers.go b/internal/xds/rbac/matchers.go index d44232652aeb..e6a868f65c47 100644 --- a/internal/xds/rbac/matchers.go +++ b/internal/xds/rbac/matchers.go @@ -388,14 +388,15 @@ func newAuthenticatedMatcher(authenticatedMatcherConfig *v3rbacpb.Principal_Auth func (am *authenticatedMatcher) match(data *rpcData) bool { // Represents this line in the RBAC documentation = "If unset, it applies to - // any user that is authenticated" (see package-level comments). - // Thus, if a user is authenticated, the user should match. An authenticated - // user will have a certificate provided. + // any user that is authenticated" (see package-level comments). An + // authenticated downstream in a stateful TLS connection will have to + // provide a certificate to prove their identity. Thus, you can simply check + // if there is a certificate present. if am.stringMatcher == nil { - // TODO: Is this correct? If a TLS Certificate is provided, that certificate means - // that that individual was logically authenticated with a key. if len(data.certs) != 0 { return true + } else { + return false } } diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index 97bc5adbf13c..4c9783b58719 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -75,7 +75,6 @@ func (cre *ChainEngine) IsAuthorized(ctx context.Context, methodName string) err return status.Errorf(codes.PermissionDenied, "incoming RPC %v did not match an allow policy", rpcData) case engine.action == deny && err != errPolicyNotFound: return status.Errorf(codes.PermissionDenied, "incoming RPC %+v matched a deny policy", rpcData) - default: } } // If the incoming RPC gets through all of the engines successfully (i.e. diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index 3b0330dc2444..73ee7e2ee32d 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -55,20 +55,20 @@ type addr struct { func (addr) Network() string { return "" } func (a *addr) String() string { return a.ipAddress } -// TestChainEngineConstruction tests the construction of the ChainEngine. Due to -// some types of RBAC configuration being logically wrong and returning an error +// TestNewChainEngine tests the construction of the ChainEngine. Due to some +// types of RBAC configuration being logically wrong and returning an error // rather than successfully constructing the RBAC Engine, this test tests both // RBAC Configurations deemed successful and also RBAC Configurations that will // raise errors. -func (s) TestChainEngineConstruction(t *testing.T) { +func (s) TestNewChainEngine(t *testing.T) { tests := []struct { - name string - rbacConfigs []*v3rbacpb.RBAC - wantErr bool + name string + policies []*v3rbacpb.RBAC + wantErr bool }{ { - name: "TestSuccessCaseAnyMatchSingular", - rbacConfigs: []*v3rbacpb.RBAC{ + name: "SuccessCaseAnyMatchSingular", + policies: []*v3rbacpb.RBAC{ { Action: v3rbacpb.RBAC_ALLOW, Policies: map[string]*v3rbacpb.Policy{ @@ -85,8 +85,8 @@ func (s) TestChainEngineConstruction(t *testing.T) { }, }, { - name: "TestSuccessCaseAnyMatchMultiple", - rbacConfigs: []*v3rbacpb.RBAC{ + name: "SuccessCaseAnyMatchMultiple", + policies: []*v3rbacpb.RBAC{ { Action: v3rbacpb.RBAC_ALLOW, Policies: map[string]*v3rbacpb.Policy{ @@ -116,8 +116,8 @@ func (s) TestChainEngineConstruction(t *testing.T) { }, }, { - name: "TestSuccessCaseSimplePolicySingular", - rbacConfigs: []*v3rbacpb.RBAC{ + name: "SuccessCaseSimplePolicySingular", + policies: []*v3rbacpb.RBAC{ { Action: v3rbacpb.RBAC_ALLOW, Policies: map[string]*v3rbacpb.Policy{ @@ -134,9 +134,14 @@ func (s) TestChainEngineConstruction(t *testing.T) { }, }, }, + // SuccessCaseSimplePolicyTwoPolicies tests the construction of the + // chained engines in the case where there are two policies in a list, + // one with an allow policy and one with a deny policy. A situation + // where two policies (allow and deny) is a very common use case for + // this API, and should successfully build. { - name: "TestSuccessCaseSimplePolicyMultiple", - rbacConfigs: []*v3rbacpb.RBAC{ + name: "SuccessCaseSimplePolicyTwoPolicies", + policies: []*v3rbacpb.RBAC{ { Action: v3rbacpb.RBAC_ALLOW, Policies: map[string]*v3rbacpb.Policy{ @@ -168,8 +173,8 @@ func (s) TestChainEngineConstruction(t *testing.T) { }, }, { - name: "TestSuccessCaseEnvoyExampleSingular", - rbacConfigs: []*v3rbacpb.RBAC{ + name: "SuccessCaseEnvoyExampleSingular", + policies: []*v3rbacpb.RBAC{ { Action: v3rbacpb.RBAC_ALLOW, Policies: map[string]*v3rbacpb.Policy{ @@ -193,7 +198,9 @@ func (s) TestChainEngineConstruction(t *testing.T) { {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 80}}, {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 443}}, }, - }}}, + }, + }, + }, }, }, }, @@ -208,8 +215,8 @@ func (s) TestChainEngineConstruction(t *testing.T) { }, }, { - name: "TestSourceIpMatcherSuccessSingular", - rbacConfigs: []*v3rbacpb.RBAC{ + name: "SourceIpMatcherSuccessSingular", + policies: []*v3rbacpb.RBAC{ { Action: v3rbacpb.RBAC_ALLOW, Policies: map[string]*v3rbacpb.Policy{ @@ -226,8 +233,8 @@ func (s) TestChainEngineConstruction(t *testing.T) { }, }, { - name: "TestSourceIpMatcherFailureSingular", - rbacConfigs: []*v3rbacpb.RBAC{ + name: "SourceIpMatcherFailureSingular", + policies: []*v3rbacpb.RBAC{ { Action: v3rbacpb.RBAC_ALLOW, Policies: map[string]*v3rbacpb.Policy{ @@ -245,8 +252,8 @@ func (s) TestChainEngineConstruction(t *testing.T) { wantErr: true, }, { - name: "TestDestinationIpMatcherSuccess", - rbacConfigs: []*v3rbacpb.RBAC{ + name: "DestinationIpMatcherSuccess", + policies: []*v3rbacpb.RBAC{ { Action: v3rbacpb.RBAC_ALLOW, Policies: map[string]*v3rbacpb.Policy{ @@ -263,8 +270,8 @@ func (s) TestChainEngineConstruction(t *testing.T) { }, }, { - name: "TestDestinationIpMatcherFailure", - rbacConfigs: []*v3rbacpb.RBAC{ + name: "DestinationIpMatcherFailure", + policies: []*v3rbacpb.RBAC{ { Action: v3rbacpb.RBAC_ALLOW, Policies: map[string]*v3rbacpb.Policy{ @@ -282,8 +289,8 @@ func (s) TestChainEngineConstruction(t *testing.T) { wantErr: true, }, { - name: "TestMatcherToNotPolicy", - rbacConfigs: []*v3rbacpb.RBAC{ + name: "MatcherToNotPolicy", + policies: []*v3rbacpb.RBAC{ { Action: v3rbacpb.RBAC_ALLOW, Policies: map[string]*v3rbacpb.Policy{ @@ -300,8 +307,8 @@ func (s) TestChainEngineConstruction(t *testing.T) { }, }, { - name: "TestMatcherToNotPrinicipal", - rbacConfigs: []*v3rbacpb.RBAC{ + name: "MatcherToNotPrinicipal", + policies: []*v3rbacpb.RBAC{ { Action: v3rbacpb.RBAC_ALLOW, Policies: map[string]*v3rbacpb.Policy{ @@ -317,9 +324,12 @@ func (s) TestChainEngineConstruction(t *testing.T) { }, }, }, + // PrinicpalProductViewer tests the construction of a chained engine + // with a policy that allows any downstream to send a GET request on a + // certain path. { - name: "TestPrincipalProductViewer", - rbacConfigs: []*v3rbacpb.RBAC{ + name: "PrincipalProductViewer", + policies: []*v3rbacpb.RBAC{ { Action: v3rbacpb.RBAC_ALLOW, Policies: map[string]*v3rbacpb.Policy{ @@ -345,9 +355,12 @@ func (s) TestChainEngineConstruction(t *testing.T) { }, }, }, + // Certain Headers tests the construction of a chained engine with a + // policy that allows any downstream to send an HTTP request with + // certain headers. { - name: "TestCertainHeaders", - rbacConfigs: []*v3rbacpb.RBAC{ + name: "CertainHeaders", + policies: []*v3rbacpb.RBAC{ { Policies: map[string]*v3rbacpb.Policy{ "certain-headers": { @@ -376,8 +389,8 @@ func (s) TestChainEngineConstruction(t *testing.T) { }, }, { - name: "TestLogAction", - rbacConfigs: []*v3rbacpb.RBAC{ + name: "LogAction", + policies: []*v3rbacpb.RBAC{ { Action: v3rbacpb.RBAC_LOG, Policies: map[string]*v3rbacpb.Policy{ @@ -395,8 +408,8 @@ func (s) TestChainEngineConstruction(t *testing.T) { wantErr: true, }, { - name: "TestActionNotSpecified", - rbacConfigs: []*v3rbacpb.RBAC{ + name: "ActionNotSpecified", + policies: []*v3rbacpb.RBAC{ { Policies: map[string]*v3rbacpb.Policy{ "anyone": { @@ -414,8 +427,8 @@ func (s) TestChainEngineConstruction(t *testing.T) { } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - if _, err := NewChainEngine(test.rbacConfigs); (err != nil) != test.wantErr { - t.Fatalf("NewChainEngine(%+v) returned err: %v, wantErr: %v", test.rbacConfigs, err, test.wantErr) + if _, err := NewChainEngine(test.policies); (err != nil) != test.wantErr { + t.Fatalf("NewChainEngine(%+v) returned err: %v, wantErr: %v", test.policies, err, test.wantErr) } }) } @@ -435,12 +448,12 @@ func (s) TestChainEngine(t *testing.T) { wantStatusCode codes.Code } }{ - // TestSuccessCaseAnyMatch tests a single RBAC Engine instantiated with + // SuccessCaseAnyMatch tests a single RBAC Engine instantiated with // a config with a policy with any rules for both permissions and // principals, meaning that any data about incoming RPC's that the RBAC // Engine is queried with should match that policy. { - name: "TestSuccessCaseAnyMatch", + name: "SuccessCaseAnyMatch", rbacConfigs: []*v3rbacpb.RBAC{ { Policies: map[string]*v3rbacpb.Policy{ @@ -470,11 +483,11 @@ func (s) TestChainEngine(t *testing.T) { }, }, }, - // TestSuccessCaseSimplePolicy is a test that tests a single policy + // SuccessCaseSimplePolicy is a test that tests a single policy // that only allows an rpc to proceed if the rpc is calling with a certain // path and port. { - name: "TestSuccessCaseSimplePolicy", + name: "SuccessCaseSimplePolicy", rbacConfigs: []*v3rbacpb.RBAC{ { Policies: map[string]*v3rbacpb.Policy{ @@ -522,13 +535,13 @@ func (s) TestChainEngine(t *testing.T) { }, }, }, - // TestSuccessCaseEnvoyExample is a test based on the example provided + // SuccessCaseEnvoyExample is a test based on the example provided // in the EnvoyProxy docs. The RBAC Config contains two policies, // service admin and product viewer, that provides an example of a real // RBAC Config that might be configured for a given for a given backend // service. { - name: "TestSuccessCaseEnvoyExample", + name: "SuccessCaseEnvoyExample", rbacConfigs: []*v3rbacpb.RBAC{ { Policies: map[string]*v3rbacpb.Policy{ @@ -638,7 +651,7 @@ func (s) TestChainEngine(t *testing.T) { }, }, { - name: "TestNotMatcher", + name: "NotMatcher", rbacConfigs: []*v3rbacpb.RBAC{ { Policies: map[string]*v3rbacpb.Policy{ @@ -684,7 +697,7 @@ func (s) TestChainEngine(t *testing.T) { }, }, { - name: "TestSourceIpMatcher", + name: "SourceIpMatcher", rbacConfigs: []*v3rbacpb.RBAC{ { Policies: map[string]*v3rbacpb.Policy{ @@ -724,7 +737,7 @@ func (s) TestChainEngine(t *testing.T) { }, }, { - name: "TestDestinationIpMatcher", + name: "DestinationIpMatcher", rbacConfigs: []*v3rbacpb.RBAC{ { Policies: map[string]*v3rbacpb.Policy{ @@ -756,12 +769,12 @@ func (s) TestChainEngine(t *testing.T) { }, }, }, - // TestAllowAndDenyPolicy tests a policy with an allow (on path) and + // AllowAndDenyPolicy tests a policy with an allow (on path) and // deny (on port) policy chained together. This represents how a user // configured interceptor would use this, and also is a potential // configuration for a dynamic xds interceptor. { - name: "TestAllowAndDenyPolicy", + name: "AllowAndDenyPolicy", rbacConfigs: []*v3rbacpb.RBAC{ { Policies: map[string]*v3rbacpb.Policy{ From 570c834ee6f5d7589c079aab7e39093eebd8a8d3 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Fri, 9 Jul 2021 11:58:26 -0400 Subject: [PATCH 23/36] Vet --- internal/xds/rbac/matchers.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/xds/rbac/matchers.go b/internal/xds/rbac/matchers.go index e6a868f65c47..577debd584d9 100644 --- a/internal/xds/rbac/matchers.go +++ b/internal/xds/rbac/matchers.go @@ -395,9 +395,8 @@ func (am *authenticatedMatcher) match(data *rpcData) bool { if am.stringMatcher == nil { if len(data.certs) != 0 { return true - } else { - return false } + return false } // The order of matching as per the RBAC documentation (see package-level comments) From 6ff22164c489df0e7949554927b7b3f47e8846c6 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Fri, 9 Jul 2021 12:00:28 -0400 Subject: [PATCH 24/36] Vet --- internal/xds/rbac/matchers.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/internal/xds/rbac/matchers.go b/internal/xds/rbac/matchers.go index 577debd584d9..70c50486c9bc 100644 --- a/internal/xds/rbac/matchers.go +++ b/internal/xds/rbac/matchers.go @@ -393,10 +393,7 @@ func (am *authenticatedMatcher) match(data *rpcData) bool { // provide a certificate to prove their identity. Thus, you can simply check // if there is a certificate present. if am.stringMatcher == nil { - if len(data.certs) != 0 { - return true - } - return false + return len(data.certs) != 0 } // The order of matching as per the RBAC documentation (see package-level comments) From 6e2574762da2cdcd6908750d91c94c714a558a94 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Fri, 9 Jul 2021 12:36:27 -0400 Subject: [PATCH 25/36] Changed API --- internal/xds/rbac/rbac_engine.go | 21 +++++++++++++++------ internal/xds/rbac/rbac_engine_test.go | 27 ++++++++++++++++++++++++++- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index 4c9783b58719..de38f8c01a81 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -28,6 +28,7 @@ import ( "strconv" v3rbacpb "github.com/envoyproxy/go-control-plane/envoy/config/rbac/v3" + "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/credentials" "google.golang.org/grpc/metadata" @@ -59,10 +60,10 @@ func NewChainEngine(policies []*v3rbacpb.RBAC) (*ChainEngine, error) { // engines and their associated actions. // // Errors returned by this function are compatible with the status package. -func (cre *ChainEngine) IsAuthorized(ctx context.Context, methodName string) error { - // This conversion step (i.e. pulling things out of generic ctx) can be done - // once, and then be used for the whole chain of RBAC Engines. - rpcData, err := newRPCData(ctx, methodName) +func (cre *ChainEngine) IsAuthorized(ctx context.Context) error { + // This conversion step (i.e. pulling things out of ctx) can be done once, + // and then be used for the whole chain of RBAC Engines. + rpcData, err := newRPCData(ctx) if err != nil { return status.Errorf(codes.InvalidArgument, "missing fields in ctx %+v: %v", ctx, err) } @@ -144,7 +145,7 @@ func (r *engine) findMatchingPolicy(rpcData *rpcData) (string, error) { // it) and the method name of the Service being called server side and populates // an rpcData struct ready to be passed to the RBAC Engine to find a matching // policy. -func newRPCData(ctx context.Context, methodName string) (*rpcData, error) { +func newRPCData(ctx context.Context) (*rpcData, error) { // The caller should populate all of these fields (i.e. for empty headers, // pipe an empty md into context). md, ok := metadata.FromIncomingContext(ctx) @@ -157,6 +158,14 @@ func newRPCData(ctx context.Context, methodName string) (*rpcData, error) { return nil, errors.New("missing peer info in incoming context") } + // The methodName will be available in the passed in ctx from a unary or streaming + // interceptor, as grpc.Server pipes in a transport stream which contains the methodName + // into contexts available in both unary or streaming interceptors. + mn, ok := grpc.Method(ctx) + if !ok { + return nil, errors.New("missing method in incoming context") + } + // The connection is needed in order to find the destination address and // port of the incoming RPC Call. conn, ok := getConnection(ctx) @@ -183,7 +192,7 @@ func newRPCData(ctx context.Context, methodName string) (*rpcData, error) { return &rpcData{ md: md, peerInfo: pi, - fullMethod: methodName, + fullMethod: mn, destinationPort: uint32(dp), destinationAddr: conn.LocalAddr(), certs: peerCertificates, diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index 73ee7e2ee32d..cd4e130b4dd0 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -22,6 +22,7 @@ import ( "crypto/x509" "crypto/x509/pkix" "fmt" + "google.golang.org/grpc" "net" "net/url" "testing" @@ -901,8 +902,12 @@ func (s) TestChainEngine(t *testing.T) { conn := <-connCh ctx = SetConnection(ctx, conn) ctx = peer.NewContext(ctx, data.rpcData.peerInfo) + stream := &ServerTransportStreamWithMethod{ + method: data.rpcData.fullMethod, + } - err = cre.IsAuthorized(ctx, data.rpcData.fullMethod) + ctx = grpc.NewContextWithServerTransportStream(ctx, stream) + err = cre.IsAuthorized(ctx) status, _ := status.FromError(err) if data.wantStatusCode != status.Code() { t.Fatalf("IsAuthorized(%+v, %+v) returned (%+v), want(%+v)", ctx, data.rpcData.fullMethod, status.Code(), data.wantStatusCode) @@ -914,3 +919,23 @@ func (s) TestChainEngine(t *testing.T) { }) } } + +type ServerTransportStreamWithMethod struct { + method string +} + +func (sts *ServerTransportStreamWithMethod) Method() string { + return sts.method +} + +func (sts *ServerTransportStreamWithMethod) SetHeader(md metadata.MD) error { + return nil +} + +func (sts *ServerTransportStreamWithMethod) SendHeader(md metadata.MD) error { + return nil +} + +func (sts *ServerTransportStreamWithMethod) SetTrailer(md metadata.MD) error { + return nil +} From 21d0121a1723daddd1cfda4d2a47bcb8b2b46365 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Fri, 9 Jul 2021 12:38:18 -0400 Subject: [PATCH 26/36] Vet --- internal/xds/rbac/rbac_engine_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index cd4e130b4dd0..01b2fb0c2bcd 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -22,7 +22,6 @@ import ( "crypto/x509" "crypto/x509/pkix" "fmt" - "google.golang.org/grpc" "net" "net/url" "testing" @@ -33,6 +32,7 @@ import ( v3matcherpb "github.com/envoyproxy/go-control-plane/envoy/type/matcher/v3" v3typepb "github.com/envoyproxy/go-control-plane/envoy/type/v3" wrapperspb "github.com/golang/protobuf/ptypes/wrappers" + "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/credentials" "google.golang.org/grpc/internal/grpctest" From 3279ee57c933f564bfc3294916178b122efbf60f Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Fri, 16 Jul 2021 11:22:10 -0400 Subject: [PATCH 27/36] Responded to Doug's comments --- internal/xds/rbac/rbac_engine.go | 49 ++++++++++++++------------- internal/xds/rbac/rbac_engine_test.go | 7 ++-- 2 files changed, 29 insertions(+), 27 deletions(-) diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index de38f8c01a81..21c1e8fb2d14 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -24,6 +24,7 @@ import ( "context" "crypto/x509" "errors" + "fmt" "net" "strconv" @@ -69,13 +70,13 @@ func (cre *ChainEngine) IsAuthorized(ctx context.Context) error { } for _, engine := range cre.chainedEngines { // TODO: What do I do now with this matchingPolicyName? - _, err := engine.findMatchingPolicy(rpcData) + matchingPolicyName, matchingPolicy := engine.findMatchingPolicy(rpcData) switch { - case engine.action == allow && err == errPolicyNotFound: - return status.Errorf(codes.PermissionDenied, "incoming RPC %v did not match an allow policy", rpcData) - case engine.action == deny && err != errPolicyNotFound: - return status.Errorf(codes.PermissionDenied, "incoming RPC %+v matched a deny policy", rpcData) + case engine.action == allow && !matchingPolicy: + return status.Errorf(codes.PermissionDenied, "incoming RPC did not match an allow policy") + case engine.action == deny && matchingPolicy: + return status.Errorf(codes.PermissionDenied, "incoming RPC matched a deny policy %v", matchingPolicyName) } } // If the incoming RPC gets through all of the engines successfully (i.e. @@ -100,14 +101,14 @@ type engine struct { // newEngine creates an RBAC Engine based on the contents of policy. Returns a // non-nil error if the policy is invalid. func newEngine(policy *v3rbacpb.RBAC) (*engine, error) { - var action action + var a action switch *policy.Action.Enum() { case v3rbacpb.RBAC_ALLOW: - action = allow + a = allow case v3rbacpb.RBAC_DENY: - action = deny - case v3rbacpb.RBAC_LOG: - return nil, errors.New("unsupported action") + a = deny + default: + return nil, fmt.Errorf("unsupported action %s", policy.Action) } policies := make(map[string]*policyMatcher, len(policy.Policies)) @@ -120,26 +121,26 @@ func newEngine(policy *v3rbacpb.RBAC) (*engine, error) { } return &engine{ policies: policies, - action: action, + action: a, }, nil } var errPolicyNotFound = errors.New("a matching policy was not found") // findMatchingPolicy determines if an incoming RPC matches a policy. On a -// successful match, it returns the name of the matching policy and a nil error -// to specify that there was a matching policy found. It returns an error in +// successful match, it returns the name of the matching policy and a true bool +// to specify that there was a matching policy found. It returns false in // the case of not finding a matching policy. -func (r *engine) findMatchingPolicy(rpcData *rpcData) (string, error) { +func (r *engine) findMatchingPolicy(rpcData *rpcData) (string, bool) { for policy, matcher := range r.policies { if matcher.match(rpcData) { - return policy, nil + return policy, true } } - return "", errPolicyNotFound + return "", false } -// newRPCData takes a incoming context (should be a context representing state +// newRPCData takes an incoming context (should be a context representing state // needed for server RPC Call with metadata, peer info (used for source ip/port // and TLS information) and connection (used for destination ip/port) piped into // it) and the method name of the Service being called server side and populates @@ -168,17 +169,17 @@ func newRPCData(ctx context.Context) (*rpcData, error) { // The connection is needed in order to find the destination address and // port of the incoming RPC Call. - conn, ok := getConnection(ctx) - if !ok { + conn := getConnection(ctx) + if conn == nil { return nil, errors.New("missing connection in incoming context") } _, dPort, err := net.SplitHostPort(conn.LocalAddr().String()) if err != nil { - return nil, err + return nil, fmt.Errorf("error parsing local address: %v", err) } dp, err := strconv.ParseUint(dPort, 10, 32) if err != nil { - return nil, err + return nil, fmt.Errorf("error parsing local address: %v", err) } var peerCertificates []*x509.Certificate @@ -220,9 +221,9 @@ type rpcData struct { type connectionKey struct{} -func getConnection(ctx context.Context) (net.Conn, bool) { - conn, ok := ctx.Value(connectionKey{}).(net.Conn) - return conn, ok +func getConnection(ctx context.Context) net.Conn { + conn, _ := ctx.Value(connectionKey{}).(net.Conn) + return conn } // SetConnection adds the connection to the context to be able to get diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index 01b2fb0c2bcd..6bbc1722f2e8 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -908,9 +908,10 @@ func (s) TestChainEngine(t *testing.T) { ctx = grpc.NewContextWithServerTransportStream(ctx, stream) err = cre.IsAuthorized(ctx) - status, _ := status.FromError(err) - if data.wantStatusCode != status.Code() { - t.Fatalf("IsAuthorized(%+v, %+v) returned (%+v), want(%+v)", ctx, data.rpcData.fullMethod, status.Code(), data.wantStatusCode) + if gotCode := status.Code(err); gotCode != data.wantStatusCode { + t.Fatalf("IsAuthorized(%+v, %+v) returned (%+v), want(%+v)", ctx, data.rpcData.fullMethod, gotCode, data.wantStatusCode) + conn.Close() + lis.Close() } conn.Close() From f86087efde1be3070ce254b50d62f0e0822c5f67 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Fri, 16 Jul 2021 11:42:22 -0400 Subject: [PATCH 28/36] Responded to Ashitha's comment --- internal/xds/rbac/matchers.go | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/internal/xds/rbac/matchers.go b/internal/xds/rbac/matchers.go index 70c50486c9bc..8ac93222547f 100644 --- a/internal/xds/rbac/matchers.go +++ b/internal/xds/rbac/matchers.go @@ -395,27 +395,25 @@ func (am *authenticatedMatcher) match(data *rpcData) bool { if am.stringMatcher == nil { return len(data.certs) != 0 } - + // No certificate present, so will never successfully match. + if len(data.certs) == 0 { + return false + } + cert := data.certs[0] // The order of matching as per the RBAC documentation (see package-level comments) // is as follows: URI SANs, DNS SANs, and then subject name. - for _, cert := range data.certs { - for _, uriSAN := range cert.URIs { - if am.stringMatcher.Match(uriSAN.String()) { + for _, uriSAN := range cert.URIs { + if am.stringMatcher.Match(uriSAN.String()) { return true } - } } - for _, cert := range data.certs { - for _, dnsSAN := range cert.DNSNames { - if am.stringMatcher.Match(dnsSAN) { + for _, dnsSAN := range cert.DNSNames { + if am.stringMatcher.Match(dnsSAN) { return true } - } } - for _, cert := range data.certs { - if am.stringMatcher.Match(cert.Subject.String()) { + if am.stringMatcher.Match(cert.Subject.String()) { return true - } } return false } From abc2d778be660786286824f1443bf5b8b5171d64 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Fri, 16 Jul 2021 11:44:25 -0400 Subject: [PATCH 29/36] Vet --- internal/xds/rbac/matchers.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/xds/rbac/matchers.go b/internal/xds/rbac/matchers.go index 8ac93222547f..0d538d4e15d9 100644 --- a/internal/xds/rbac/matchers.go +++ b/internal/xds/rbac/matchers.go @@ -404,16 +404,16 @@ func (am *authenticatedMatcher) match(data *rpcData) bool { // is as follows: URI SANs, DNS SANs, and then subject name. for _, uriSAN := range cert.URIs { if am.stringMatcher.Match(uriSAN.String()) { - return true - } + return true + } } for _, dnsSAN := range cert.DNSNames { if am.stringMatcher.Match(dnsSAN) { - return true - } + return true + } } if am.stringMatcher.Match(cert.Subject.String()) { - return true + return true } return false } From 3b55e0fbd590de9bb8206b4b42f5fb7738a6eff2 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Fri, 16 Jul 2021 11:47:32 -0400 Subject: [PATCH 30/36] Vet --- internal/xds/rbac/matchers.go | 5 +---- internal/xds/rbac/rbac_engine.go | 2 -- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/internal/xds/rbac/matchers.go b/internal/xds/rbac/matchers.go index 0d538d4e15d9..25d1cc0e8b98 100644 --- a/internal/xds/rbac/matchers.go +++ b/internal/xds/rbac/matchers.go @@ -412,8 +412,5 @@ func (am *authenticatedMatcher) match(data *rpcData) bool { return true } } - if am.stringMatcher.Match(cert.Subject.String()) { - return true - } - return false + return am.stringMatcher.Match(cert.Subject.String()) } diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index 21c1e8fb2d14..2031a415898f 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -125,8 +125,6 @@ func newEngine(policy *v3rbacpb.RBAC) (*engine, error) { }, nil } -var errPolicyNotFound = errors.New("a matching policy was not found") - // findMatchingPolicy determines if an incoming RPC matches a policy. On a // successful match, it returns the name of the matching policy and a true bool // to specify that there was a matching policy found. It returns false in From 0f87719dc66f09202998919f0fbca4eff68d8e1f Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Tue, 20 Jul 2021 12:11:25 -0400 Subject: [PATCH 31/36] Responded to all comments except lis port --- internal/xds/rbac/rbac_engine.go | 21 ++++---- internal/xds/rbac/rbac_engine_test.go | 78 ++++++++++++++------------- 2 files changed, 51 insertions(+), 48 deletions(-) diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index 2031a415898f..e94d6c382255 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -69,15 +69,16 @@ func (cre *ChainEngine) IsAuthorized(ctx context.Context) error { return status.Errorf(codes.InvalidArgument, "missing fields in ctx %+v: %v", ctx, err) } for _, engine := range cre.chainedEngines { - // TODO: What do I do now with this matchingPolicyName? - matchingPolicyName, matchingPolicy := engine.findMatchingPolicy(rpcData) + matchingPolicyName, ok := engine.findMatchingPolicy(rpcData) switch { - case engine.action == allow && !matchingPolicy: + case engine.action == allow && !ok: return status.Errorf(codes.PermissionDenied, "incoming RPC did not match an allow policy") - case engine.action == deny && matchingPolicy: - return status.Errorf(codes.PermissionDenied, "incoming RPC matched a deny policy %v", matchingPolicyName) + case engine.action == deny && ok: + return status.Errorf(codes.PermissionDenied, "incoming RPC matched a deny policy %q", matchingPolicyName) } + // Every policy in the engine list must be queried. Thus, iterate to the + // next policy. } // If the incoming RPC gets through all of the engines successfully (i.e. // doesn't not match an allow or match a deny engine), the RPC is authorized @@ -100,19 +101,19 @@ type engine struct { // newEngine creates an RBAC Engine based on the contents of policy. Returns a // non-nil error if the policy is invalid. -func newEngine(policy *v3rbacpb.RBAC) (*engine, error) { +func newEngine(config *v3rbacpb.RBAC) (*engine, error) { var a action - switch *policy.Action.Enum() { + switch *config.Action.Enum() { case v3rbacpb.RBAC_ALLOW: a = allow case v3rbacpb.RBAC_DENY: a = deny default: - return nil, fmt.Errorf("unsupported action %s", policy.Action) + return nil, fmt.Errorf("unsupported action %s", config.Action) } - policies := make(map[string]*policyMatcher, len(policy.Policies)) - for name, config := range policy.Policies { + policies := make(map[string]*policyMatcher, len(config.Policies)) + for name, config := range config.Policies { matcher, err := newPolicyMatcher(config) if err != nil { return nil, err diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index 6bbc1722f2e8..f8bb117e0a39 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -874,48 +874,50 @@ func (s) TestChainEngine(t *testing.T) { // Query the created chain of RBAC Engines with different args to see // if the chain of RBAC Engines configured as such works as intended. for _, data := range test.rbacQueries { - // Construct the context with three data points that have enough - // information to represent incoming RPC's. This will be how a - // user uses this API. A user will have to put MD, PeerInfo, and - // the connection the RPC is sent on in the context. - ctx := metadata.NewIncomingContext(context.Background(), data.rpcData.md) + func() { + // Construct the context with three data points that have enough + // information to represent incoming RPC's. This will be how a + // user uses this API. A user will have to put MD, PeerInfo, and + // the connection the RPC is sent on in the context. + ctx := metadata.NewIncomingContext(context.Background(), data.rpcData.md) - // Make a TCP connection with a certain destination port. The - // address/port of this connection will be used to populate the - // destination ip/port in RPCData struct. This represents what - // the user of ChainEngine will have to place into - // context, as this is only way to get destination ip and port. - lis, err := net.Listen("tcp", "localhost:"+fmt.Sprint(data.rpcData.destinationPort)) - if err != nil { - t.Fatalf("Error listening: %v", err) - } - connCh := make(chan net.Conn, 1) - go func() { - conn, err := lis.Accept() + // Make a TCP connection with a certain destination port. The + // address/port of this connection will be used to populate the + // destination ip/port in RPCData struct. This represents what + // the user of ChainEngine will have to place into + // context, as this is only way to get destination ip and port. + lis, err := net.Listen("tcp", "localhost:"+fmt.Sprint(data.rpcData.destinationPort)) + defer lis.Close() if err != nil { - t.Errorf("Error accepting connection: %v", err) - return + t.Fatalf("Error listening: %v", err) + } + connCh := make(chan net.Conn, 1) + go func() { + conn, err := lis.Accept() + if err != nil { + t.Errorf("Error accepting connection: %v", err) + return + } + connCh <- conn + }() + _, err = net.Dial("tcp", lis.Addr().String()) + if err != nil { + t.Fatalf("Error dialing: %v", err) + } + conn := <-connCh + defer conn.Close() + ctx = SetConnection(ctx, conn) + ctx = peer.NewContext(ctx, data.rpcData.peerInfo) + stream := &ServerTransportStreamWithMethod{ + method: data.rpcData.fullMethod, } - connCh <- conn - }() - net.Dial("tcp", lis.Addr().String()) - conn := <-connCh - ctx = SetConnection(ctx, conn) - ctx = peer.NewContext(ctx, data.rpcData.peerInfo) - stream := &ServerTransportStreamWithMethod{ - method: data.rpcData.fullMethod, - } - - ctx = grpc.NewContextWithServerTransportStream(ctx, stream) - err = cre.IsAuthorized(ctx) - if gotCode := status.Code(err); gotCode != data.wantStatusCode { - t.Fatalf("IsAuthorized(%+v, %+v) returned (%+v), want(%+v)", ctx, data.rpcData.fullMethod, gotCode, data.wantStatusCode) - conn.Close() - lis.Close() - } - conn.Close() - lis.Close() + ctx = grpc.NewContextWithServerTransportStream(ctx, stream) + err = cre.IsAuthorized(ctx) + if gotCode := status.Code(err); gotCode != data.wantStatusCode { + t.Fatalf("IsAuthorized(%+v, %+v) returned (%+v), want(%+v)", ctx, data.rpcData.fullMethod, gotCode, data.wantStatusCode) + } + }() } }) } From 79222565cedad6263d33c50935664f925ed6350c Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Tue, 20 Jul 2021 12:42:23 -0400 Subject: [PATCH 32/36] Listened on wildcard port --- internal/xds/rbac/rbac_engine_test.go | 34 ++++----------------------- 1 file changed, 5 insertions(+), 29 deletions(-) diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index f8bb117e0a39..1d9fa6f5b7b6 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -21,7 +21,6 @@ import ( "crypto/tls" "crypto/x509" "crypto/x509/pkix" - "fmt" "net" "net/url" "testing" @@ -486,7 +485,7 @@ func (s) TestChainEngine(t *testing.T) { }, // SuccessCaseSimplePolicy is a test that tests a single policy // that only allows an rpc to proceed if the rpc is calling with a certain - // path and port. + // path. { name: "SuccessCaseSimplePolicy", rbacConfigs: []*v3rbacpb.RBAC{ @@ -494,7 +493,6 @@ func (s) TestChainEngine(t *testing.T) { Policies: map[string]*v3rbacpb.Policy{ "localhost-fan": { Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 8080}}, {Rule: &v3rbacpb.Permission_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "localhost-fan-page"}}}}}}, }, Principals: []*v3rbacpb.Principal{ @@ -512,10 +510,7 @@ func (s) TestChainEngine(t *testing.T) { // this RPC should be allowed to proceed. { rpcData: &rpcData{ - md: map[string][]string{ - ":path": {"localhost-fan-page"}, - }, - destinationPort: 8080, + fullMethod: "localhost-fan-page", peerInfo: &peer.Peer{ Addr: &addr{ipAddress: "0.0.0.0"}, }, @@ -527,7 +522,6 @@ func (s) TestChainEngine(t *testing.T) { // this rpc shouldn't be allowed to proceed. { rpcData: &rpcData{ - destinationPort: 8000, peerInfo: &peer.Peer{ Addr: &addr{ipAddress: "0.0.0.0"}, }, @@ -562,12 +556,6 @@ func (s) TestChainEngine(t *testing.T) { Rules: []*v3rbacpb.Permission{ {Rule: &v3rbacpb.Permission_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_ExactMatch{ExactMatch: "GET"}}}}, {Rule: &v3rbacpb.Permission_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Prefix{Prefix: "/products"}}}}}}, - {Rule: &v3rbacpb.Permission_OrRules{OrRules: &v3rbacpb.Permission_Set{ - Rules: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 80}}, - {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 443}}, - }, - }}}, }, }, }, @@ -612,7 +600,6 @@ func (s) TestChainEngine(t *testing.T) { // These incoming RPC calls should not match any policy. { rpcData: &rpcData{ - destinationPort: 8000, peerInfo: &peer.Peer{ Addr: &addr{ipAddress: "0.0.0.0"}, }, @@ -621,7 +608,6 @@ func (s) TestChainEngine(t *testing.T) { }, { rpcData: &rpcData{ - destinationPort: 8080, fullMethod: "get-product-list", peerInfo: &peer.Peer{ Addr: &addr{ipAddress: "0.0.0.0"}, @@ -631,7 +617,6 @@ func (s) TestChainEngine(t *testing.T) { }, { rpcData: &rpcData{ - destinationPort: 8080, peerInfo: &peer.Peer{ Addr: &addr{ipAddress: "0.0.0.0"}, AuthInfo: credentials.TLSInfo{ @@ -794,7 +779,6 @@ func (s) TestChainEngine(t *testing.T) { Policies: map[string]*v3rbacpb.Policy{ "localhost-fan": { Permissions: []*v3rbacpb.Permission{ - {Rule: &v3rbacpb.Permission_DestinationPort{DestinationPort: 8080}}, {Rule: &v3rbacpb.Permission_UrlPath{UrlPath: &v3matcherpb.PathMatcher{Rule: &v3matcherpb.PathMatcher_Path{Path: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "localhost-fan-page"}}}}}}, }, Principals: []*v3rbacpb.Principal{ @@ -813,7 +797,6 @@ func (s) TestChainEngine(t *testing.T) { // match with the deny and thus should be allowed to proceed. { rpcData: &rpcData{ - destinationPort: 8000, peerInfo: &peer.Peer{ Addr: &addr{ipAddress: "0.0.0.0"}, }, @@ -824,10 +807,7 @@ func (s) TestChainEngine(t *testing.T) { // and thus shouldn't be allowed to proceed as matched with deny. { rpcData: &rpcData{ - md: map[string][]string{ - ":path": {"localhost-fan-page"}, - }, - destinationPort: 8080, + fullMethod: "localhost-fan-page", peerInfo: &peer.Peer{ Addr: &addr{ipAddress: "0.0.0.0"}, }, @@ -838,7 +818,6 @@ func (s) TestChainEngine(t *testing.T) { // shouldn't be allowed to proceed as didn't match with allow. { rpcData: &rpcData{ - destinationPort: 8000, peerInfo: &peer.Peer{ Addr: &addr{ipAddress: "10.0.0.0"}, }, @@ -849,10 +828,7 @@ func (s) TestChainEngine(t *testing.T) { // thus shouldn't be allowed to proceed. { rpcData: &rpcData{ - md: map[string][]string{ - ":path": {"localhost-fan-page"}, - }, - destinationPort: 8080, + fullMethod: "localhost-fan-page", peerInfo: &peer.Peer{ Addr: &addr{ipAddress: "10.0.0.0"}, }, @@ -886,7 +862,7 @@ func (s) TestChainEngine(t *testing.T) { // destination ip/port in RPCData struct. This represents what // the user of ChainEngine will have to place into // context, as this is only way to get destination ip and port. - lis, err := net.Listen("tcp", "localhost:"+fmt.Sprint(data.rpcData.destinationPort)) + lis, err := net.Listen("tcp", "localhost:0") defer lis.Close() if err != nil { t.Fatalf("Error listening: %v", err) From a35e19be13e3668118d305eaae7f528f67ae5225 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Tue, 20 Jul 2021 12:43:38 -0400 Subject: [PATCH 33/36] Vet --- internal/xds/rbac/rbac_engine_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index 1d9fa6f5b7b6..34577eebfdc3 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -608,7 +608,7 @@ func (s) TestChainEngine(t *testing.T) { }, { rpcData: &rpcData{ - fullMethod: "get-product-list", + fullMethod: "get-product-list", peerInfo: &peer.Peer{ Addr: &addr{ipAddress: "0.0.0.0"}, }, From 00b24c232763c1310c0575516c418a5c24eebc82 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Tue, 20 Jul 2021 12:44:46 -0400 Subject: [PATCH 34/36] Vet --- internal/xds/rbac/rbac_engine_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index 34577eebfdc3..2521ac4526aa 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -863,10 +863,10 @@ func (s) TestChainEngine(t *testing.T) { // the user of ChainEngine will have to place into // context, as this is only way to get destination ip and port. lis, err := net.Listen("tcp", "localhost:0") - defer lis.Close() if err != nil { t.Fatalf("Error listening: %v", err) } + defer lis.Close() connCh := make(chan net.Conn, 1) go func() { conn, err := lis.Accept() From 79fe8965a1ebba941a832a71cbbccc7a8fb0df7a Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 21 Jul 2021 21:57:51 -0400 Subject: [PATCH 35/36] Responded to Doug's comments --- internal/xds/rbac/rbac_engine.go | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index e94d6c382255..d8ce62e75bda 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -72,9 +72,9 @@ func (cre *ChainEngine) IsAuthorized(ctx context.Context) error { matchingPolicyName, ok := engine.findMatchingPolicy(rpcData) switch { - case engine.action == allow && !ok: + case engine.action == v3rbacpb.RBAC_ALLOW && !ok: return status.Errorf(codes.PermissionDenied, "incoming RPC did not match an allow policy") - case engine.action == deny && ok: + case engine.action == v3rbacpb.RBAC_DENY && ok: return status.Errorf(codes.PermissionDenied, "incoming RPC matched a deny policy %q", matchingPolicyName) } // Every policy in the engine list must be queried. Thus, iterate to the @@ -86,35 +86,24 @@ func (cre *ChainEngine) IsAuthorized(ctx context.Context) error { return status.Error(codes.OK, "") } -type action int - -const ( - allow action = iota - deny -) - // engine is used for matching incoming RPCs to policies. type engine struct { policies map[string]*policyMatcher - action action + // action must be ALLOW or DENY. + action v3rbacpb.RBAC_Action } // newEngine creates an RBAC Engine based on the contents of policy. Returns a // non-nil error if the policy is invalid. func newEngine(config *v3rbacpb.RBAC) (*engine, error) { - var a action - switch *config.Action.Enum() { - case v3rbacpb.RBAC_ALLOW: - a = allow - case v3rbacpb.RBAC_DENY: - a = deny - default: + a := *config.Action.Enum() + if a != v3rbacpb.RBAC_ALLOW && a != v3rbacpb.RBAC_DENY { return nil, fmt.Errorf("unsupported action %s", config.Action) } policies := make(map[string]*policyMatcher, len(config.Policies)) - for name, config := range config.Policies { - matcher, err := newPolicyMatcher(config) + for name, policy := range config.Policies { + matcher, err := newPolicyMatcher(policy) if err != nil { return nil, err } From e2e15cfa9faf27ac20ea84296867f6a17423ed36 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 21 Jul 2021 21:58:55 -0400 Subject: [PATCH 36/36] Vet --- internal/xds/rbac/rbac_engine.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index d8ce62e75bda..609d123c8039 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -90,7 +90,7 @@ func (cre *ChainEngine) IsAuthorized(ctx context.Context) error { type engine struct { policies map[string]*policyMatcher // action must be ALLOW or DENY. - action v3rbacpb.RBAC_Action + action v3rbacpb.RBAC_Action } // newEngine creates an RBAC Engine based on the contents of policy. Returns a