Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add extra layer on top of RBAC Engine #4576

Merged
merged 36 commits into from
Jul 22, 2021
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
6201082
Finished helper function
zasweq Jun 30, 2021
3345452
Vet
zasweq Jun 30, 2021
c6f6c7c
Vet
zasweq Jun 30, 2021
d2fdd85
Vet
zasweq Jun 30, 2021
31301be
Save progress
zasweq Jul 1, 2021
73d9c40
Save progress
zasweq Jul 1, 2021
11acf73
Save progress before cleanup
zasweq Jul 1, 2021
153fbb6
Cleanup
zasweq Jul 2, 2021
16002a5
More cleanup
zasweq Jul 2, 2021
72311a2
Save progress
zasweq Jul 2, 2021
d60b45b
Working instantiation test
zasweq Jul 2, 2021
cbd8112
Save progress
zasweq Jul 6, 2021
9ff3255
Save progress
zasweq Jul 6, 2021
4fe746b
Save progress, almost done
zasweq Jul 7, 2021
9ea7103
Clean up
zasweq Jul 7, 2021
ae0e62c
vet
zasweq Jul 7, 2021
8b9ebee
Vet
zasweq Jul 7, 2021
16273d7
Vet
zasweq Jul 7, 2021
0cb3455
Added Auth Info
zasweq Jul 7, 2021
2aad5fb
Responded to Easwar's comments
zasweq Jul 8, 2021
8b6ee2f
Missed one
zasweq Jul 8, 2021
e5c1f30
Responded to Easwar's comments
zasweq Jul 9, 2021
570c834
Vet
zasweq Jul 9, 2021
6ff2216
Vet
zasweq Jul 9, 2021
6e25747
Changed API
zasweq Jul 9, 2021
21d0121
Vet
zasweq Jul 9, 2021
3279ee5
Responded to Doug's comments
zasweq Jul 16, 2021
f86087e
Responded to Ashitha's comment
zasweq Jul 16, 2021
abc2d77
Vet
zasweq Jul 16, 2021
3b55e0f
Vet
zasweq Jul 16, 2021
0f87719
Responded to all comments except lis port
zasweq Jul 20, 2021
7922256
Listened on wildcard port
zasweq Jul 20, 2021
a35e19b
Vet
zasweq Jul 20, 2021
00b24c2
Vet
zasweq Jul 20, 2021
79fe896
Responded to Doug's comments
zasweq Jul 22, 2021
e2e15cf
Vet
zasweq Jul 22, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 59 additions & 21 deletions internal/xds/rbac/matchers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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
}

Expand All @@ -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)
}

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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 {
Expand All @@ -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
Expand All @@ -361,26 +361,64 @@ 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
// 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. authenticatedMatcher implements the matcher interface.
type authenticatedMatcher struct {
stringMatcher internalmatcher.StringMatcher
stringMatcher *internalmatcher.StringMatcher
}

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" (see package-level comments).
if authenticatedMatcherConfig.PrincipalName == nil {
return &authenticatedMatcher{}, nil
}
stringMatcher, err := internalmatcher.StringMatcherFromProto(authenticatedMatcherConfig.PrincipalName)
if err != nil {
return nil, err
}
return &authenticatedMatcher{stringMatcher: stringMatcher}, nil
}
return &authenticatedMatcher{stringMatcher: &stringMatcher}, nil
}

func (am *authenticatedMatcher) match(data *rpcData) bool {
// Represents this line in the RBAC documentation = "If unset, it applies to
easwars marked this conversation as resolved.
Show resolved Hide resolved
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm ... I don't think I understand this comment.
What is the relationship between a certificate provided and the user being authenticated or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is correct. https://github.com/envoyproxy/envoy/blob/0fae6970ddaf93f024908ba304bbd2b34e997a51/source/extensions/filters/common/rbac/matchers.cc#L166 simply checks for a connection that was authenticated. In a stateful TLS conneciton, the downstream will have to provide a cert to authenticate their identity. Thus, logically, if a certificate is present, it means the connection was authenticated. Reworded the comment.

if am.stringMatcher == nil {
// TODO: Is this correct? If a TLS Certificate is provided, that certificate means
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this question directed to one of the reviewers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, in case this encounters problems later, this might give an insight into why? Perhaps I should delete this comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you should try to get an answer to this question by raising it with whoever knows more about this topic, and have the TODO addressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this to be correct. Deleted this TODO.

// that that individual was logically authenticated with a key.
if len(data.certs) != 0 {
return true
}
}

func (am *authenticatedMatcher) match(data *RPCData) bool {
return am.stringMatcher.Match(data.PrincipalName)
// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we are looping through chain of certificates. But if you look through envoy code, the evaluation is done only with leaf certificate. We may want to update this accordingly.
https://source.corp.google.com/piper///depot/google3/third_party/envoy/src/source/extensions/transport_sockets/tls/ssl_handshaker.cc;rcl=384056464;l=170

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, similar to grpc-java: (https://github.com/grpc/grpc-java/blob/c8ba60152958cb0f37d0e9a32ae406ce8d5f1ff0/xds/src/main/java/io/grpc/xds/internal/rbac/engine/GrpcAuthorizationEngine.java#L280). Thanks for pointing this out, and sorry for the delay in answering. Envoy seems to persist a singular cert, where as in grpc we persist a list. Rather than loop through all the certificates, I only checked the first certificate.

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) {
return true
}
}
}
for _, cert := range data.certs {
if am.stringMatcher.Match(cert.Subject.String()) {
return true
}
}
return false
}
Loading