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

Fix create token sudo non-root namespace check #7224

Merged
merged 4 commits into from
Aug 5, 2019

Conversation

michelvocks
Copy link
Contributor

The SudoPrivilege check expects a path without a namespace prefix since it calls AllowOperation which is already namespace-aware and checks by default the full context namespace path: https://github.com/hashicorp/vault/blob/master/vault/acl.go#L342

The consequence is that you have to define a separate policy to apply sudo rights for the token/create endpoint e.g.:

This policy allows you to create tokens but only without sudo-rights:

vault policy write -ns=test pol1 -<<EOF
path "auth/token/*" {
    capabilities=["sudo", "create", "update", "delete", "list", "read"]
}
EOF

You can only get sudo-rights by explicitly specifying the namespace:

vault policy write -ns=test pol1 -<<EOF
path "auth/token/*" {
    capabilities=["sudo", "create", "update", "delete", "list", "read"]
}

path "test/auth/token/*" {
    capabilities=["sudo", "create", "update", "delete", "list", "read"]
}
EOF

We should add additional tests on the enterprise side to make sure that this change doesn't introduce other side effects.

@michelvocks michelvocks added this to the 1.2.1 milestone Jul 31, 2019
vault/dynamic_system_view.go Outdated Show resolved Hide resolved
@jefferai
Copy link
Member

jefferai commented Aug 1, 2019

I think there is still a weird case but I'm not sure we need to/should fix it.

Imagine you have a token in ns1/. We, for some reason, are checking the path ns2/foo/bar. This will cause the path that is eventually checked to be ns1/ns2/foo/bar. This is correct from the perspective of AllowOperation, but is potentially incorrect from the perspective of whether SudoPrivilege should be checking a full path or not. So far what gets passed in is a full path.

Possibly the actual correct thing to do is to not do anything at all. Require what comes in to be a full path and validate it against a context with no namespace whatsoever. This will mean AllowOperation will run against a full path -- which is what it's trying to do by attaching the context namespace to the incoming path.

@jefferai jefferai removed this from the 1.2.1 milestone Aug 5, 2019
@jefferai jefferai merged commit 6e1360b into master Aug 5, 2019
@jefferai jefferai deleted the fix_create_token_sudo_nspath branch August 5, 2019 20:04
jefferai pushed a commit that referenced this pull request Aug 5, 2019
* Fix create token sudo non-root namespace check

* Moved path trimming to SudoPrivilege

* Changed to tokenCtx instead of request ctx

* Use root context for AllowOperation; details in comment
@jefferai jefferai added this to the 1.2.1 milestone Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants