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

Only use delegation passphrase env var for non-base roles #822

Merged
merged 3 commits into from
Jul 19, 2016

Conversation

riyazdf
Copy link
Contributor

@riyazdf riyazdf commented Jul 8, 2016

Part of the issue noted in #820

Signed-off-by: Riyaz Faizullabhoy riyaz.faizullabhoy@docker.com

@@ -234,7 +235,8 @@ func getPassphraseRetriever() notary.PassRetriever {
// For delegation roles, we can also try the "delegation" alias if it is specified
// Note that we don't check if the role name is for a delegation to allow for names like "user"
// since delegation keys can be shared across repositories
if v := env["delegation"]; v != "" {
// This cannot be a base role or imported key, though.
if v := env["delegation"]; !data.IsBaseRole(alias) && !strings.Contains(alias, "imported ") && v != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice catch for if the base roles aren't provided but delegation roles are!

Copy link
Contributor

@cyli cyli Jul 8, 2016

Choose a reason for hiding this comment

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

I'm wondering if we should should specify a constant or otherwise provide some kind of utility function for this, so we ensure that all imported keys have a similarly-formatted alias?

Edit: "this" = the word "imported"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

totally agree, I've refactored "imported " into a constant 👍

@cyli
Copy link
Contributor

cyli commented Jul 8, 2016

Thanks for fixing this bug! I have one caveat about the word "imported" being special, but this LGTM other than that!

@endophage
Copy link
Contributor

Want to circle back on this once the import refactor is merged

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
@riyazdf riyazdf force-pushed the passphrase-delegation-alias branch from 3373c40 to 06a12a8 Compare July 19, 2016 21:56
@riyazdf
Copy link
Contributor Author

riyazdf commented Jul 19, 2016

@endophage: rebased, doesn't handle the "imported" keyword since that was removed

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
@riyazdf riyazdf force-pushed the passphrase-delegation-alias branch from 06a12a8 to 7a98a07 Compare July 19, 2016 22:31
@endophage
Copy link
Contributor

LGTM pending CI

@riyazdf riyazdf merged commit 0abcc1b into master Jul 19, 2016
@riyazdf riyazdf deleted the passphrase-delegation-alias branch July 19, 2016 23:30
@riyazdf riyazdf mentioned this pull request Jul 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants