Deprecating --username/--password and login/logout in favor of new -a…

…uth option passing BASIC authentication to /cli endpoint. Simpler, does not rely on Remoting, allows use of API tokens, and bypasses JENKINS-12543. (You could actually do this before but only by embedding userinfo in the -s URL, especially awkward for usernames containing @.)
jenkinsci · Mar 13, 2017 · 12ae48e · 12ae48e
1 parent 8d622e0
commit 12ae48e
Show file tree

Hide file tree

Showing 12 changed files with 84 additions and 19 deletions.
diff --git a/cli/src/main/java/hudson/cli/CLI.java b/cli/src/main/java/hudson/cli/CLI.java
@@ -81,6 +81,7 @@
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import static java.util.logging.Level.*;
+import org.apache.commons.io.FileUtils;
 import org.apache.sshd.client.SshClient;
 import org.apache.sshd.client.channel.ClientChannel;
 import org.apache.sshd.client.channel.ClientChannelEvent;
@@ -185,6 +186,11 @@ protected void onDead() {
 
     private Channel connectViaCliPort(URL jenkins, CliPort clip) throws IOException {
         LOGGER.log(FINE, "Trying to connect directly via Remoting over TCP/IP to {0}", clip.endpoint);
+
+        if (authorization != null) {
+            System.err.println("Warning: -auth ignored when using JNLP agent port");
+        }
+
         final Socket s = new Socket();
         // this prevents a connection from silently terminated by the router in between or the other peer
         // and that goes without unnoticed. However, the time out is often very long (for example 2 hours
@@ -420,6 +426,7 @@ public static int _main(String[] _args) throws Exception {
         Mode mode = null;
 
         String user = null;
+        String auth = null;
 
         while(!args.isEmpty()) {
             String head = args.get(0);
@@ -496,6 +503,11 @@ public boolean verify(String s, SSLSession sslSession) {
                 args = args.subList(2, args.size());
                 continue;
             }
+            if (head.equals("-auth") && args.size() >= 2) {
+                auth = args.get(1);
+                args = args.subList(2, args.size());
+                continue;
+            }
             if(head.equals("-p") && args.size()>=2) {
                 httpProxy = args.get(1);
                 args = args.subList(2,args.size());
@@ -532,6 +544,10 @@ public boolean verify(String s, SSLSession sslSession) {
 
         LOGGER.log(FINE, "using connection mode {0}", mode);
 
+        if (user != null && auth != null) {
+            System.err.println("-user and -auth are mutually exclusive");
+        }
+
         if (mode == Mode.SSH) {
             if (user == null) {
                 // TODO SshCliAuthenticator already autodetects the user based on public key; why cannot AsynchronousCommand.getCurrentUser do the same?
@@ -549,6 +565,8 @@ public boolean verify(String s, SSLSession sslSession) {
         String userInfo = new URL(url).getUserInfo();
         if (userInfo != null) {
             factory = factory.basicAuth(userInfo);
+        } else if (auth != null) {
+            factory = factory.basicAuth(auth.startsWith("@") ? FileUtils.readFileToString(new File(auth.substring(1))).trim() : auth);
         }
 
         if (mode == Mode.HTTP) {

diff --git a/cli/src/main/java/hudson/cli/CLIConnectionFactory.java b/cli/src/main/java/hudson/cli/CLIConnectionFactory.java
@@ -60,11 +60,16 @@ public CLIConnectionFactory authorization(String value) {
 
     /**
      * Convenience method to call {@link #authorization} with the HTTP basic authentication.
+     * Currently unused.
      */
     public CLIConnectionFactory basicAuth(String username, String password) {
         return basicAuth(username+':'+password);
     }
 
+    /**
+     * Convenience method to call {@link #authorization} with the HTTP basic authentication.
+     * Cf. {@code BasicHeaderApiTokenAuthenticator}.
+     */
     public CLIConnectionFactory basicAuth(String userInfo) {
         return authorization("Basic " + new String(Base64.encodeBase64((userInfo).getBytes())));
     }

diff --git a/cli/src/main/resources/hudson/cli/client/Messages.properties b/cli/src/main/resources/hudson/cli/client/Messages.properties
@@ -11,6 +11,8 @@ CLI.Usage=Jenkins CLI\n\
    -noKeyAuth   : don't try to load the SSH authentication private key. Conflicts with -i\n\
    -user        : specify user (for use with -ssh)\n\
    -logger FINE : enable detailed logging from the client\n\
+   -auth [ USER:SECRET | @FILE ] : specify username and either password or API token (or load from them both from a file);\n\
+                                   for use with -http, or -remoting but only when the JNLP agent port is disabled\n\
   \n\
   The available commands depend on the server. Run the 'help' command to\n\
   see the list.

diff --git a/core/src/main/java/hudson/cli/CLICommand.java b/core/src/main/java/hudson/cli/CLICommand.java
@@ -333,7 +333,9 @@ public Channel checkChannel() throws AbortException {
     /**
      * Loads the persisted authentication information from {@link ClientAuthenticationCache}
      * if the current transport provides {@link Channel}.
+     * @deprecated Assumes Remoting, and vulnerable to JENKINS-12543.
      */
+    @Deprecated
     protected Authentication loadStoredAuthentication() throws InterruptedException {
         try {
             if (channel!=null)

diff --git a/core/src/main/java/hudson/cli/ClientAuthenticationCache.java b/core/src/main/java/hudson/cli/ClientAuthenticationCache.java
@@ -27,7 +27,9 @@
  *
  * @author Kohsuke Kawaguchi
  * @since 1.351
+ * @deprecated Assumes Remoting, and vulnerable to JENKINS-12543.
  */
+@Deprecated
 public class ClientAuthenticationCache implements Serializable {
     /**
      * Where the store should be placed.

diff --git a/core/src/main/java/hudson/cli/LoginCommand.java b/core/src/main/java/hudson/cli/LoginCommand.java
@@ -1,6 +1,7 @@
 package hudson.cli;
 
 import hudson.Extension;
+import java.io.PrintStream;
 import jenkins.model.Jenkins;
 import org.acegisecurity.Authentication;
 import org.kohsuke.args4j.CmdLineException;
@@ -10,14 +11,22 @@
  *
  * @author Kohsuke Kawaguchi
  * @since 1.351
+ * @deprecated Assumes Remoting, and vulnerable to JENKINS-12543.
  */
 @Extension
+@Deprecated
 public class LoginCommand extends CLICommand {
     @Override
     public String getShortDescription() {
         return Messages.LoginCommand_ShortDescription();
     }
 
+    @Override
+    protected void printUsageSummary(PrintStream stderr) {
+        super.printUsageSummary(stderr);
+        stderr.println(Messages.LoginCommand_FullDescription());
+    }
+
     /**
      * If we use the stored authentication for the login command, login becomes no-op, which is clearly not what
      * the user has intended.

diff --git a/core/src/main/java/hudson/cli/LogoutCommand.java b/core/src/main/java/hudson/cli/LogoutCommand.java
@@ -1,20 +1,29 @@
 package hudson.cli;
 
 import hudson.Extension;
+import java.io.PrintStream;
 
 /**
  * Deletes the credential stored with the login command.
  *
  * @author Kohsuke Kawaguchi
  * @since 1.351
+ * @deprecated See {@link LoginCommand}.
  */
+@Deprecated
 @Extension
 public class LogoutCommand extends CLICommand {
     @Override
     public String getShortDescription() {
         return Messages.LogoutCommand_ShortDescription();
     }
 
+    @Override
+    protected void printUsageSummary(PrintStream stderr) {
+        super.printUsageSummary(stderr);
+        stderr.println(Messages.LogoutCommand_FullDescription());
+    }
+
     @Override
     protected int run() throws Exception {
         ClientAuthenticationCache store = new ClientAuthenticationCache(checkChannel());

diff --git a/core/src/main/java/hudson/security/AbstractPasswordBasedSecurityRealm.java b/core/src/main/java/hudson/security/AbstractPasswordBasedSecurityRealm.java
@@ -50,6 +50,7 @@ public SecurityComponents createSecurityComponents() {
                 new ImpersonatingUserDetailsService(this));
     }
 
+    @Deprecated
     @Override
     public CliAuthenticator createCliAuthenticator(final CLICommand command) {
         return new CliAuthenticator() {

diff --git a/core/src/main/java/hudson/security/CliAuthenticator.java b/core/src/main/java/hudson/security/CliAuthenticator.java
@@ -75,7 +75,9 @@
  *
  * @author Kohsuke Kawaguchi
  * @since 1.350
+ * @deprecated Vulnerable to JENKINS-12543.
  */
+@Deprecated
 public abstract class CliAuthenticator {
     /**
      * Authenticates the CLI invocation. See class javadoc for the semantics.

diff --git a/core/src/main/java/hudson/security/SecurityRealm.java b/core/src/main/java/hudson/security/SecurityRealm.java
@@ -189,7 +189,9 @@ public IdStrategy getGroupIdStrategy() {
      * @return
      *      never null. By default, this method returns a no-op authenticator that always authenticates
      *      the session as authenticated by the transport (which is often just {@link jenkins.model.Jenkins#ANONYMOUS}.)
+     * @deprecated See {@link CliAuthenticator}.
      */
+    @Deprecated
     public CliAuthenticator createCliAuthenticator(final CLICommand command) {
         return new CliAuthenticator() {
             public Authentication authenticate() {

diff --git a/core/src/main/resources/hudson/cli/Messages.properties b/core/src/main/resources/hudson/cli/Messages.properties
@@ -41,9 +41,20 @@ ListJobsCommand.ShortDescription=\
 ListPluginsCommand.ShortDescription=\
  Outputs a list of installed plugins.
 LoginCommand.ShortDescription=\
- Saves the current credential to allow future commands to run without explicit credential information.
+ Saves the current credentials to allow future commands to run without explicit credential information. [deprecated]
+LoginCommand.FullDescription=\
+ Depending on the security realm, you will need to pass something like:\n\
+ --username USER [ --password PASS | --password-file FILE ]\n\
+ May not be supported in some security realms, such as single-sign-on.\n\
+ Pair with the logout command.\n\
+ The same options can be used on any other command, but unlike other generic CLI options,\n\
+ these come *after* the command name.\n\
+ Whether stored or not, this authentication mode will not let you refer to (e.g.) jobs as arguments\n\
+ if Jenkins denies anonymous users Overall/Read and (e.g.) Job/Read.\n\
+ *Deprecated* in favor of -auth.
 LogoutCommand.ShortDescription=\
- Deletes the credential stored with the login command. 
+ Deletes the credentials stored with the login command. [deprecated]
+LogoutCommand.FullDescription=*Deprecated* in favor of -auth.
 MailCommand.ShortDescription=\
  Reads stdin and sends that out as an e-mail.
 SetBuildDescriptionCommand.ShortDescription=\

diff --git a/test/src/test/java/hudson/cli/CLIActionTest.java b/test/src/test/java/hudson/cli/CLIActionTest.java
@@ -124,14 +124,14 @@ public void authentication() throws Exception {
         File jar = tmp.newFile("jenkins-cli.jar");
         FileUtils.copyURLToFile(j.jenkins.getJnlpJars("jenkins-cli.jar").getURL(), jar);
         j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
-        j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy().grant(Jenkins.ADMINISTER).everywhere().to("admin"));
+        j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy().grant(Jenkins.ADMINISTER).everywhere().to(ADMIN));
         j.createFreeStyleProject("p");
         // CLICommand with @Argument:
         assertExitCode(3, false, jar, "-remoting", "get-job", "p"); // IllegalArgumentException from GenericItemOptionHandler
         assertExitCode(3, false, jar, "get-job", "p"); // ditto under new protocol
-        assertExitCode(3, false, jar, "-remoting", "get-job", "--username", "admin", "--password", "admin", "p"); // JENKINS-12543: too late
-        assertExitCode(3, false, jar, "get-job", "--username", "admin", "--password", "admin", "p"); // same
-        assertExitCode(0, false, jar, "-remoting", "login", "--username", "admin", "--password", "admin");
+        assertExitCode(3, false, jar, "-remoting", "get-job", "--username", ADMIN, "--password", ADMIN, "p"); // JENKINS-12543: too late
+        assertExitCode(3, false, jar, "get-job", "--username", ADMIN, "--password", ADMIN, "p"); // same
+        assertExitCode(0, false, jar, "-remoting", "login", "--username", ADMIN, "--password", ADMIN);
         try {
             assertExitCode(3, false, jar, "-remoting", "get-job", "p"); // ClientAuthenticationCache also used too late
         } finally {
@@ -142,9 +142,9 @@ public void authentication() throws Exception {
         // @CLIMethod:
         assertExitCode(6, false, jar, "-remoting", "disable-job", "p"); // AccessDeniedException from CLIRegisterer?
         assertExitCode(6, false, jar, "disable-job", "p");
-        assertExitCode(0, false, jar, "-remoting", "disable-job", "--username", "admin", "--password", "admin", "p"); // works from CliAuthenticator
-        assertExitCode(0, false, jar, "disable-job", "--username", "admin", "--password", "admin", "p");
-        assertExitCode(0, false, jar, "-remoting", "login", "--username", "admin", "--password", "admin");
+        assertExitCode(0, false, jar, "-remoting", "disable-job", "--username", ADMIN, "--password", ADMIN, "p"); // works from CliAuthenticator
+        assertExitCode(0, false, jar, "disable-job", "--username", ADMIN, "--password", ADMIN, "p");
+        assertExitCode(0, false, jar, "-remoting", "login", "--username", ADMIN, "--password", ADMIN);
         try {
             assertExitCode(0, false, jar, "-remoting", "disable-job", "p"); // or from ClientAuthenticationCache
         } finally {
@@ -153,12 +153,12 @@ public void authentication() throws Exception {
         assertExitCode(6, true, jar, "-remoting", "disable-job", "p");
         assertExitCode(0, true, jar, "disable-job", "p");
         // If we have anonymous read access, then the situation is simpler.
-        j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy().grant(Jenkins.ADMINISTER).everywhere().to("admin").grant(Jenkins.READ, Item.READ).everywhere().toEveryone());
+        j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy().grant(Jenkins.ADMINISTER).everywhere().to(ADMIN).grant(Jenkins.READ, Item.READ).everywhere().toEveryone());
         assertExitCode(6, false, jar, "-remoting", "get-job", "p"); // AccessDeniedException from AbstractItem.writeConfigDotXml
         assertExitCode(6, false, jar, "get-job", "p");
-        assertExitCode(0, false, jar, "-remoting", "get-job", "--username", "admin", "--password", "admin", "p");
-        assertExitCode(0, false, jar, "get-job", "--username", "admin", "--password", "admin", "p");
-        assertExitCode(0, false, jar, "-remoting", "login", "--username", "admin", "--password", "admin");
+        assertExitCode(0, false, jar, "-remoting", "get-job", "--username", ADMIN, "--password", ADMIN, "p");
+        assertExitCode(0, false, jar, "get-job", "--username", ADMIN, "--password", ADMIN, "p");
+        assertExitCode(0, false, jar, "-remoting", "login", "--username", ADMIN, "--password", ADMIN);
         try {
             assertExitCode(0, false, jar, "-remoting", "get-job", "p");
         } finally {
@@ -168,9 +168,9 @@ public void authentication() throws Exception {
         assertExitCode(0, true, jar, "get-job", "p"); // but does under new protocol
         assertExitCode(6, false, jar, "-remoting", "disable-job", "p"); // AccessDeniedException from AbstractProject.doDisable
         assertExitCode(6, false, jar, "disable-job", "p");
-        assertExitCode(0, false, jar, "-remoting", "disable-job", "--username", "admin", "--password", "admin", "p");
-        assertExitCode(0, false, jar, "disable-job", "--username", "admin", "--password", "admin", "p");
-        assertExitCode(0, false, jar, "-remoting", "login", "--username", "admin", "--password", "admin");
+        assertExitCode(0, false, jar, "-remoting", "disable-job", "--username", ADMIN, "--password", ADMIN, "p");
+        assertExitCode(0, false, jar, "disable-job", "--username", ADMIN, "--password", ADMIN, "p");
+        assertExitCode(0, false, jar, "-remoting", "login", "--username", ADMIN, "--password", ADMIN);
         try {
             assertExitCode(0, false, jar, "-remoting", "disable-job", "p");
         } finally {
@@ -184,12 +184,14 @@ public void authentication() throws Exception {
         assertExitCode(0, true, jar, "-remoting", "disable-job", "p");
     }
 
+    private static final String ADMIN = "admin@mycorp.com";
+
     private void assertExitCode(int code, boolean useApiToken, File jar, String... args) throws IOException, InterruptedException {
-        String url = j.getURL().toString();
+        List<String> commands = Lists.newArrayList("java", "-jar", jar.getAbsolutePath(), "-s", j.getURL().toString(), /* not covering SSH keys in this test */ "-noKeyAuth");
         if (useApiToken) {
-            url = url.replace("://localhost:", "://admin:" + User.get("admin").getProperty(ApiTokenProperty.class).getApiToken() + "@localhost:");
+            commands.add("-auth");
+            commands.add(ADMIN + ":" + User.get(ADMIN).getProperty(ApiTokenProperty.class).getApiToken());
         }
-        List<String> commands = Lists.newArrayList("java", "-jar", jar.getAbsolutePath(), "-s", url, /* not covering SSH keys in this test */ "-noKeyAuth");
         commands.addAll(Arrays.asList(args));
         assertEquals(code, new Launcher.LocalLauncher(StreamTaskListener.fromStderr()).launch().cmds(commands).stdout(System.out).stderr(System.err).join());
     }