[JENKINS-41745] Non-Remoting-based CLI #2795

Merged
merged 64 commits into from Apr 8, 2017

Conversation

6 participants
@jglick
Member

jglick commented Mar 10, 2017

JENKINS-41745

Complete proposal

Downstream of jenkinsci/sshd-module#12. Also cf. jenkinsci/support-core-plugin#110, jenkins-infra/jenkins.io#802, jenkinsci-cert/SECURITY-218#8, #2834.

Proposed changelog entries:

  • JENKINS-41745: the use of Jenkins CLI in its traditional “Remoting” mode has been deprecated for security reasons; this mode will be disabled on new installations, and new downloads of jenkins-cli.jar require a -remoting option to use it. You may continue to access the CLI using a stock ssh command; or you may download a new jenkins-cli.jar supporting a new, default HTTP-based protocol as well as SSH-based access. See the client help text for options, as well as the CLI documentation.
  • The install-plugin command now supports a source of = to load a plugin *.hpi from standard input, since use of local file paths only worked in Remoting mode.
  • The build command’s -p option applied to a file parameter now supports a blank filename to load contents from standard input, since use of local file paths only worked in Remoting mode.
  • JENKINS-33595: the SSH port is now disabled by default on new installations. Use Configure Global Security to enable it.
  • implement PlainCLIProtocol on server
  • enhance client
    • implement -http mode and -auth
    • implement -ssh mode
    • -logger, and make client a little less chatty
  • deprecate Remoting-dependent APIs
    • CLICommand.channel, checkChannel, getClientEnvironmentVariable, getClientSystemProperty
    • CLI.<init> and instance methods and related members
    • ClientAuthenticationCache, CliAuthenticator
    • CommandDuringBuild and unsupportable commands
  • deprecate Remoting mode on server side
    • disable CliProtocol & CliProtocol2 in setup wizard
    • UI to enable
    • admin monitor when enabled
    • deprecate -Djenkins.CLI.disabled=true
  • new automated tests
    • tests of authentication modes in -http vs. -remoting
    • -ssh mode specifics
    • unit tests of PlainCLIProtocol
    • interleaved stdio in -http
    • interrupts à la build -s -v in all modes
    • locale & encoding in -http (neither supported by -ssh)
    • replace test usages of CLI with CLICommandInvoker where appropriate
  • improve core commands
    • InstallPluginCommand to accept - for SOURCE to read from stdin
    • FileParameterDefinition.createValue to accept - to read from stdin

@reviewbybees & @jenkinsci/code-reviewers

jglick added some commits Mar 13, 2017

Establishing baseline behavior of JENKINS-12543: no workaround when u…
…sing Remoting transport other than SSH authentication.

(Verifying that this affects only @argument in CLICommand, not @CLIMethod.)
With the new HTTP protocol in JENKINS-41745, API tokens may be used to set a transport authentication.
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 @.)

@jglick jglick referenced this pull request in jenkinsci/sshd-module Mar 15, 2017

Merged

Updating sshd-core to 1.4.0 #10

jglick added some commits Mar 17, 2017

Added -strictHostKey option to CLI in -ssh mode.
[FIXED JENKINS-33595] Picks up jenkinsci/sshd-module#11
to turn off SSHD by default, but expose it to tests which wish to enable it.
Test failure on Windows, perhaps specific to filesystem type of Tempo…
…raryFolder.

java.nio.file.FileSystemException: …\.ssh\known_hosts: Incorrect function.
        at sun.nio.fs.WindowsException.translateToIOException(WindowsException.java:86)
        at sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:90)
        at sun.nio.fs.WindowsAclFileAttributeView.getFileSecurity(WindowsAclFileAttributeView.java:88)
        at sun.nio.fs.WindowsAclFileAttributeView.getOwner(WindowsAclFileAttributeView.java:121)
        at sun.nio.fs.FileOwnerAttributeViewImpl.getOwner(FileOwnerAttributeViewImpl.java:91)
        at java.nio.file.Files.getOwner(Files.java:2079)
        at org.apache.sshd.common.util.io.IoUtils.getFileOwner(IoUtils.java:272)
        at org.apache.sshd.common.util.io.ModifiableFileWatcher.validateStrictConfigFilePermissions(ModifiableFileWatcher.java:240)
        at org.apache.sshd.client.keyverifier.DefaultKnownHostsServerKeyVerifier.reloadKnownHosts(DefaultKnownHostsServerKeyVerifier.java:79)
        at org.apache.sshd.client.keyverifier.KnownHostsServerKeyVerifier.verifyServerKey(KnownHostsServerKeyVerifier.java:161)
        at …
Interactive testing of known_hosts on XP works as expected.
Improved handling of stream closure.
Making interrupt of `build -s` work in `-http` mode.
Does not solve `ReadPendingException`s.

jglick added a commit to jglick/jenkins that referenced this pull request Apr 4, 2017

Porting POM fix from #2795: we must allow snapshots from repo.jenkins…
…-ci.org so as to be able to test upstream component PRs.

@jglick jglick removed the work-in-progress label Apr 6, 2017

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Apr 6, 2017

Member

While there are still some automated tests I would like to write, I consider this functionally complete.

Member

jglick commented Apr 6, 2017

While there are still some automated tests I would like to write, I consider this functionally complete.

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Apr 6, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@@ -48,16 +48,31 @@ private static String basicAuth(String userInfo) {
return null;
}
+ @Deprecated
+ public FullDuplexHttpStream(URL target, String authorization) throws IOException {
+ this(new URL(target.toString().replaceFirst("/cli.*$", "")), target.toString().replaceFirst("(.+)/cli.*$", "$1"), authorization);

This comment has been minimized.

@stephenc

stephenc Apr 6, 2017

Member

This seems to be being altogether too clever for it's own good. at least have a comment explaining what the URL transformation is supposed to be

@stephenc

stephenc Apr 6, 2017

Member

This seems to be being altogether too clever for it's own good. at least have a comment explaining what the URL transformation is supposed to be

This comment has been minimized.

@jglick

jglick Apr 6, 2017

Member

Yeah the original code in createCrumbUrlBase was also too clever for its own good. I can add an explanatory comment.

@jglick

jglick Apr 6, 2017

Member

Yeah the original code in createCrumbUrlBase was also too clever for its own good. I can add an explanatory comment.

@stephenc

Looks good. Could not spot any obvious issues

@oleg-nenashev oleg-nenashev self-requested a review Apr 7, 2017

@oleg-nenashev

🐛 for some changes in the code, but my main concern is the SSHD Core upgrade over the version with explicit binary compatibility changes.

There are a couple of other PRs with such discussion, but I am still afraid it is going to nuke something in Jenkins. FTR jenkinsci/sshd-module#10 required some changes in the SSHD Module codebase, and this module will likely blow up if we enforce newer library in the core.

cli/pom.xml
@@ -50,6 +54,16 @@
<version>1.24</version>
</dependency>
<dependency>
+ <groupId>org.apache.sshd</groupId>
+ <artifactId>sshd-core</artifactId>
+ <optional>true</optional> <!-- do not expose to core -->

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

It's already in the core due to the sshd-module

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

It's already in the core due to the sshd-module

This comment has been minimized.

@jglick

jglick Apr 7, 2017

Member

No, it is in war.

@jglick

jglick Apr 7, 2017

Member

No, it is in war.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

Right, nvm

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

Right, nvm

+ */
+ @Deprecated
+ public FullDuplexHttpStream(URL target, String authorization) throws IOException {
+ this(new URL(target.toString().replaceFirst("/cli.*$", "/")), target.toString().replaceFirst("^.+/(cli.*)$", "$1"), authorization);

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

🐛 Will Boom if Jenkins URL is "https://my.corp/clinkerhq" or whatever. And it was a Jenkins-based vendor at some point... IMHO regexps should take such corner-case into account (and have it autotested)

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

🐛 Will Boom if Jenkins URL is "https://my.corp/clinkerhq" or whatever. And it was a Jenkins-based vendor at some point... IMHO regexps should take such corner-case into account (and have it autotested)

This comment has been minimized.

@jglick

jglick Apr 7, 2017

Member

The only usage of this constructor was from CLI, which passed for example http://jenkins/cli. I am performing the same transformation that the code previously performed in the crumb retrieval.

@jglick

jglick Apr 7, 2017

Member

The only usage of this constructor was from CLI, which passed for example http://jenkins/cli. I am performing the same transformation that the code previously performed in the crumb retrieval.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

I am performing the same transformation that the code previously performed in the crumb retrieval.

This is fine (c) then.

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

I am performing the same transformation that the code previously performed in the crumb retrieval.

This is fine (c) then.

@@ -77,8 +98,9 @@ public FullDuplexHttpStream(URL target, String authorization) throws IOException
con.getOutputStream().close();
input = con.getInputStream();
// make sure we hit the right URL
- if(con.getHeaderField("Hudson-Duplex")==null)
- throw new IOException(target+" doesn't look like Jenkins");
+ if (con.getHeaderField("Hudson-Duplex") == null) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

Maybe worth creating an issue for the "Jenkins-Duplex" header

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

Maybe worth creating an issue for the "Jenkins-Duplex" header

This comment has been minimized.

@jglick

jglick Apr 7, 2017

Member

Not worth the bother I think.

@jglick

jglick Apr 7, 2017

Member

Not worth the bother I think.

@@ -128,8 +150,7 @@ private void getData() {
}
private String createCrumbUrlBase() {
- String url = target.toExternalForm();
- return new StringBuilder(url.substring(0, url.lastIndexOf("/cli"))).append("/crumbIssuer/api/xml/").toString();
+ return base + "crumbIssuer/api/xml/";

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

It assumes base.toString() always ends with slash. Are we sure about that? I am not 🐜

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

It assumes base.toString() always ends with slash. Are we sure about that? I am not 🐜

This comment has been minimized.

@jglick

jglick Apr 7, 2017

Member

This is enforced in the FullDuplexHttpStream constructor.

@jglick

jglick Apr 7, 2017

Member

This is enforced in the FullDuplexHttpStream constructor.

+ STDOUT,
+ /** Chunk of stderr. */
+ STDERR
+ }

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

No KEEPALIVE?

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

No KEEPALIVE?

This comment has been minimized.

@jglick

jglick Apr 7, 2017

Member

Meaning what?

@jglick

jglick Apr 7, 2017

Member

Meaning what?

- * <p>
- * If this connection is lost, we'll abort the channel.
- */
- public synchronized void download(StaplerRequest req, StaplerResponse rsp) throws InterruptedException, IOException {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

4reviewers: moved to the parent class

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

4reviewers: moved to the parent class

+ ping.start();
+ main(channel);
+ channel.join();
+ ping.interrupt();

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

Maybe a bug in the old code, I would expect ping.interrupt() to happen within a finally block

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

Maybe a bug in the old code, I would expect ping.interrupt() to happen within a finally block

This comment has been minimized.

@jglick

jglick Apr 7, 2017

Member

Perhaps. This code was just moved from its original location.

@jglick

jglick Apr 7, 2017

Member

Perhaps. This code was just moved from its original location.

+ UUID uuid = UUID.fromString(req.getHeader("Session"));
+ rsp.setHeader("Hudson-Duplex", "true"); // set the header so that the client would know
+
+ if (req.getHeader("Side").equals("download")) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

yep, nothing changes in the behavior

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

yep, nothing changes in the behavior

+# THE SOFTWARE.
+
+blurb=\
+ Allowing Jenkins CLI to work in <code>-remoting</code> mode is considered dangerous and usually unnecessary. \

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

Nice2Have: Link to the explanation (e.g. in JIRA) or more details in the text body

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

Nice2Have: Link to the explanation (e.g. in JIRA) or more details in the text body

This comment has been minimized.

@jglick

jglick Apr 7, 2017

Member

Yes, I will link to the official documentation.

@jglick

jglick Apr 7, 2017

Member

Yes, I will link to the official documentation.

pom.xml
+ <dependency>
+ <groupId>org.apache.sshd</groupId>
+ <artifactId>sshd-core</artifactId>
+ <version>1.2.0</version> <!-- TODO 1.3.0 requires Java 8 -->

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

🐛 I am VERY VERY afraid of it since there are known compatibility changes in the library, e.g.: https://issues.apache.org/jira/browse/SSHD-441 in SSHD Core 1.0. Now we use 0.14.0 coming from the SSH Module. It needs to get a number of thumbs up from the developer mailing list before going forward.

Any chance to avoid explicit specification?

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

🐛 I am VERY VERY afraid of it since there are known compatibility changes in the library, e.g.: https://issues.apache.org/jira/browse/SSHD-441 in SSHD Core 1.0. Now we use 0.14.0 coming from the SSH Module. It needs to get a number of thumbs up from the developer mailing list before going forward.

Any chance to avoid explicit specification?

This comment has been minimized.

@jglick

jglick Apr 7, 2017

Member

The client uses 1.2.0. The server could continue to run 0.14.0. I had problems making the test module handle two different versions of sshd-core in use. I will see if it is possible to resolve that at least temporarily.

Anyway all discussion of the pros or cons of upgrading should be kept in jenkinsci/sshd-module#10.

@jglick

jglick Apr 7, 2017

Member

The client uses 1.2.0. The server could continue to run 0.14.0. I had problems making the test module handle two different versions of sshd-core in use. I will see if it is possible to resolve that at least temporarily.

Anyway all discussion of the pros or cons of upgrading should be kept in jenkinsci/sshd-module#10.

@jglick jglick referenced this pull request in jenkinsci/sshd-module Apr 7, 2017

Merged

[JENKINS-33595] Disable SSHD port by default on new installations #12

@oleg-nenashev

sshConnection() uses Future await() calls without timeouts. There is also a case when the ClientSession may be leaked (didn't dig into).

+ client.setServerKeyVerifier(verifier);
+ client.start();
+
+ ConnectFuture cf = client.connect(user, sshHost, sshPort);

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

client.stop() won't be invoked if this method or await() fail. Not sure if it matters

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

client.stop() won't be invoked if this method or await() fail. Not sure if it matters

This comment has been minimized.

@jglick

jglick Apr 7, 2017

Member

Should not matter. The process will exit anyway.

@jglick

jglick Apr 7, 2017

Member

Should not matter. The process will exit anyway.

+ client.start();
+
+ ConnectFuture cf = client.connect(user, sshHost, sshPort);
+ cf.await();

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

🐛 no timeout

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

🐛 no timeout

This comment has been minimized.

@jglick

jglick Apr 7, 2017

Member

So? Either it connects eventually; or the socket gives a timeout, in which case we get an exception; or the socket does not give a timeout, in which case the client waits and the user eventually kills it. Why would we need to impose an additional timeout?

@jglick

jglick Apr 7, 2017

Member

So? Either it connects eventually; or the socket gives a timeout, in which case we get an exception; or the socket does not give a timeout, in which case the client waits and the user eventually kills it. Why would we need to impose an additional timeout?

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

in which case the client waits and the user eventually kills it

Jenkins CLI is also a tool for automation. Such possible hanging is not a good UX IMHO. Can convert to Ant if you feel strongly && can handle the possible fallout later if it happens

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

in which case the client waits and the user eventually kills it

Jenkins CLI is also a tool for automation. Such possible hanging is not a good UX IMHO. Can convert to Ant if you feel strongly && can handle the possible fallout later if it happens

This comment has been minimized.

@jglick

jglick Apr 7, 2017

Member

Jenkins CLI is also a tool for automation.

Sure, and automated scripts typically have their own timeouts.

handle the possible fallout later if it happens

Remember that if anyone is dissatisfied with the quality of implementation of SSHCLI they can simply use an OpenSSH (or whatever) client, with its dozens of options and years of tuning and bug fixes. -ssh mode is provided as a convenience to minimize the friction of switching away from Remoting.

@jglick

jglick Apr 7, 2017

Member

Jenkins CLI is also a tool for automation.

Sure, and automated scripts typically have their own timeouts.

handle the possible fallout later if it happens

Remember that if anyone is dissatisfied with the quality of implementation of SSHCLI they can simply use an OpenSSH (or whatever) client, with its dozens of options and years of tuning and bug fixes. -ssh mode is provided as a convenience to minimize the friction of switching away from Remoting.

+ channel.setOut(new NoCloseOutputStream(System.out));
+ channel.setErr(new NoCloseOutputStream(System.err));
+ WaitableFuture wf = channel.open();
+ wf.await();

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

no timeout as well

@oleg-nenashev

oleg-nenashev Apr 7, 2017

Member

no timeout as well

This comment has been minimized.

@jglick

jglick Apr 7, 2017

Member

Ditto.

@jglick

jglick Apr 7, 2017

Member

Ditto.

Revert "Noting that ssh-cli-auth is obsolete."
This reverts commit 5caee58.
In fact ssh-cli-auth contains UserPropertyImpl which is used by the sshd module.
@oleg-nenashev

OK, here we go 🐝

@jglick jglick referenced this pull request in jenkinsci/ssh-cli-auth-module Apr 7, 2017

Merged

[JENKINS-16337] Form validation for SSH public keys #4

@jglick jglick added ready-for-merge and removed needs-review labels Apr 7, 2017

+ handleClose();
+ break; // TODO verify that we hit EOF immediately, not partway into framelen
+ } catch (IOException x) {
+ if (x.getCause() instanceof TimeoutException) {

This comment has been minimized.

@jglick

jglick Apr 7, 2017

Member

On Tomcat 8 it seems this is a SocketTimeoutException. Have not yet checked whether catching that (and continuing) allows a long-running groovysh or the like to work.

We could also send empty ping packets in either direction after every 15s or so of inactivity on that side, but there is no way to know what timeouts will be applied by different containers.

@jglick

jglick Apr 7, 2017

Member

On Tomcat 8 it seems this is a SocketTimeoutException. Have not yet checked whether catching that (and continuing) allows a long-running groovysh or the like to work.

We could also send empty ping packets in either direction after every 15s or so of inactivity on that side, but there is no way to know what timeouts will be applied by different containers.

This comment has been minimized.

@jglick jglick merged commit 7ae404c into jenkinsci:master Apr 8, 2017

2 checks passed

Jenkins This pull request looks good
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@jglick jglick deleted the jglick:SSH-CLI-JENKINS-41745 branch Apr 8, 2017

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Apr 8, 2017

Member

Well, I expected it to stay there for another day after @reviewbybees done and to ping code-reviewers. I am also not a big fan of merging PRs with 64 commits, which do not have JIRA ticket prefix or whatever. It really complicates backporting IMHO.

Member

oleg-nenashev commented Apr 8, 2017

Well, I expected it to stay there for another day after @reviewbybees done and to ping code-reviewers. I am also not a big fan of merging PRs with 64 commits, which do not have JIRA ticket prefix or whatever. It really complicates backporting IMHO.

+ }
+
+ /**
+ * @param base the base URL of Jenkins
* @param target

This comment has been minimized.

@jglick

jglick Apr 10, 2017

Member

Oops, should be relativeTarget.

@jglick

jglick Apr 10, 2017

Member

Oops, should be relativeTarget.

kohsuke pushed a commit that referenced this pull request Apr 26, 2017

oleg-nenashev added a commit that referenced this pull request May 1, 2017

Do not load detached plugins during tests (#2829)
* Do not load detached plugins during tests.
Also warn about plugins which force us to create classes.jar.

* Porting POM fix from #2795: we must allow snapshots from repo.jenkins-ci.org so as to be able to test upstream component PRs.

* Fixed failures in tests which assumed detached plugins were being loaded.

* jenkins-test-harness 2.20

* Unreproducible test failure on CI, probably due to timing.

@oc243 oc243 referenced this pull request in voxpupuli/puppet-jenkins May 2, 2017

Closed

puppet_helper.groovy uses deprecated remoting mode #749

@jglick jglick referenced this pull request in jenkinsci/sshd-module Oct 5, 2017

Merged

Async startup of the SSHD server #19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment