Unify two entrypoints #677
Conversation
basil
force-pushed
the
entrypoints
branch
3 times, most recently
from
a3d6afb
to
3ecf5f6
Compare
basil
force-pushed
the
entrypoints
branch
3 times, most recently
from
993610b
to
bcf78a8
Compare
| private SSLSocketFactory getSSLSocketFactory() | ||
| @CheckForNull | ||
| @Restricted(NoExternalUse.class) | ||
| static SSLSocketFactory getSSLSocketFactory(List<X509Certificate> x509Certificates) |
There was a problem hiding this comment.
Widening the visibility of this method and making static so that it can be reused in another place where this same logic is needed. Also added a @CheckForNull annotation while I was here, since this method can sometimes return null.
| /** | ||
| * @deprecated use {@link #secret}, {@link #name}, {@link #urls}, {@link #webSocket}, {@link #tunnel}, | ||
| * {@link #workDir}, {@link #internalDir}, and/or {@link #failIfWorkDirIsMissing} directly. | ||
| */ | ||
| @Option(name="-jnlpUrl",usage="instead of talking to the controller via stdin/stdout, " + | ||
| "emulate a JNLP client by making a TCP connection to the controller. " + | ||
| "Connection parameters are obtained by parsing the JNLP file.") | ||
| "Connection parameters are obtained by parsing the JNLP file.", forbids = {"-direct", "-name", "-tunnel", "-url", "-webSocket"}) | ||
| @Deprecated | ||
| public URL agentJnlpURL = null; |
There was a problem hiding this comment.
The most complicated mode of this plugin is when -jnlpUrl is passed in, which is not done by the Docker inbound agent image used for the Kubernetes plugin but which is recommended when creating an inbound agent in the UI. In this mode, you have to pass some arguments in the command-line invocation, but some other arguments can be provided by the server from the pseudo-JNLP endpoint. This is quite complex and it seems simpler to prefer that all clients simply invoke the tool with the parameters they want rather than relying on the pseudo-JNLP endpoint as a primitive bootstrapping mechanism.
| public URL agentJnlpURL = null; | ||
|
|
||
| @Option(name="-jnlpCredentials",metaVar="USER:PASSWORD",usage="HTTP BASIC AUTH header to pass in for making HTTP requests.") | ||
| @Option(name="-credentials",metaVar="USER:PASSWORD",aliases="-jnlpCredentials",usage="HTTP BASIC AUTH header to pass in for making HTTP requests.") |
There was a problem hiding this comment.
-credentials in the other entrypoint. Since JNLP is an outdated concept, I think it's better to prefer -credentials as the main name for this argument.
| * @deprecated use {@link #agentJnlpCredentials} or {@link #proxyCredentials} | ||
| */ | ||
| @Deprecated | ||
| @Option(name="-auth",metaVar="user:pass",usage="(deprecated) unused; use -credentials or -proxyCredentials") | ||
| public String auth = null; |
There was a problem hiding this comment.
The implementation of this was dead code, which is now deleted. The only ways of passing credentials that actually did anything were -credentials and/or -proxyCredentials.
| SSLContext context = SSLContext.getInstance("TLS"); | ||
| context.init(null, new TrustManager[]{new NoCheckTrustManager()}, new java.security.SecureRandom()); | ||
| HttpsURLConnection.setDefaultSSLSocketFactory(context.getSocketFactory()); | ||
| // bypass host name check, too. | ||
| HttpsURLConnection.setDefaultHostnameVerifier(new NoCheckHostnameVerifier()); |
There was a problem hiding this comment.
This entrypoint set the default SSL socket factory and the default hostname verifier JVM-wide, not just for the HTTP connections made by Remoting. The other entrypoint only set these values for the HTTP connections made by Remoting. Whenever dealing with a matter of security where each entrypoint had different behavior, we always chose the more secure option. Thus, we prefer the behavior of the other entrypoint which does not change JVM-wide behavior.
| if (agentJnlpCredentials != null) { | ||
| jnlpArgs.add("-credentials"); | ||
| jnlpArgs.add(agentJnlpCredentials); | ||
| } |
There was a problem hiding this comment.
This code is actually being kept, just moved next to its siblings for legibility rather than in this random place.
| @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "Parameter supplied by user / administrator.") | ||
| private Engine createEngine() { | ||
| LOGGER.log(Level.INFO, "Setting up agent: {0}", name); | ||
| Engine engine = new Engine( | ||
| new CuiListener(), | ||
| urls, | ||
| secret, | ||
| name, | ||
| directConnection, | ||
| instanceIdentity, | ||
| new HashSet<>(protocols)); | ||
| engine.setWebSocket(webSocket); | ||
| if (webSocketHeaders != null) { | ||
| engine.setWebSocketHeaders(webSocketHeaders); | ||
| } | ||
| if (tunnel != null) { | ||
| engine.setTunnel(tunnel); | ||
| } | ||
| if (agentJnlpCredentials != null) { | ||
| engine.setCredentials(agentJnlpCredentials); | ||
| } | ||
| if (proxyCredentials != null) { | ||
| engine.setProxyCredentials(proxyCredentials); | ||
| } | ||
| if (jarCache != null) { | ||
| engine.setJarCache(new FileSystemJarCache(jarCache, true)); | ||
| } | ||
| engine.setNoReconnect(noReconnect); | ||
| engine.setKeepAlive(!noKeepAlive); | ||
|
|
||
| if (noCertificateCheck) { | ||
| LOGGER.log(Level.WARNING, "Certificate validation for HTTPs endpoints is disabled"); | ||
| } | ||
| engine.setDisableHttpsCertValidation(noCertificateCheck); | ||
|
|
||
| // TODO: ideally logging should be initialized before the "Setting up agent" entry | ||
| if (agentLog != null) { | ||
| try { | ||
| engine.setAgentLog(PathUtils.fileToPath(agentLog)); | ||
| } catch (IOException ex) { | ||
| throw new IllegalStateException("Cannot retrieve custom log destination", ex); | ||
| } | ||
| } | ||
| if (loggingConfigFilePath != null) { | ||
| try { | ||
| engine.setLoggingConfigFile(PathUtils.fileToPath(loggingConfigFilePath)); | ||
| } catch (IOException ex) { | ||
| throw new IllegalStateException("Logging config file is invalid", ex); | ||
| } | ||
| } | ||
|
|
||
| if (x509Certificates != null && !x509Certificates.isEmpty()) { | ||
| engine.setCandidateCertificates(x509Certificates); | ||
| } | ||
|
|
||
| // Working directory settings | ||
| if (workDir != null) { | ||
| try { | ||
| engine.setWorkDir(PathUtils.fileToPath(workDir)); | ||
| } catch (IOException ex) { | ||
| throw new IllegalStateException("Work directory path is invalid", ex); | ||
| } | ||
| } | ||
| engine.setInternalDir(internalDir); | ||
| engine.setFailIfWorkDirIsMissing(failIfWorkDirIsMissing); | ||
|
|
||
| return engine; | ||
| } | ||
|
|
||
| /** | ||
| * {@link EngineListener} implementation that sends output to {@link Logger}. | ||
| */ | ||
| private static final class CuiListener implements EngineListener { | ||
| @Override | ||
| public void status(String msg, Throwable t) { | ||
| LOGGER.log(Level.INFO, msg, t); | ||
| } | ||
|
|
||
| @Override | ||
| public void status(String msg) { | ||
| status(msg, null); | ||
| } | ||
|
|
||
| @Override | ||
| @SuppressFBWarnings( | ||
| value = "DM_EXIT", | ||
| justification = "Yes, we really want to exit in the case of severe error") | ||
| public void error(Throwable t) { | ||
| LOGGER.log(Level.SEVERE, t.getMessage(), t); | ||
| System.exit(-1); | ||
| } | ||
|
|
||
| @Override | ||
| public void onDisconnect() {} | ||
|
|
||
| @Override | ||
| public void onReconnect() {} | ||
| } | ||
|
|
There was a problem hiding this comment.
Not original; moved from the other entrypoint.
| @@ -100,63 +91,6 @@ static String indent(String s) { | |||
| return " " + s.trim().replace("\n", "\n "); | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Fret not that these public methods are being deleted, as this whole class is @Restricted(NoExternalUse.class). Anyway, these were all duplicate code. Keeping the more widely used copy.
| @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD", justification = "Used by the agent for retrieving connection info from the server.") | ||
| static URLConnection openURLConnection(URL url, String credentials, String proxyCredentials, | ||
| public static URLConnection openURLConnection(URL url, String credentials, String proxyCredentials, |
There was a problem hiding this comment.
The more widely used copy of the utility code deleted from the Util class. Visibility is being increased so that this code can be reused.
| //FIXME: Is it really required in this path? Seems like a bug | ||
| httpsConnection.setHostnameVerifier(new NoCheckHostnameVerifier()); |
There was a problem hiding this comment.
As before, whenever it comes to matters of security, we always err on the side of the more restrictive option when unifying the two implementations. Previously, this bug existed in the second entrypoint but not the first (i.e., the second entrypoint was more loose). So if invoking the first entrypoint (which itself invokes the second entrypoint), you got the more restrictive behavior, and if invoking the second entrypoint directly you got the loose behavior. Now that the entrypoints are being unified, I had to choose which behavior to follow. Rather than risk introducing a security issue, I choose the more restrictive behavior. By the way, this can be turned off with -noCertificateCheck (I tested this) and these semantics exactly match Swarm, where there have been no complaints, so I think this is a reasonable change in behavior in a very minor edge case.
| } | ||
| } | ||
|
|
||
| private void bootstrapInboundAgent() throws CmdLineException, IOException, InterruptedException { |
There was a problem hiding this comment.
In the long term I think this mode should cease to exist
Agreed; see jenkinsci/docker-swarm-plugin#125 KostyaSha/yet-another-docker-plugin#288 (comment) etc.
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
|
I did some more testing with WebSocket, a self-signed certificate, and a local Squid forwarding proxy. All worked fine except for proxy authentication, but no matter which combination of entrypoints and arguments I tried, I couldn't get that to work in the original trunk code either before this PR. I did come up with a fix for proxy authentication, but I will leave that to a separate PR. |
| System.err.println("Most likely a configuration error in the controller"); | ||
| System.err.println(e.getMessage()); | ||
| System.exit(1); | ||
| if (agentJnlpURL != null || !urls.isEmpty()) { |
There was a problem hiding this comment.
I think this also needs to cover directConnection, where neither -jnlpUrl nor -url is used yet we are running an inbound agent. jenkinsci/kubernetes-plugin#1451 (comment) ⇒ #689
There was a problem hiding this comment.
Indeed, somehow I missed testing this mode during all the other manual testing I did. Thanks for fixing this so quickly.
Problem
While reading this code recently, I found it quite confusing that there were two entrypoints:
java -jar remoting.jarandjava -cp remoting.jar hudson.remoting.jnlp.Main. The entrypoint code is a bit messy on both the interface level and the implementation level. There are two unique entrypoints, with quite similar APIs that overlap much of the time but also differ in some minor ways. This is quite confusing; for example, why should the argument be called-noreconnectin one entrypoint and-noReconnectin another, or why should the secret be specified via-secretin one entrypoint and with a positional argument in the other? Similarly, reading the implementation is quite confusing, as in some cases (but not all) the first implementation vectors into the second. So there is not only duplicate code, but also the logic is confusing to follow because sometimes only one implementation is being used, but sometimes two implementations are being used.Solution
Without breaking compatibility, this PR unifies the two entrypoints into a single implementation, with
Mainnow delegating toLauncher, which now supports both its original semantics and that ofMain. While this entails quite a few aliases, the idea is that once all usages are going through theLauncherentrypoint, we can also take the opportunity to standardize on the desired semantics (e.g.,-noReconnectinstead of-noreconnect,-credentialsinstead of-jnlpCredentials, etc). The idea is that once this PR is widely adopted, I will go through and update usages to standardize on a common set of calling conventions; for example, named arguments rather than positional arguments, camel case whenever possible, etc. More than just cleaning up the API, this also cleans up the implementations by merging them into one class that is easier to read and understand because the execution is all in one file and can be followed linearly rather than trying to understand what happens when one main method vectors into the other.Testing done
Apart from
mvn clean verify, I've done some light testing against my local controller with bothjava -jar remoting.jarandjava -cp remoting.jar hudson.remoting.jnlp.Main. I also tested Swarm with these changes and tested that the-noCertificateCheckoption could be used to connect to a TLS-based Jenkins controller with a self-signed certificate on my local machine. I also ran the core test suite and PCT with these changes.