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 support to pass security token through token_secret in DC/OS #943

Merged

Conversation

ggriffiths
Copy link
Contributor

Signed-off-by: Grant Griffiths [email protected]

What this PR does / why we need it:

  • Add supports to pass token through token_secret in dc/os

Which issue(s) this PR fixes (optional)
Closes #928

Special notes for your reviewer:

@ggriffiths ggriffiths requested a review from lpabon March 15, 2019 18:19
@ggriffiths ggriffiths force-pushed the dcos_docker_vol_secrets branch 2 times, most recently from a213398 to a4f3e13 Compare March 15, 2019 18:40
@ggriffiths ggriffiths requested a review from adityadani March 15, 2019 21:12
@ggriffiths ggriffiths force-pushed the dcos_docker_vol_secrets branch from a4f3e13 to fd9f7cd Compare March 15, 2019 21:28
}

// get token secret
secret, context, ok := d.GetTokenSecretFromString(request.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Check if request.Opts has a api.TokenSecret ?

// get token secret
secret, context, ok := d.GetTokenSecretFromString(request.Name)
if ok && d.secretsStore != nil {
token, err := d.secretsStore.GetToken(secret, context)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should return an error when secret store is not initialized, but a token_secret was provided. In this way the user would get the correct error.

In the current format, user would get "not authorized" error instead of "secret store not initialized"

if ok && d.secretsStore != nil {
token, err := d.secretsStore.GetToken(secret, context)
if err != nil {
return ctx, ""
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above

"authorization": "bearer " + token,
})
return metadata.NewOutgoingContext(ctx, md), token
// get token secret
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: moving the GetTokenFromString and GetTokenSecretFromString to addTokenMetadata function, will reduce code duplication

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ended up adding a parseTokenInput that's shared by attachTokenMount and attachToken to reduce code duplication

@ggriffiths ggriffiths force-pushed the dcos_docker_vol_secrets branch 3 times, most recently from 8a8c9b8 to 20392bc Compare March 15, 2019 22:53
@ggriffiths
Copy link
Contributor Author

Addressed PR comments @adityadani

Copy link
Contributor

@adityadani adityadani left a comment

Choose a reason for hiding this comment

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

lgtm

@ggriffiths ggriffiths force-pushed the dcos_docker_vol_secrets branch from ecea611 to f349860 Compare March 18, 2019 21:20
@ggriffiths ggriffiths merged commit 58230f9 into libopenstorage:master Mar 18, 2019
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.

DCOS secrets need to be supported in Docker Volume to get auth token
2 participants