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

Use top-level authentication for extended auth #1286

Closed
wants to merge 1 commit into from
Closed

Use top-level authentication for extended auth #1286

wants to merge 1 commit into from

Conversation

sebastiankirsch
Copy link
Contributor

@sebastiankirsch sebastiankirsch commented Nov 7, 2019

When relying on Extended Authentication for ECR, we unfortunately have to configure the push and pull sections, even though they are the same. The problem seems to be that the AWS session token has to be put into the <auth> element, whereas the general section only knows an <authToken> element. This change would allow Extended Authentication to work with the general configuration.

Maybe the proper way forward would be to deprecate the authToken property and introduce auth - or vice versa.

When relying on [Extended Authentication](https://dmp.fabric8.io/#extended-authentication) for ECR, we unfortunately have to configure the `push` and `pull` sections, even though they are the same. The problem seems to be that the AWS session token has to be put into the `<auth>` element, whereas the general section only knows an `<authToken>` element. This change would allow Extended Authentication to work with the general configuration.

When using system properties on the other hand, the correct property is `docker.auth` instead. So maybe the proper way forward would be to deprecate the `authToken` property and introduce `auth` - or vice versa.
@codecov
Copy link

codecov bot commented Nov 7, 2019

Codecov Report

Merging #1286 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #1286      +/-   ##
============================================
- Coverage     55.44%   55.44%   -0.01%     
  Complexity     1764     1764              
============================================
  Files           156      156              
  Lines          8487     8488       +1     
  Branches       1304     1304              
============================================
  Hits           4706     4706              
- Misses         3327     3328       +1     
  Partials        454      454
Impacted Files Coverage Δ Complexity Δ
...maven/docker/config/RegistryAuthConfiguration.java 0% <0%> (ø) 0 <0> (ø) ⬇️

@rhuss
Copy link
Collaborator

rhuss commented Nov 21, 2019

Sorry, I do not fully understand the issue. Do you say that pull and push uses a different configuration format than what ?

Could you please elaborate a bit when auth and when authToken should be used ? Is there canonical documentation on the AWS side about the extended authentication ? (sorry, not deep in that code as this was a contribution, too, and I don't have much experience with AWS)

@sebastiankirsch
Copy link
Contributor Author

@rhuss We have to configure dmp like this:

<plugin>
    <groupId>io.fabric8</groupId>
    <artifactId>docker-maven-plugin</artifactId>
    <configuration>
        <authConfig>
            <pull>
                <auth>${AWS_SESSION_TOKEN}</auth>
                <username>${AWS_ACCESS_KEY_ID}</username>
                <password>${AWS_SECRET_ACCESS_KEY}</password>
            </pull>
            <push>
                <auth>${AWS_SESSION_TOKEN}</auth>
                <username>${AWS_ACCESS_KEY_ID}</username>
                <password>${AWS_SECRET_ACCESS_KEY}</password>
            </push>
        </authConfig>
    </configuration>
</plugin>

instead of this

<plugin>
    <groupId>io.fabric8</groupId>
    <artifactId>docker-maven-plugin</artifactId>
    <configuration>
        <authConfig>
            <authToken>${AWS_SESSION_TOKEN}</authToken>
            <username>${AWS_ACCESS_KEY_ID}</username>
            <password>${AWS_SECRET_ACCESS_KEY}</password>
        </authConfig>
    </configuration>
</plugin>

Same goes for configuration via System properties. Now this is awkward at least for the AWS-related stuff; but my understanding is that the <auth> and <authToken> elements are used for other authentication mechanisms as well, and are likely to have the same issue.

@sebastiankirsch
Copy link
Contributor Author

"Extended Authentication" (not sure where the term comes from) means to fetch an authorization token to access AWS ECR, AWS's docker repositories.
In order to do that, one has to authenticate the necessary API call with AWS. If used in conjunction with temporary credentials - which is the recommended way to authenticate with AWS for security, you have to provide not only username & password, but also a "session token", hence the third criteria.

rhuss added a commit to rhuss/docker-maven-plugin that referenced this pull request Nov 21, 2019
This allows extended ecr authentication to share the same token for pull and push requests.

See also fabric8io#1286.
@rhuss
Copy link
Collaborator

rhuss commented Nov 21, 2019

Ok, I see. I think we can fix this more generally by deprecating the top-level "authToken" in favor of an "auth" there, too. I created a PR #1296 would be awesome if you could verify whether it fits your bill.

@sebastiankirsch
Copy link
Contributor Author

#1296 looks good to me, just added some minor comments.
It certainly supersedes this PR!

rhuss added a commit to rhuss/docker-maven-plugin that referenced this pull request Nov 21, 2019
This allows extended ecr authentication to share the same token for pull and push requests.

See also fabric8io#1286.
@rhuss
Copy link
Collaborator

rhuss commented Nov 21, 2019

Perfect, so lets get #1296 merged and I'm closing this one.

Thanks!

@rhuss rhuss closed this Nov 21, 2019
@sebastiankirsch
Copy link
Contributor Author

Thank you!

rhuss added a commit that referenced this pull request Nov 21, 2019
* fix(auth): Deprecate "authToken" in favor of "auth"

This allows extended ecr authentication to share the same token for pull and push requests.

See also #1286.

* chore(auth): Applied review comments
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.

3 participants