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.

Sign up for GitHub

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 insecure flag to allow bypassing SSL hostname verification and cert checks #205

Merged
merged 1 commit into from
Nov 20, 2017
Merged

Add insecure flag to allow bypassing SSL hostname verification and cert checks #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions src/main/java/hudson/remoting/Engine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,11 @@
*/
package hudson.remoting;

import edu.umd.cs.findbugs.annotations.Nullable;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.remoting.Channel.Mode;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.PrintStream;
import java.net.Socket;
import java.net.URL;
import java.nio.file.Path;
Expand Down Expand Up @@ -134,7 +130,6 @@ public void run() {
*/
@CheckForNull
private URL hudsonUrl;

private final String secretKey;
public final String slaveName;
private String credentials;
Expand All @@ -145,6 +140,8 @@ public void run() {
*/
private String tunnel;

private boolean insecure;

private boolean noReconnect;

/**
Expand All @@ -156,6 +153,23 @@ public void run() {



/**
* Determines if JNLPAgentEndpointResolver will not perform certificate validation
* @return
Copy link
Member

Choose a reason for hiding this comment

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

Please do not keep empty javadoc tags.
@since TODO would be useful

*/
public boolean isInsecure() {
return insecure;
}

/**
* Sets if JNLPAgentEndpointResolver will not perform certificate validation
*
* @param insecure
*/
Copy link
Member

Choose a reason for hiding this comment

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

same, also strange line breaks


public void setInsecure(boolean insecure) {
this.insecure = insecure;
}

@CheckForNull
private JarCache jarCache = null;
Expand Down Expand Up @@ -327,6 +341,8 @@ public void setNoReconnect(boolean noReconnect) {
this.noReconnect = noReconnect;
}



/**
* Sets the destination for agent logs.
* @param agentLog Path to the agent log.
Expand Down Expand Up @@ -482,6 +498,7 @@ private void innerRun(IOHub hub, SSLContext context, ExecutorService service) {
resolver.setTunnel(tunnel);
try {
resolver.setSslSocketFactory(getSSLSocketFactory());
resolver.setInsecure(insecure);
} catch (Exception e) {
events.error(e);
}
Expand Down Expand Up @@ -809,8 +826,8 @@ private SSLSocketFactory getSSLSocketFactory()
trustManagerFactory.init(keyStore);
// prepare the SSL context
SSLContext ctx = SSLContext.getInstance("TLS");
ctx.init(null, trustManagerFactory.getTrustManagers(), null);
// now we have our custom socket factory
ctx.init(null, trustManagerFactory.getTrustManagers(), null);
sslSocketFactory = ctx.getSocketFactory();
}
return sslSocketFactory;
Expand Down
10 changes: 7 additions & 3 deletions src/main/java/hudson/remoting/jnlp/Main.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,12 @@
import hudson.remoting.FileSystemJarCache;
import java.io.ByteArrayInputStream;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.UnsupportedEncodingException;
import java.security.cert.CertificateException;
import java.security.cert.CertificateFactory;
import java.security.cert.X509Certificate;

import org.jenkinsci.remoting.engine.WorkDirManager;
import org.jenkinsci.remoting.util.IOUtils;
import org.kohsuke.args4j.Option;
import org.kohsuke.args4j.CmdLineParser;
import org.kohsuke.args4j.Argument;
Expand All @@ -51,7 +49,6 @@

import hudson.remoting.Engine;
import hudson.remoting.EngineListener;
import java.nio.file.Path;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
Expand Down Expand Up @@ -92,6 +89,10 @@ public class Main {
usage="If the connection ends, don't retry and just exit.")
public boolean noReconnect = false;

@Option(name="-insecure",
usage="Ignore SSL validation errors - use as a last resort only.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly the type of comment I would like to see more often on such option :) 👍

public boolean insecure = false;

@Option(name="-noKeepAlive",
usage="Disable TCP socket keep alive on connection to the master.")
public boolean noKeepAlive = false;
Expand Down Expand Up @@ -242,6 +243,9 @@ public Engine createEngine() {
engine.setJarCache(new FileSystemJarCache(jarCache,true));
engine.setNoReconnect(noReconnect);
engine.setKeepAlive(!noKeepAlive);
LOGGER.log(INFO, "Insecure Status: {0}", insecure);
engine.setInsecure(insecure);


// TODO: ideally logging should be initialized before the "Setting up slave" entry
if (agentLog != null) {
Expand Down
85 changes: 80 additions & 5 deletions src/main/java/org/jenkinsci/remoting/engine/JnlpAgentEndpointResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,10 @@
import java.net.URL;
import java.net.URLConnection;
import java.security.KeyFactory;
import java.security.KeyManagementException;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.security.cert.X509Certificate;
import java.security.interfaces.RSAPublicKey;
import java.security.spec.InvalidKeySpecException;
import java.security.spec.X509EncodedKeySpec;
Expand All @@ -58,6 +61,12 @@
import javax.net.ssl.HttpsURLConnection;
import javax.net.ssl.SSLSocketFactory;

import javax.net.ssl.HostnameVerifier;
import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLSession;
import javax.net.ssl.TrustManager;
import javax.net.ssl.X509TrustManager;

import static java.util.logging.Level.INFO;
import static org.jenkinsci.remoting.util.ThrowableUtils.chain;

Expand All @@ -80,6 +89,8 @@ public class JnlpAgentEndpointResolver {

private String tunnel;

private boolean insecure;

/**
* If specified, only the protocols from the list will be tried during the connection.
* The option provides protocol names, but the order of the check is defined internally and cannot be changed.
Expand Down Expand Up @@ -137,6 +148,25 @@ public void setTunnel(String tunnel) {
this.tunnel = tunnel;
}

/**
* Determine if certificate checking should be ignored for JNLP endpoint
*
* @return if insecure, endpoint check is ignored
*/

public boolean isInsecure() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird line break +1

return insecure;
}

/**
* Sets if insecure mode of endpoint should be used.
*
* @param insecure
*/
public void setInsecure(boolean insecure) {
this.insecure = insecure;
}

@CheckForNull
public JnlpAgentEndpoint resolve() throws IOException {
IOException firstError = null;
Expand All @@ -154,7 +184,7 @@ public JnlpAgentEndpoint resolve() throws IOException {

// find out the TCP port
HttpURLConnection con =
(HttpURLConnection) openURLConnection(salURL, credentials, proxyCredentials, sslSocketFactory);
(HttpURLConnection) openURLConnection(salURL, credentials, proxyCredentials, sslSocketFactory, insecure);
try {
try {
con.setConnectTimeout(30000);
Expand Down Expand Up @@ -310,7 +340,7 @@ public void waitForReady() throws InterruptedException {
t.setName(oldName + ": trying " + url + " for " + retries + " times");

HttpURLConnection con =
(HttpURLConnection) openURLConnection(url, credentials, proxyCredentials, sslSocketFactory);
(HttpURLConnection) openURLConnection(url, credentials, proxyCredentials, sslSocketFactory, insecure);
con.setConnectTimeout(5000);
con.setReadTimeout(5000);
con.connect();
Expand All @@ -331,7 +361,6 @@ public void waitForReady() throws InterruptedException {
} finally {
t.setName(oldName);
}

}

@CheckForNull
Expand Down Expand Up @@ -378,7 +407,7 @@ static InetSocketAddress getResolvedHttpProxyAddress(@Nonnull String host, int p
* Credentials can be passed e.g. to support running Jenkins behind a (reverse) proxy requiring authorization
*/
static URLConnection openURLConnection(URL url, String credentials, String proxyCredentials,
SSLSocketFactory sslSocketFactory) throws IOException {
SSLSocketFactory sslSocketFactory, boolean insecure) throws IOException {
String httpProxy = null;
// If http.proxyHost property exists, openConnection() uses it.
if (System.getProperty("http.proxyHost") == null) {
Expand Down Expand Up @@ -407,8 +436,54 @@ static URLConnection openURLConnection(URL url, String credentials, String proxy
String encoding = Base64.encode(proxyCredentials.getBytes("UTF-8"));
con.setRequestProperty("Proxy-Authorization", "Basic " + encoding);
}
if (con instanceof HttpsURLConnection && sslSocketFactory != null) {

if (insecure && con instanceof HttpsURLConnection) {
System.out.println(String.format("Insecure Status: %s", insecure));
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to use a logger instead of System.out.println.

Copy link
Member

Choose a reason for hiding this comment

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

It requires a wider refactoring, the option is being used in other places

try {
SSLContext ctx = SSLContext.getInstance("TLS");

ctx.init(null, new TrustManager[]{new X509TrustManager() {
@Override
public X509Certificate[] getAcceptedIssuers() {
return null;
}

@Override
public void checkClientTrusted(X509Certificate[] certs,
String authType) {
}

@Override
public void checkServerTrusted(X509Certificate[] certs,
String authType) {
}

}}, new SecureRandom());
sslSocketFactory = ctx.getSocketFactory();

HostnameVerifier allHostsValid = new HostnameVerifier() {
@Override
public boolean verify(String hostname, SSLSession session) {
return true;
}
};

((HttpsURLConnection) con).setHostnameVerifier(allHostsValid);
((HttpsURLConnection) con).setSSLSocketFactory(sslSocketFactory);
Copy link
Contributor

Choose a reason for hiding this comment

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

Double casting could be avoided by using a local variable just above like

HttpsURLConnection httpsCon = (HttpsURLConnection) con;
httpsCon.setHostnameVerifier(allHostsValid);
httpsCon.setSSLSocketFactory(sslSocketFactory);

} catch (KeyManagementException | NoSuchAlgorithmException ex) {
System.err.println(String.format("Error setting insecure; %s", ex.getMessage()));
}

}
else if (con instanceof HttpsURLConnection && sslSocketFactory != null) {
((HttpsURLConnection) con).setSSLSocketFactory(sslSocketFactory);
HostnameVerifier allHostsValid = new HostnameVerifier() {
@Override
public boolean verify(String hostname, SSLSession session) {
return true;
}
};
((HttpsURLConnection) con).setHostnameVerifier(allHostsValid);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto double cast

}
return con;
}
Expand Down