[JENKINS-48437] Authorization for Docker Endpoint #68

Dohbedoh · 2018-03-28T00:51:09Z

Proposed a fix for the issue with the method that retrieves the credentials to get the DockerRegistryToken.
Add implementation accepting a Run as context (and using CredentialsProvider.findCredentialById) that can be later leverage by docker-workflow.

@reviewbybees

jglick

Not entirely sure what is going on here but this does not look like the right solution to me. Anyway there is no downstream PR in docker-workflow using a timestamped snapshot to consume the newly introduced APIs, much less a test that demonstrates the bug and the effectiveness of the fix using MockQueueItemAuthenticator.

jglick · 2018-04-05T01:10:26Z

src/main/java/org/jenkinsci/plugins/docker/commons/credentials/DockerRegistryEndpoint.java

@@ -182,10 +185,35 @@ DockerRegistryToken getToken(Item context) {

        // look for subtypes that know how to create a token, such as Google Container Registry
        return AuthenticationTokens.convert(DockerRegistryToken.class, firstOrNull(CredentialsProvider.lookupCredentials(
-                IdCredentials.class, context, Jenkins.getAuthentication(),requirements),
+                IdCredentials.class, context, Tasks.getAuthenticationOf((Queue.Task)context),requirements),


Smells wrong. As you can see from the Javadoc of that method, it is a prediction, not an indication of what actual authentication is associated with the build—which would be Jenkins.getAuthentication().

This is an attempt to fix the JENKINS-48437 without needing to update the dependencies in docker-worflow (or other dependents of docker-commons).

jglick · 2018-04-05T01:11:36Z

src/main/java/org/jenkinsci/plugins/docker/commons/credentials/DockerServerEndpoint.java

+                    domainRequirements);
+        }
+
+        // the directory needs to be outside workspace to avoid prying eyes


If a new overload is indeed necessary, clearly we would want the duplicated code to be factored out into an internal method somehow.

@jglick How do you want me to factor this out exactly ? I don't see how I could avoid to duplicate the code here unless I use a typing like Object or one of the interface that Run and Item have in common (not sure if doing this would make sense though). So is it worth it ?

I guess it does not matter so long as only one nondeprecated code path remains.

Dohbedoh · 2018-04-13T11:51:35Z

@jglick Are the PR builds deploying snapshots that I can use ? How exactly can I do the following ?:

there is no downstream PR in docker-workflow using a timestamped snapshot to consume the newly introduced APIs

jglick · 2018-05-11T13:03:39Z

Are the PR builds deploying snapshots that I can use?

No; traditionally this was done by a manual mvn deploy. Now that JEP-305 is live, though, I could just “incrementalify” this repository.

jglick · 2018-05-17T17:00:09Z

…#70

…ntialById

…tication

Dohbedoh

@jglick Could I get further review ?

Dohbedoh · 2018-08-03T11:09:01Z

src/main/java/org/jenkinsci/plugins/docker/commons/credentials/DockerServerEndpoint.java

+                    domainRequirements);
+        }
+
+        // the directory needs to be outside workspace to avoid prying eyes


@jglick How do you want me to factor this out exactly ? I don't see how I could avoid to duplicate the code here unless I use a typing like Object or one of the interface that Run and Item have in common (not sure if doing this would make sense though). So is it worth it ?

stephenc

Credentials stuff looks correct from 10,000ft

jglick

A few nits. Will have to take @stephenc’s word for it on the appropriateness of the new API calls, since there is no functional test. (I never managed to write one in DockerRegistryEndpointTest, especially now as it would be trying to run docker login, so you would need to either have a mock endpoint accepting the authentication or wrap in a test block step with a LauncherDecorator that just echoes commands rather than actually running them.)

src/main/java/org/jenkinsci/plugins/docker/commons/credentials/DockerRegistryEndpoint.java

jglick · 2019-01-04T19:37:45Z

src/main/java/org/jenkinsci/plugins/docker/commons/credentials/DockerServerEndpoint.java

+                    domainRequirements);
+        }
+
+        // the directory needs to be outside workspace to avoid prying eyes


I guess it does not matter so long as only one nondeprecated code path remains.

jglick · 2019-01-04T19:38:43Z

src/main/java/org/jenkinsci/plugins/docker/commons/credentials/DockerRegistryEndpoint.java

+     *       specify that build. Otherwise null. If you are scoped to something else, you might
+     *       have to interact with {@link CredentialsProvider} directly.
+     */
+    public @CheckForNull DockerRegistryToken getToken(Run context) {


Why is this public if it is only called internally and from tests? Make it default access. If and when some other plugin actually needs to call this (apparently docker-workflow does not), a separate set of PRs can be made which demonstrates the usage.

jglick · 2019-01-04T19:39:15Z

src/main/java/org/jenkinsci/plugins/docker/commons/credentials/DockerRegistryEndpoint.java

+     *
+     * @param context
+     *       If you are a build step trying to access DockerHub in the context of a build/job,
+     *       specify that build. Otherwise null. If you are scoped to something else, you might


Otherwise null.

Then mark it @CheckForNull.

Done in bc6d2de.

jglick · 2019-01-04T19:41:04Z

src/main/java/org/jenkinsci/plugins/docker/commons/credentials/DockerRegistryEndpoint.java

@@ -181,12 +190,44 @@ DockerRegistryToken getToken(Item context) {
            // shrug off this error and move on. We are matching with ID anyway.
        }

+        Authentication runAuth = context instanceof Queue.Task ?


Why it getToken(Item) being patched if this method no longer being used?

src/test/java/org/jenkinsci/plugins/docker/commons/credentials/DockerRegistryEndpointTest.java

jglick · 2019-01-04T19:44:40Z

src/test/java/org/jenkinsci/plugins/docker/commons/credentials/DockerRegistryEndpointTest.java

@@ -72,6 +89,56 @@ public void testParseNullUrlAndImageName() throws Exception {
        new DockerRegistryEndpoint(null, null).imageName(null);
    }

+    @Issue("JENKINS-48437")
+    @Test
+    public void testGetTokenForItem() throws IOException {


Do we care about this case? IIUC it is deprecated anyway.

jglick · 2019-01-04T19:45:23Z

src/test/java/org/jenkinsci/plugins/docker/commons/credentials/DockerRegistryEndpointTest.java

+                globalCredentialsId, "test-global-creds", "user", "password");
+        CredentialsProvider.lookupStores(j.jenkins).iterator().next().addCredentials(Domain.global(), credentials);
+
+        FreeStyleProject item = j.createFreeStyleProject("testGetToken");


BTW @Rule JenkinsRule starts a new instance for each test case, so there is no need to pass a job name argument.

jglick · 2019-01-04T19:46:10Z

src/test/java/org/jenkinsci/plugins/docker/commons/credentials/DockerRegistryEndpointTest.java

+
+        FreeStyleProject item = j.createFreeStyleProject("testGetToken");
+
+        SecurityContextHolder.getContext().setAuthentication(User.get("alice").impersonate());


Please rather use ACL.as, even in tests. Just safer.

jglick · 2019-01-04T19:47:03Z

src/test/java/org/jenkinsci/plugins/docker/commons/credentials/DockerRegistryEndpointTest.java

+import hudson.model.*;
+import jenkins.model.Jenkins;
+import org.acegisecurity.context.SecurityContextHolder;
+import org.apache.commons.codec.binary.Base64;


BTW there is a standard utility now in Java 8.

stale

jglick · 2019-01-04T19:57:50Z

there is no functional test. (I never managed to write one in DockerRegistryEndpointTest

RegistryEndpointStepTest I meant.

…though you could write a downstream functional test demonstrating the complete scenario in ServerEndpointStepTest, using variables as a model (no need for the Jenkins restart in the middle), since in that case the step does more portable stuff which you can test comfortably using MockQueueItemAuthenticator and a user credentials store.

mfominov · 2019-03-01T16:46:14Z

@jglick any plans to accept this PR or fix this bug by Jenkins core team?

Dohbedoh · 2019-03-05T00:27:48Z

Adding tests downstream: jenkinsci/docker-workflow-plugin#141

oleg-nenashev · 2019-03-05T03:34:35Z

Jenkins core team has nothing to do with this plugin. Individual contributors might be interested, but AFAIK there is no active maintainer here

…

On Tue, Mar 5, 2019, 01:27 Allan Burdajewicz ***@***.***> wrote: Adding tests downstream: jenkinsci/docker-workflow-plugin#141 <jenkinsci/docker-workflow-plugin#141> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#68 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC3IoIvwGk9e5o7sQwTZyslL5D6KHF7uks5vTboKgaJpZM4S92l7> .

caitlinelfring · 2019-03-18T18:12:20Z

Hoping to get an update on releasing this? It's currently blocking me from using docker within pipelines with authentication. Thanks in advance!

dwnusbaum

Ok, I dug through the credential-related APIs to understand the key differences between the old and new code. The summary is that CredentialsProvider.findCredentialById has a fallback to call Credentials.lookupCredentials with ACL.SYSTEM if the Run's authentication has Credentials.USE_ITEM permissions (implied by Job.CONFIGURE in practice) on the Run's Job. Users without Credentials.USE_ITEM still won't be able to use credentials in some cases if authorize-project is in use, so that's something to keep in mind if we see further reports from users.

We could inline that same fallback here to avoid introducing new APIs and deprecating the existing ones, but Credentials.findCredentialById has other fallbacks related to parameters that might matter in some cases so I think this PR is the best approach.

I submitted #74 which subsumes this PR to address the minor nits I noted and to add another case to the new test to make the permissions that matter a bit clearer.

dwnusbaum · 2019-03-29T18:58:24Z

src/main/java/org/jenkinsci/plugins/docker/commons/credentials/DockerRegistryEndpoint.java

+     *
+     * @param context
+     *       If you are a build step trying to access DockerHub in the context of a build/job,
+     *       specify that build. Otherwise null. If you are scoped to something else, you might


Done in bc6d2de.

dwnusbaum · 2019-03-29T19:11:08Z

src/main/java/org/jenkinsci/plugins/docker/commons/credentials/DockerServerEndpoint.java

-import hudson.model.Describable;
-import hudson.model.Descriptor;
-import hudson.model.Item;
+import hudson.model.*;


nit: please don't do this in non-test code (and even in test code I tend to avoid it for things other than assertion methods or similar)

dwnusbaum · 2019-03-29T19:20:07Z

src/main/java/org/jenkinsci/plugins/docker/commons/credentials/DockerRegistryEndpoint.java

+        try {
+            requirements = Collections.<DomainRequirement>singletonList(new HostnameRequirement(getEffectiveUrl().getHost()));
+        } catch (IOException e) {
+            LOGGER.log(Level.WARNING, "Could not initiliaze registry URL: ", e);


nit: typo. Also, given that the previous code didn't log here, would it be best for this to not log as well, or at least to log only at Level.FINE (and make the old code log as well for consistency)?

jglick requested a review from stephenc April 5, 2018 01:11

jglick previously requested changes Apr 5, 2018

View reviewed changes

Dohbedoh force-pushed the JENKINS-48437 branch from beb21e9 to 70083ca Compare April 12, 2018 13:26

Dohbedoh force-pushed the JENKINS-48437 branch from 70083ca to aa94429 Compare May 18, 2018 09:28

Dohbedoh added 5 commits May 21, 2018 13:06

[JENKINS-48437] Add implementation for a Run context to use findCrede…

1c52f67

…ntialById

[JENKINS-48437] Fix issue with authorization by using the Item authen…

5fc78cc

…tication

[JENKINS-48437] Use same logic as findCredentialsById but for Item

1d1e52a

[JENKINS-48437] Add tests for getToken methods

35280c0

[JENKINS-48437] Add envVars argument

6f6d770

Dohbedoh force-pushed the JENKINS-48437 branch from aa94429 to 6f6d770 Compare May 21, 2018 03:13

Dohbedoh mentioned this pull request May 21, 2018

[JENKINS-48437] Use the new API passing the Run to retrieve the token jenkinsci/docker-workflow-plugin#141

Merged

Dohbedoh commented Aug 3, 2018

View reviewed changes

stephenc approved these changes Sep 3, 2018

View reviewed changes

jglick self-requested a review January 2, 2019 20:20

jglick reviewed Jan 4, 2019

View reviewed changes

Dohbedoh added 6 commits January 21, 2019 21:08

[JENKINS-48437] Use ACL instead of SecurityContextHolder

46f38cd

[JENKINS-48437] Use java.util.Base64

3885e44

[JENKINS-48437] Use @WithoutJenkins where no Jenkins instance is needed

087a24d

[JENKINS-48437] Remove test of deprecated method

7cfe9ef

[JENKINS-48437] Deprecate old overload

9611f55

[JENKINS-48437] Deprecate old overloads and apply few fixes

bc6d2de

jglick self-requested a review January 22, 2019 18:29

jglick removed their request for review March 18, 2019 18:49

dwnusbaum approved these changes Apr 1, 2019

View reviewed changes

dwnusbaum mentioned this pull request Apr 1, 2019

[JENKINS-48437] Add new APIs to retrieve Docker credentials for a Run #74

Merged

dwnusbaum merged commit bc6d2de into jenkinsci:master Apr 2, 2019

kneirinck mentioned this pull request Feb 14, 2024

Fix accessing credentials when authorize project is enabled jfrog/jenkins-artifactory-plugin#922

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-48437] Authorization for Docker Endpoint #68

[JENKINS-48437] Authorization for Docker Endpoint #68

Dohbedoh commented Mar 28, 2018 •

edited by jglick

Loading

jglick left a comment

jglick Apr 5, 2018

Dohbedoh Apr 10, 2018

jglick Apr 5, 2018

Dohbedoh Aug 3, 2018

jglick Jan 4, 2019

Dohbedoh commented Apr 13, 2018

jglick commented May 11, 2018

jglick commented May 17, 2018

Dohbedoh left a comment

Dohbedoh Aug 3, 2018

stephenc left a comment

jglick left a comment

jglick Jan 4, 2019

jglick Jan 4, 2019

jglick Jan 4, 2019

dwnusbaum Mar 29, 2019

jglick Jan 4, 2019

jglick Jan 4, 2019

jglick Jan 4, 2019

jglick Jan 4, 2019

jglick Jan 4, 2019

jglick commented Jan 4, 2019

mfominov commented Mar 1, 2019

Dohbedoh commented Mar 5, 2019

oleg-nenashev commented Mar 5, 2019 via email

caitlinelfring commented Mar 18, 2019

dwnusbaum left a comment •

edited

Loading

dwnusbaum Mar 29, 2019

dwnusbaum Mar 29, 2019

dwnusbaum Mar 29, 2019


		FreeStyleProject item = j.createFreeStyleProject("testGetToken");

		SecurityContextHolder.getContext().setAuthentication(User.get("alice").impersonate());

[JENKINS-48437] Authorization for Docker Endpoint #68

[JENKINS-48437] Authorization for Docker Endpoint #68

Conversation

Dohbedoh commented Mar 28, 2018 • edited by jglick Loading

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dohbedoh commented Apr 13, 2018

jglick commented May 11, 2018

jglick commented May 17, 2018

Dohbedoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenc left a comment

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Jan 4, 2019

mfominov commented Mar 1, 2019

Dohbedoh commented Mar 5, 2019

oleg-nenashev commented Mar 5, 2019 via email

caitlinelfring commented Mar 18, 2019

dwnusbaum left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dohbedoh commented Mar 28, 2018 •

edited by jglick

Loading

dwnusbaum left a comment •

edited

Loading