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

Check ACLs more often for xDS endpoints. #5237

Merged
merged 2 commits into from
Jan 29, 2019

Conversation

rboyer
Copy link
Member

@rboyer rboyer commented Jan 18, 2019

Currently for xDS endpoints we only check ACL permissions when the gRPC streaming call is initiated. As this is a stream it could have a long life and if the Token is deleted or the attached Policy rules change while the stream is active nothing will re-assert ACLs.

This change will now check ACL permissions on each incoming DiscoveryRequest or outgoing DiscoveryResponse as one would do for a more traditional direct (non-streaming) RPC.

In the event that a stream remains open and blocked but there is no request/response activity, after 5 minutes elapse the stream handler will unblock long enough to recheck ACL permissions.

If the Token has been deleted or the Policies provide insufficient privileges the server will terminate the stream.

@rboyer rboyer added the theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies label Jan 18, 2019
@rboyer rboyer requested a review from a team January 18, 2019 22:18
@hashicorp-cla
Copy link

hashicorp-cla commented Jan 18, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Some minor points inline but overall this looks good to me.

Did you test it against a real Envoy as a sanity check too? The TestEnvoy helper is useful but would be good to confirm Envoy's real behaviour against the mid-stream Auth failure!

agent/xds/server.go Show resolved Hide resolved
agent/xds/server.go Outdated Show resolved Hide resolved
Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

The code looks good. Just one question.

agent/xds/server.go Show resolved Hide resolved
@rboyer
Copy link
Member Author

rboyer commented Jan 22, 2019

Did you test it against a real Envoy as a sanity check too? The TestEnvoy helper is useful but would be good to confirm Envoy's real behaviour against the mid-stream Auth failure!

I actually wrote this against a real envoy setup first and wrote the test after the fact.

For established xDS gRPC streams recheck ACLs for each DiscoveryRequest
or DiscoveryResponse. If more than 5 minutes has elapsed since the last
ACL check, recheck even without an incoming DiscoveryRequest or
DiscoveryResponse. ACL failures will terminate the stream.
@rboyer rboyer force-pushed the term-grpc-stream-on-token-failure branch from 7c34dbd to d3eb781 Compare January 22, 2019 17:13
@rboyer rboyer merged commit db8a871 into master Jan 29, 2019
@rboyer rboyer deleted the term-grpc-stream-on-token-failure branch January 29, 2019 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants