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 accessorID of token when ops are denied by ACL system #7117

Merged
merged 9 commits into from
Jan 27, 2020

Conversation

mkcp
Copy link
Contributor

@mkcp mkcp commented Jan 23, 2020

Intended to resolve customer asks in #7010, plus wherever else I could find ACL debug logs with access to a token.

We employ a few different ways of getting the accessorID, depending on where we are in the agent.

  • agent/local/state.go acquires it via internal RPC because local is a subcomponent of the agent and we don't have access to one. Adding an additional network hop in these service and check sync loops might be prohibitively expensive on a critical path, but I haven't profiled it. I would appreciate feedback on my impl here from folks w/ more Go and consul experience.

  • Then various delegate and srv calls to ResolveIdentityFromToken so we can pull the accessorID off of it.

  • Added some doc comments early on when I was still researching the problem. Can split into its own PR if needed.

And important note is that none of this is particularly tested, because I couldn't find any place that felt like it needed unit testing (could be wrong there) and integration testing logging felt... bizarre? I also had some difficulty building up a test case to induce the logs themselves so much of what I'm submitting here is taken on faith, the type checker, and a few thousand Test{Agent,ACL} runs to ensure I didn't break something important.

I would appreciate that whoever review this PR get about as nitpicky as possible. Treat this code like someone with 5 years of Go experience got a little {drunk/tired from a new baby/etc.} and rushed through this. I would like to learn as much as possible from whatever I missed. Thanks!

Will squash before merge.

@mkcp mkcp added theme/acls ACL and token generation theme/operator-usability Replaces UX. Anything related to making things easier for the practitioner labels Jan 23, 2020
@mkcp mkcp requested a review from a team January 23, 2020 18:07
@mkcp mkcp self-assigned this Jan 23, 2020
agent/agent.go Outdated Show resolved Hide resolved
agent/acl_test.go 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.

Overall this looks great. I had handful of really minor comments or suggestions, as well as one slightly more important one about the new RPC and its usage in the agent local state.

Would it be possible to have tests for any of this? I have my doubts about whether it is feasible without major alterations to our logging and then whether its even desirable. Its probably not worth it but have you given it any consideration?

agent/consul/acl.go Outdated Show resolved Hide resolved
agent/agent.go Show resolved Hide resolved
agent/agent.go Outdated Show resolved Hide resolved
agent/consul/acl_server.go Show resolved Hide resolved
agent/consul/intention_endpoint.go Show resolved Hide resolved
agent/consul/intention_endpoint.go Outdated Show resolved Hide resolved
agent/consul/internal_endpoint.go Outdated Show resolved Hide resolved
agent/consul/internal_endpoint.go Outdated Show resolved Hide resolved
agent/consul/acl_endpoint.go Outdated Show resolved Hide resolved
agent/local/state.go Outdated Show resolved Hide resolved
@mkcp mkcp changed the title Add accessorID of token whens ops are denied by ACL system Add accessorID of token when ops are denied by ACL system Jan 23, 2020
@mkcp
Copy link
Contributor Author

mkcp commented Jan 24, 2020

@mkeeler I believe I hit all of your feedback!

Biggest change is that I extracted the code to grab the ident.ID() field and the accompanying requeset error handling up to an T.aclAccessorID fn that wraps when we call T.srv.ResolveIdentityFromToken or T.Delegate.ResolveIdentityFromToken with small changes depending on the context (a few endpoints & the agent/local/state.go).

There's a decent amount of copy-paste repetition in this approach, but it didn't feel like a complex enough operation to pull up to a utility package/repeatable helper fn. As a result of this change, the call sites for the logs are cleaned up significantly, getting a value to work directly and not having to fuss w/ errors/nil behaviors unrelated to logging the accessorID.

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.

LGTM. This is going to make debugging ACL issues much easier.

@banks
Copy link
Member

banks commented Jan 27, 2020

@pierresouchay you might like this :)

Great job @mkcp 🎉

agent/acl.go Outdated Show resolved Hide resolved
Co-Authored-By: R.B. Boyer <public@richardboyer.net>
@mkcp
Copy link
Contributor Author

mkcp commented Jan 27, 2020

The last unaddressed item from review is: how do we test logs, and perhaps other telemetry-type functionality that's off core functionality but still providers user value.

My impression is that it seems a bit far off the critical path to warrant adding weight to the integration tests, and straightforward enough to see the results of that we'll hear back pretty fast from users if it's broken. And, given it's non-criticality, if it's broken it's not terribly destructive (e.g. dc downtime, lost business-critical records, etc.)

I'm going to squash and merge here this PR without testing it, but I'm willing to continue the discussion and follow up with tests if we decide to go that route.

@mkcp mkcp merged commit 0d336ed into hashicorp:master Jan 27, 2020
@mkcp mkcp deleted the logging/acl-denied branch January 27, 2020 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/acls ACL and token generation theme/operator-usability Replaces UX. Anything related to making things easier for the practitioner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants