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

added named roles feature for module.vault #48586

Merged
merged 7 commits into from Jul 20, 2018

Conversation

Projects
None yet
4 participants
@astorath
Copy link

commented Jul 13, 2018

What does this PR do?

Added an option to define named role for creation of vault tokens. This is the preferred way to control access permissions and available policies of secondary minion tokens: https://www.vaultproject.io/api/auth/token/index.html#create-token

What issues does this PR fix or reference?

Previous Behavior

Child token can have any policies main token have (this is only controlled by vault security mechanisms). Child token can have an option to create other tokens (as parent token does), leading to security risks.
Salt master token must have all the access policies minions have.

New Behavior

User can define specific token named role for minion created tokens and explicitly define its behavior and access policies. Example: https://www.nomadproject.io/docs/vault-integration/index.html#vault-token-role-configuration

Tests written?

Yes

Commits signed with GPG?

No

@rallytime
Copy link
Contributor

left a comment

Thanks for submitting this. I have some python sytle/syntax things that need to be fixed.

I'd also like @gtmanfred to look at this since he is more familiar with the vault module than I am.

url = '{0}/v1/auth/token/create'.format(config['url'])
role_name = config.get('role_name', None)
base_url = config['url']
if role_name is None

This comment has been minimized.

Copy link
@rallytime

rallytime Jul 16, 2018

Contributor

Missing a : at the end of this line.

if role_name is None
url = '{0}/v1/auth/token/create'.format(base_url)
else
url = '{0}/v1/auth/token/create/{1}'.format(base_url, role_name)

This comment has been minimized.

Copy link
@rallytime

rallytime Jul 16, 2018

Contributor

I think this whole block could be simplified:

url = '{0}/v1/auth/token/create'.format(config['url'])
role_name = config.get('role_name')
if role_name:
    url = url + '/' + role_name

What do you think?

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Jul 16, 2018

Contributor

seconded

This comment has been minimized.

Copy link
@cachedout

cachedout Jul 16, 2018

Collaborator

Not that it matters too much but do we run a risk of doubling-up on a slash after URL using this method?

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Jul 17, 2018

Contributor

We control the while url building in this case, the user cannot pass anything in that would cause an extra slash.

@@ -73,7 +78,7 @@ def generate_token(minion_id, signature, impersonated_by_master=False):
payload = {
'policies': _get_policies(minion_id, config),
'num_uses': 1,
'metadata': audit_data
'meta': audit_data

This comment has been minimized.

Copy link
@rallytime

rallytime Jul 16, 2018

Contributor

Can you explain why this key is changing? I didn't see a mention of this in your PR explanation. (Though I certainly could have missed it.)

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Jul 16, 2018

Contributor

I am not sure why it was ever metadata, because the docs for vault v1 say meta https://www.vaultproject.io/api/auth/token/index.html#meta

This comment has been minimized.

Copy link
@rallytime

rallytime Jul 16, 2018

Contributor

Oh, this was mentioned/changed in this PR #48585.

@rallytime rallytime requested a review from gtmanfred Jul 16, 2018

@gtmanfred
Copy link
Contributor

left a comment

looks good to me.

blind-review

if role_name is None
url = '{0}/v1/auth/token/create'.format(base_url)
else
url = '{0}/v1/auth/token/create/{1}'.format(base_url, role_name)

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Jul 16, 2018

Contributor

seconded

@@ -73,7 +78,7 @@ def generate_token(minion_id, signature, impersonated_by_master=False):
payload = {
'policies': _get_policies(minion_id, config),
'num_uses': 1,
'metadata': audit_data
'meta': audit_data

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Jul 16, 2018

Contributor

I am not sure why it was ever metadata, because the docs for vault v1 say meta https://www.vaultproject.io/api/auth/token/index.html#meta

Andrey Tuzhilin added some commits Jul 16, 2018

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2018

@astorath

This comment has been minimized.

Copy link
Author

commented Jul 18, 2018

@rallytime Sure, I just want to update docs and tests a bit.

Andrey Tuzhilin added some commits Jul 18, 2018

Andrey Tuzhilin
@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Jul 18, 2018

@rallytime Re-review requested.

Andrey Tuzhilin
@astorath

This comment has been minimized.

Copy link
Author

commented Jul 19, 2018

@rallytime @cachedout, I've fixed linting errors and added some tests and docs. I don't know which version to address in docs though.

Andrey Tuzhilin
@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2018

Thanks @astorath!

@rallytime rallytime merged commit 103d6dc into saltstack:develop Jul 20, 2018

5 of 9 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has failed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has failed
Details
jenkins/pr/py2-centos-7 running py2-centos-7...
Details
WIP ready for review
Details
codeclimate All good!
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.