Refactor Config-file-based auth #1051

johnflavin · 2018-08-04T02:38:55Z

I wanted to support the credsHelper values in the .docker/config.json file. Along the way I kind of refactored all the code around that file and using it for credentials, because it was all kind of a mess.

Add support for credsHelper values in .docker/config.json. Closes Add support for credHelpers in new docker versions #1037.
You can have different registries in the "auth" and "credsHelpers" sections, and their values will get used correctly. And if you want to auth with a registry that is not explicitly mentioned in either of those, the credential helper from "credsStore" will be used as a fallback. Closes Compatibility between credsStore and passphrase. #1042.
Added auto value classes DockerConfig and DockerCredentialHelperAuth to bind JSON into objects and avoid parsing out JSON in code. The former reflects the structure of the .docker/config.json file. The latter is the auth response received from a credential helper on a get, and sent on a store.
Abstracted all communication with credential helpers into its own class: DockerCredentialHelper. I implemented all the methods that are supported for these processes: get, list, store, and erase. (Right now only the get is used in any of the code, but the rest are available for users to use as they like. This might be something we want to expose through the main client? Or maybe not.) I followed the delegate pattern established in DockerHost (see Move default Docker constants + methods into DockerHost #441 and code comments therein) to enable mocking out those system calls for tests.
Related to above, I removed some existing tests that were interacting with live credential helpers. I'm not comfortable storing and erasing values in secure locations on servers, let alone on my own machine, during tests. I didn't add back any tests of the credential helpers explicitly because... it feels like I would have just been mocking something and verifying that mock. I'd be testing "did Mockito work? yep!"
The credential helper get operation is mocked and used in some other tests that I added, since it is the one credential helper operation involved in the whole authentication chain.
There's probably other stuff I did in here.

Lastly, in my mind this fills some of the same role of the ECR support PRs #876 and #1013. Amazon has a docker credential helper (awslabs/amazon-ecr-credential-helper), so having that installed and referenced as a credsStore or credsHelper in your docker config file should allow docker-client to support it with minimal code changes. I don't know the degree to which this level of support is better or worse than a full ECR RegistryAuth implementation, because I don't know the intricacies of their auth API, so take that for what you will. But I expect (and hope) this will be sufficient to fulfill my needs.

codecov-io · 2018-08-04T02:51:16Z

Codecov Report

Merging #1051 into master will decrease coverage by 0.05%.
The diff coverage is 50.73%.

@@             Coverage Diff              @@
##             master    #1051      +/-   ##
============================================
- Coverage     67.37%   67.32%   -0.06%     
- Complexity      774      795      +21     
============================================
  Files           176      178       +2     
  Lines          3234     3287      +53     
  Branches        368      385      +17     
============================================
+ Hits           2179     2213      +34     
- Misses          898      911      +13     
- Partials        157      163       +6

johnflavin · 2018-08-14T13:39:42Z

Ping @davidxia @mattnworb for review.

mattnworb · 2018-08-14T14:21:33Z

@johnflavin I’ve been away traveling and won’t have a chance to review this in depth until next week. I did want to say thanks a ton though for doing this, from the PR description alone this sounds amazing. 🙌

mattnworb

LGTM, thanks again so much for doings this! I have a few small questions/requests but I don't think they are big enough to set my status to "Request changes".

Related to above, I removed some existing tests that were interacting with live credential helpers. I'm not comfortable storing and erasing values in secure locations on servers, let alone on my own machine, during tests.

agreed 💯 %

Lastly, in my mind this fills some of the same role of the ECR support PRs #876 and #1013. Amazon has a docker credential helper (awslabs/amazon-ecr-credential-helper), so having that installed and referenced as a credsStore or credsHelper in your docker config file should allow docker-client to support it with minimal code changes. I don't know the degree to which this level of support is better or worse than a full ECR RegistryAuth implementation, because I don't know the intricacies of their auth API, so take that for what you will. But I expect (and hope) this will be sufficient to fulfill my needs.

I'd say that support creds-helper over adding code support for various providers makes more sense to me too. This seems to be the direction that Google Cloud is moving in as well, gcloud now ships a docker credentials helper (and has deprecated their support for the non-credentials-helper path)

mattnworb · 2018-08-22T15:07:50Z

src/main/java/com/spotify/docker/client/DockerConfig.java

+  public abstract ImmutableMap<String, RegistryAuth> auths();
+
+  @Nullable
+  @JsonProperty("HttpHeaders")


confusing that this field is capitialized differently than all the others 😕 I double-checked the format in https://github.com/docker/cli/blob/08cf36daa65e22771cc47365ff1507c156c4a459/man/docker-config-json.5.md to make sure this seems ok though 👍

I hadn't seen that page. All the searches I did for docker config got swamped by the new feature: https://docs.docker.com/engine/reference/commandline/config/.

I should add several of these properties to the object. Not that I think there is any real chance someone will need them, but if we have the thing, it may as well be able to read what we know can be in there.

mattnworb · 2018-08-22T15:13:44Z

src/main/java/com/spotify/docker/client/DockerConfigReader.java

    checkNotNull(configPath);

-    final Map<String, RegistryAuth> configs = parseDockerConfig(configPath).configs();
+    final DockerConfig config = MAPPER.readValue(configPath.toFile(), DockerConfig.class);
+    if (config == null) {


not very important, but can ObjectMapper.readValue() actually ever return null?

I wrote this null check purely defensively. But, given your question, I dug into the source to figure this out. Yes, it is possible.

readValue(File , Class<T>) → _readMapAndClose(JsonParser, JavaType). The latter does under some conditions set Object result = null and returns result.

mattnworb · 2018-08-22T15:54:59Z

src/main/java/com/spotify/docker/client/messages/DockerCredentialHelperAuth.java

+import com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class DockerCredentialHelperAuth {


would you mind adding a brief javadoc summary of what this class, and the other new AutoValue ones, are intended to represent? I have a hard time remembering the difference between all of these that have very similar names (RegistryAuth, RegistryAuths) etc

mattnworb · 2018-08-22T16:00:08Z

I didn't add back any tests of the credential helpers explicitly because... it feels like I would have just been mocking something and verifying that mock. I'd be testing "did Mockito work? yep!"

IMHO tests like what you can describe can still be useful to test that the class-under-test is behaving the way you expect based on the return value of the mocked method, but if all the class-under-test does is return the same value from the mocked method, then yeah I would agree it's not a very useful test.

…Clarify names and javadoc.

…or testing)

…st with auths hitting values in "auths", "credsHelpers", and "credsStore".

…th process

johnflavin · 2018-08-23T16:28:51Z

I wrote some javadoc on the new classes DockerConfig, DockerCredentialHelper, and DockerCredentialHelperAuth. I also wrote some on RegistryAuth, even though I didn't write that, because why not?

I also rebased on master, cleaned up a couple things, and logged the changes.

…blob/08cf36daa65e22771cc47365ff1507c156c4a459/man/docker-config-json.5.md

mattnworb · 2018-08-23T19:57:37Z

thanks for also updating the changelog ❤️

mattnworb · 2018-08-23T20:14:41Z

I'll try to release 8.12.0 with this in it next

mattnworb · 2018-08-23T21:45:39Z

my gpg is screwed up so I did not get to finish releasing this today, will take another crack on Monday.

mattnworb · 2018-08-26T18:45:48Z

8.12.0 has been released!

mattnworb approved these changes Aug 22, 2018

View reviewed changes

johnflavin added 13 commits August 23, 2018 11:06

Add builder to RegistryConfigs

3553945

Refactor config parsing. Add credsHelpers support.

3b49937

Remove unused/deprecated code. Un-deprecate code that is still used. …

37cb360

…Clarify names and javadoc.

Refactor credential helper code into its own class (with a delegate f…

d4c25ad

…or testing)

Refactor code that gets a single RegistryAuth for a given registry

5ed7e4a

Use entrySet to satisfy findbugs

020a7d9

Fix a bug in the order of reading different config sections. Add a te…

8ead412

…st with auths hitting values in "auths", "credsHelpers", and "credsStore".

Minor: fix logging in creds helper

a618b9d

Add javadoc to new classes

b8f1a32

Remove RegistryAuthV2, which was used in the prior config reader / au…

31e5f72

…th process

Add javadoc for RegistryAuth

5332f27

Minor: reuse boolean value

0282016

changelog

c72c1d1

johnflavin added 2 commits August 23, 2018 11:36

Add docker config file properties from https://github.com/docker/cli/…

acfd42f

…blob/08cf36daa65e22771cc47365ff1507c156c4a459/man/docker-config-json.5.md

Fix javadoc style

d2ede02

mattnworb merged commit 6ea96c5 into spotify:master Aug 23, 2018

johnflavin deleted the creds branch August 24, 2018 02:15

t1ernan mentioned this pull request Sep 12, 2018

Add authentication support for Amazon ECR #1013

Closed

davidxia mentioned this pull request Oct 8, 2018

Tests fail if ~/.docker/config.json has credsStore key spotify/dockerfile-maven#205

Closed

NeoCN mentioned this pull request Nov 8, 2018

unauthorized: authentication required when dockerfile:push os macOS spotify/dockerfile-maven#247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Config-file-based auth #1051

Refactor Config-file-based auth #1051

johnflavin commented Aug 4, 2018

codecov-io commented Aug 4, 2018 •

edited

Loading

johnflavin commented Aug 14, 2018

mattnworb commented Aug 14, 2018

mattnworb left a comment

mattnworb Aug 22, 2018

johnflavin Aug 23, 2018

mattnworb Aug 22, 2018

johnflavin Aug 23, 2018

mattnworb Aug 22, 2018

mattnworb commented Aug 22, 2018

johnflavin commented Aug 23, 2018

mattnworb commented Aug 23, 2018

mattnworb commented Aug 23, 2018

mattnworb commented Aug 23, 2018

mattnworb commented Aug 26, 2018

Refactor Config-file-based auth #1051

Refactor Config-file-based auth #1051

Conversation

johnflavin commented Aug 4, 2018

codecov-io commented Aug 4, 2018 • edited Loading

Codecov Report

johnflavin commented Aug 14, 2018

mattnworb commented Aug 14, 2018

mattnworb left a comment

Choose a reason for hiding this comment

mattnworb Aug 22, 2018

Choose a reason for hiding this comment

johnflavin Aug 23, 2018

Choose a reason for hiding this comment

mattnworb Aug 22, 2018

Choose a reason for hiding this comment

johnflavin Aug 23, 2018

Choose a reason for hiding this comment

mattnworb Aug 22, 2018

Choose a reason for hiding this comment

mattnworb commented Aug 22, 2018

johnflavin commented Aug 23, 2018

mattnworb commented Aug 23, 2018

mattnworb commented Aug 23, 2018

mattnworb commented Aug 23, 2018

mattnworb commented Aug 26, 2018

codecov-io commented Aug 4, 2018 •

edited

Loading