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

Add a passwordFile option #66

Merged
merged 1 commit into from
Feb 2, 2018

Conversation

matthauck
Copy link
Contributor

Jenkins displays the command line as well as environment variables in
the node's "systemInfo" page. This makes both of the current password
options very readily available in the jenkins UI.

Jenkins displays the command line as well as environment variables in
the node's "systemInfo" page. This makes both of the current password
options very readily available in the jenkins UI.
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

It helps a bit, but generally the plugin should not be using passwords at all

@matthauck
Copy link
Contributor Author

Cool. IMO, being able to register as a build node is a rather privileged operation, to ensure the code is being handed off to a trusted party, and to ensure that the build artifacts are indeed the correct build artifacts. Curious how you would recommend securing the privilege to register new build nodes without a password?

@oleg-nenashev
Copy link
Member

Curious how you would recommend securing the privilege to register new build nodes without a password?

By application tokens. In such case it will limit the exposure of credentials only to the agent creation. Should be fine enough

@oleg-nenashev oleg-nenashev merged commit 5ce68e6 into jenkinsci:master Feb 2, 2018
@matthauck matthauck deleted the add-pass-file branch February 4, 2018 01:24
Vad1mo added a commit to Vad1mo/docker-jenkins-slave-dind that referenced this pull request Feb 7, 2018
Update swarm client to 3.9. 

maybe plan to switch to "–passwordFile" option as implemented in jenkinsci/swarm-plugin#66
@pvandervelde
Copy link

@oleg-nenashev Is there any documentation anywhere on how to use application tokens to register a new build node?

@oleg-nenashev
Copy link
Member

@pvandervelde same as passwords actually. You can take an API token from user profile and then use it as a password in Swarm parameters. It should work in the most of Security Realms

@pvandervelde
Copy link

Ah right, that makes total sense, why didn't I think of that one! Thanks!

@xjg6yzl
Copy link

xjg6yzl commented Mar 6, 2018

Ok, shouldn't this be as simple as adding your api token as the only line in a text file in linux (say /mydir/pswd), and replace -password myapitoken with -passwordFile /mydir/pswd ?
I really can't get this to work. I get no errors on syntax, but cannot make it authenticate to my master. No error from the swarm-client.jar other than a discoverFromMasterUrl
SEVERE: Failed to fetch slave info from Jenkins, HTTP response code: 401

I am using both "-master" and "-tunnel". My plan was to use a slave startup script configured in the master to remove the password file on connection.

@oleg-nenashev
Copy link
Member

@xjg6yzl You exception means that the client cannot retrieve the data from the endpoint:

try {
String url = masterURL.toExternalForm() + "plugin/swarm/slaveInfo";
get = new GetMethod(url);
get.setDoAuthentication(true);
get.addRequestHeader("Connection", "close");
int responseCode = client.executeMethod(get);
if (responseCode != 200) {
if (responseCode == 404) {
String msg = "Failed to fetch swarm information from Jenkins, plugin not installed?";
logger.log(Level.SEVERE, msg);
throw new RetryException(msg);
} else {
String msg ="Failed to fetch slave info from Jenkins, HTTP response code: " + responseCode;
logger.log(Level.SEVERE, msg);
throw new RetryException(msg);
}
}

Error 401 means "Unauthorized". Please check the Master System logs, they should contain more details

@sboardwell
Copy link

Hi @oleg-nenashev, I'm having the same problem as @xjg6yzl - both -password and -passwordEnvVariable work

I've tried:

  • using both password and access token
  • adding the UTF-8 BOM with echo -ne '\xEF\xBB\xBF' to ensure the file is UTF-8

Nothing seems to work.

@sboardwell
Copy link

sboardwell commented Mar 14, 2018

Hi @oleg-nenashev and @xjg6yzl, I have it. The reading of the passwordFile adds a new line to the password value rendering it incorrect.

Here is a quick example to explain what I mean:

$ echo "'$(cat /tmp/test.txt)'"
'bla'

$ groovy -e 'def s = new String(java.nio.file.Files.readAllBytes(java.nio.file.Paths.get("/tmp/test.txt")), "UTF-8"); println ">$s<";'
>bla
<

And here is the correction needed in the code:

$ git --no-pager diff
diff --git a/client/src/main/java/hudson/plugins/swarm/Client.java b/client/src/main/java/hudson/plugins/swarm/Client.java
index 9e94064..bd8ac9d 100644
--- a/client/src/main/java/hudson/plugins/swarm/Client.java
+++ b/client/src/main/java/hudson/plugins/swarm/Client.java
@@ -88,7 +88,7 @@ public class Client {
         }
         // read pass from file if no other password was specified
         if (options.password == null && options.passwordFile != null) {
-            options.password = new String(Files.readAllBytes(Paths.get(options.passwordFile)), "UTF-8");
+            options.password = new String(Files.readAllBytes(Paths.get(options.passwordFile)), "UTF-8").trim();
         }

Should I create a PR?

It would be really good to get this in soon. I don't like having passwords in plain sight.

Cheers,
Steve

@oleg-nenashev
Copy link
Member

The reading of the passwordFile adds a new line to the password value rendering it incorrect.

It should not, but maybe it is encoding-specific (e.g. you have a non-UTF-8 password file encoding).
Trimming is fine IMHO, I doubt anyone may really want to use passwords with trailing spaces. Please submit a pull request, I will get it released

@sboardwell
Copy link

Thanks

@xjg6yzl
Copy link

xjg6yzl commented Mar 16, 2018

Thank you for addressing this. I had intended to set up a wireshark test to see what was going on but hadn't had time yet.

@xjg6yzl
Copy link

xjg6yzl commented Mar 19, 2018

Any idea how soon this change will be available as an update to the swarm plugin?

@oleg-nenashev
Copy link
Member

Sorry, releasing #68

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants