-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add support for identity tokens in config file #1210
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
Conversation
da11a69
to
4b46b79
Compare
docker/auth/auth.py
Outdated
if 'identitytoken' in entry: | ||
conf[registry] = { | ||
'IdentityToken': entry['identitytoken'] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like auth
is still required otherwise this function returns early. Is that expected or could someone has an identitytoken
without an auth
key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the token holds all the information. I had Matt Bentley test it with
UCP yesterday and he confirmed it works, too.
On Wed, Sep 14, 2016, 8:44 AM Daniel Nephin notifications@github.com
wrote:
In docker/auth/auth.py
#1210 (comment):@@ -189,12 +189,17 @@ def parse_auth(entries, raise_on_error=False):
'Found entry (registry={0}, username={1})'
.format(repr(registry), repr(username))
)
conf[registry] = {
'username': username,
'password': password,
'email': entry.get('email'),
'serveraddress': registry,
}
if 'identitytoken' in entry:
conf[registry] = {
'IdentityToken': entry['identitytoken']
}
It looks like auth is still required otherwise this function returns
early. Is that expected or could someone has an identitytoken without an
auth key?—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/docker/docker-py/pull/1210/files/be7d0f01844d5c08ee157446ce96f5bc6381507c..4b46b792eb7bfa0d7cc218e103f43b200f7752a9#r78775816,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABCVnBWNUR-2ZV3xd-2LWjpupwpmpkeLks5qqBZTgaJpZM4J8SKp
.
4b46b79
to
65a290b
Compare
'credentials store instead.' | ||
) | ||
return {} | ||
conf[registry] = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, either the client is using a cred store and all entries are empty, or they're not and none should be, so returning an empty dict here is not a big problem - but just in case weird hybrid config.json
files start popping up, this should allow it to continue working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a missing return or continue here, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah crud, yeah it needs a continue
.
@dnephin PR updated - PTAL! |
08827ba
to
bb2d2e4
Compare
Signed-off-by: Joffrey F <joffrey@docker.com>
bb2d2e4
to
d731a43
Compare
LGTM |
No description provided.