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 access.TokenController for monitoring-aware token-based access control #269

Merged
merged 5 commits into from
Mar 5, 2020

Conversation

stephen-soltesz
Copy link
Contributor

@stephen-soltesz stephen-soltesz commented Mar 4, 2020

This change adds access.TokenController which is capable of admitting or rejecting clients based on an "access_token" HTTP parameter. This change explicitly allows monitoring to issue access_tokens, and assign a context value to the HTTP request context, so that subsequent access controllers can a) identify monitoring requests, b) allow the test even if it would ordinarily be rejected.

To provide the option of exempting monitoring clients, the TokenController should be applied first in a sequence of access controllers.

This controller is not yet enabled.


This change is Reviewable

@coveralls
Copy link
Collaborator

coveralls commented Mar 4, 2020

Pull Request Test Coverage Report for Build 1231

  • 55 of 55 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.5%) to 80.902%

Totals Coverage Status
Change from base Build 1225: 0.5%
Covered Lines: 1758
Relevant Lines: 2173

💛 - Coveralls

Copy link
Contributor

@gfr10598 gfr10598 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @gfr10598 and @stephen-soltesz)


access/control.go, line 1 at r1 (raw file):

package access

Nice!


access/token.go, line 19 at r1 (raw file):

	tokenAccessRequests = promauto.NewCounterVec(
		prometheus.CounterOpts{
			Name: "ndt_access_tokencontroller_requests_total",

Would you consider shortening this? ndt_token_requests_total? The other parts are implicit in the process, and I expect this will be unique across processes.


access/token.go, line 20 at r1 (raw file):

		prometheus.CounterOpts{
			Name: "ndt_access_tokencontroller_requests_total",
			Help: "Total number of requests handled by the access tokencontroller.",

Should help include "ndt"?


access/token.go, line 74 at r1 (raw file):

	if token == "" && !requireTokens {
		// The access token is missing and tokens are not requried, so accept the request.
		tokenAccessRequests.WithLabelValues("accepted").Inc()

Seems like this should have a different label, maybe "no-token"

Copy link
Contributor Author

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @gfr10598)


access/token.go, line 19 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

Would you consider shortening this? ndt_token_requests_total? The other parts are implicit in the process, and I expect this will be unique across processes.

I've made it a little shorter by removing "controller".


access/token.go, line 20 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

Should help include "ndt"?

Done.


access/token.go, line 74 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

Seems like this should have a different label, maybe "no-token"

I'm trying to keep the label request=accepted or request=rejected consistent across the controllers -- I expect this to be important for feedback to the locate service in the future.

I think you're right that it will be helpful to know why a token was accepted or not. So, I've added an additional label for reason= which may be "empty" (no access_token provided and not required), "invalid" (provided but not verified), and "" from a valid claim. Since we'll control the set of issuers, it would only ever be "monitoring" or the locate service issuer id.

@stephen-soltesz stephen-soltesz merged commit b5192ff into master Mar 5, 2020
@stephen-soltesz stephen-soltesz deleted the sandbox-soltesz-token branch March 5, 2020 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants