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

Move initialization to common method for Swarm #693

Merged
merged 4 commits into from
Oct 25, 2023
Merged

Move initialization to common method for Swarm #693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6e29716
Move initialization to common method for Swarm
basil Oct 24, 2023
b2c5750
Remove `initialized` variable, since Swarm can call `Launcher#main`
basil Oct 24, 2023
d03903b
Remove redundant initialization
basil Oct 24, 2023
219d536
Out of paranoia, guard against accidental double initialization.
basil Oct 24, 2023
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
19 changes: 14 additions & 5 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 @@ -67,6 +67,7 @@
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.stream.Collectors;
import javax.net.ssl.HostnameVerifier;
import javax.net.ssl.KeyManagerFactory;
import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLSocketFactory;
Expand Down Expand Up @@ -187,6 +188,9 @@ public Thread newThread(@NonNull final Runnable r) {

private boolean disableHttpsCertValidation = false;

@CheckForNull
private HostnameVerifier hostnameVerifier;
Comment on lines +191 to +192
Copy link
Member Author

Choose a reason for hiding this comment

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

Following a general paradigm where everytime a boolean field exists to disable validation, a HostNameVerifier field exists beneath it. Following this convention consistently in all classes that do this allows for certain common utilities like JnlpAgentEndpointResolver#openURLConnection to be simplified.


private boolean noReconnect = false;

/**
Expand Down Expand Up @@ -424,6 +428,11 @@ public boolean isDisableHttpsCertValidation() {
*/
public void setDisableHttpsCertValidation(boolean disableHttpsCertValidation) {
this.disableHttpsCertValidation = disableHttpsCertValidation;
if (disableHttpsCertValidation) {
this.hostnameVerifier = new NoCheckHostnameVerifier();
} else {
this.hostnameVerifier = null;
}
}

/**
Expand Down Expand Up @@ -713,8 +722,8 @@ public void closeRead() throws IOException {
SSLContext sslContext = getSSLContext(candidateCertificates, disableHttpsCertValidation);
if (sslContext != null) {
SslEngineConfigurator sslEngineConfigurator = new SslEngineConfigurator(sslContext);
if (disableHttpsCertValidation) {
sslEngineConfigurator.setHostnameVerifier(new NoCheckHostnameVerifier());
if (hostnameVerifier != null) {
sslEngineConfigurator.setHostnameVerifier(hostnameVerifier);
}
client.getProperties().put(ClientProperties.SSL_ENGINE_CONFIGURATOR, sslEngineConfigurator);
}
Expand Down Expand Up @@ -928,8 +937,8 @@ private JnlpEndpointResolver createEndpointResolver(List<String> jenkinsUrls, St
} catch (Exception e) {
events.error(e);
}
resolver = new JnlpAgentEndpointResolver(jenkinsUrls, credentials, proxyCredentials, tunnel,
sslSocketFactory, disableHttpsCertValidation, agentName);
resolver = new JnlpAgentEndpointResolver(jenkinsUrls, agentName, credentials, proxyCredentials, tunnel,
Copy link
Member Author

Choose a reason for hiding this comment

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

Following a general paradigm where user-specified flags come before computed objects and mandatory arguments come before optional arguments. Using a consistent order helps improve readability as you go from one file to the next, which otherwise used different ordering which was confusing.

sslSocketFactory, disableHttpsCertValidation);
} else {
resolver = new JnlpAgentEndpointConfigurator(directConnection, instanceIdentity, protocols, proxyCredentials);
}
Expand Down Expand Up @@ -1107,7 +1116,7 @@ static SSLSocketFactory getSSLSocketFactory(List<X509Certificate> x509Certificat
throws PrivilegedActionException, KeyStoreException, NoSuchProviderException, CertificateException,
NoSuchAlgorithmException, IOException, KeyManagementException {
SSLContext sslContext = getSSLContext(x509Certificates, noCertificateCheck);
return sslContext == null ? null : sslContext.getSocketFactory();
return sslContext != null ? sslContext.getSocketFactory() : null;
}

/**
Expand Down
49 changes: 35 additions & 14 deletions src/main/java/hudson/remoting/Launcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.jenkinsci.remoting.engine.JnlpAgentEndpointResolver;
import org.jenkinsci.remoting.engine.WorkDirManager;
import org.jenkinsci.remoting.util.PathUtils;
import org.jenkinsci.remoting.util.https.NoCheckHostnameVerifier;
import org.kohsuke.args4j.Argument;
import org.kohsuke.args4j.CmdLineException;
import org.kohsuke.args4j.CmdLineParser;
Expand All @@ -41,6 +42,7 @@
import javax.crypto.Cipher;
import javax.crypto.spec.IvParameterSpec;
import javax.crypto.spec.SecretKeySpec;
import javax.net.ssl.HostnameVerifier;
import javax.net.ssl.SSLHandshakeException;
import javax.net.ssl.SSLSocketFactory;
import javax.xml.parsers.DocumentBuilder;
Expand Down Expand Up @@ -204,6 +206,8 @@
@Option(name="-noCertificateCheck", aliases = "-disableHttpsCertValidation", forbids = "-cert", usage="Ignore SSL validation errors - use as a last resort only.")
public boolean noCertificateCheck = false;

private HostnameVerifier hostnameVerifier;

public InetSocketAddress connectionTarget = null;

@Option(name="-connectTo",usage="make a TCP connection to the given host and port, then start communication.",metaVar="HOST:PORT")
Expand Down Expand Up @@ -361,6 +365,8 @@
@Deprecated
public List<String> args = new ArrayList<>();

private boolean initialized;

public static void main(String... args) throws IOException, InterruptedException {
Launcher launcher = new Launcher();
CmdLineParser parser = new CmdLineParser(launcher);
Expand Down Expand Up @@ -390,8 +396,31 @@
return;
}

if (connectionTarget != null) {
initialize();
runAsTcpClient();
} else if (agentJnlpURL != null || !urls.isEmpty() || directConnection != null) {
Copy link
Member

@jglick jglick Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Fortunately this now has test coverage! (Though jenkinsci/bom#2417 is of no help since the relevant tests require special setup to run. And anyway the change needs to propagate through remoting → docker-agent → docker-inbound-agent → kubernetes-plugin rather than the normal Maven dependency chain.)

if (agentJnlpURL != null) {
bootstrapInboundAgent(); // calls initialize() internally
} else {
initialize();
}
runAsInboundAgent();
} else if (tcpPortFile != null) {
initialize();
runAsTcpServer();
} else {
initialize();
runWithStdinStdout();
}
}

private synchronized void initialize() throws IOException {
if (initialized) {
throw new IllegalStateException("double initialization");
}
// Create and verify working directory and logging
// TODO: The pass-through for the JNLP mode has been added in JENKINS-39817. But we still need to keep this parameter in

Check warning on line 423 in src/main/java/hudson/remoting/Launcher.java

View check run for this annotation

ci.jenkins.io / Open Tasks Scanner

TODO 

NORMAL:
The pass-through for the JNLP mode has been added in JENKINS-39817. But we still need to keep this parameter in
// consideration for other modes (TcpServer, TcpClient, etc.) to retain the legacy behavior.
// On the other hand, in such case there is no need to invoke WorkDirManager and handle the double initialization logic
final WorkDirManager workDirManager = WorkDirManager.getInstance();
Expand All @@ -404,26 +433,17 @@
}
workDirManager.setupLogging(internalDirPath, agentLog != null ? PathUtils.fileToPath(agentLog) : null);

// Initialize certificates
createX509Certificates();
try {
sslSocketFactory = Engine.getSSLSocketFactory(x509Certificates, noCertificateCheck);
} catch (GeneralSecurityException | PrivilegedActionException e) {
throw new RuntimeException(e);
}
if(connectionTarget!=null) {
runAsTcpClient();
} else
if (agentJnlpURL != null || !urls.isEmpty() || directConnection != null) {
if (agentJnlpURL != null) {
bootstrapInboundAgent();
}
runAsInboundAgent();
} else
if(tcpPortFile!=null) {
runAsTcpServer();
} else {
runWithStdinStdout();
if (noCertificateCheck) {
hostnameVerifier = new NoCheckHostnameVerifier();
Comment on lines +443 to +444
Copy link
Member Author

Choose a reason for hiding this comment

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

Initializing this eagerly rather than lazily for consistency with other code. Of course, this necessitates that all public methods in this file now call initialization, which is why I factored out initialization into its own method.

}
initialized = true;
}

@SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "Parameter supplied by user / administrator.")
Expand Down Expand Up @@ -624,6 +644,7 @@
*/
@SuppressFBWarnings(value = {"CIPHER_INTEGRITY", "STATIC_IV"}, justification = "Integrity not needed here. IV used for decryption only, loaded from encryptor.")
public List<String> parseJnlpArguments() throws ParserConfigurationException, SAXException, IOException, InterruptedException {
initialize(); // For Swarm, or anyone else who invokes this public method directly rather than going through main() or run()
Copy link
Member Author

Choose a reason for hiding this comment

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

With lazy initialization of the noCertificateCheck functionality, this was working, but only by accident. Now, we explicitly do all initialization in one place, and from every entrypoint.

if (secret != null) {
agentJnlpURL = new URL(agentJnlpURL + "?encrypt=true");
if (agentJnlpCredentials != null) {
Expand All @@ -633,7 +654,7 @@
while (true) {
URLConnection con = null;
try {
con = JnlpAgentEndpointResolver.openURLConnection(agentJnlpURL, agentJnlpCredentials, proxyCredentials, sslSocketFactory, noCertificateCheck, null);
con = JnlpAgentEndpointResolver.openURLConnection(agentJnlpURL, null, agentJnlpCredentials, proxyCredentials, sslSocketFactory, hostnameVerifier);
Copy link
Member Author

Choose a reason for hiding this comment

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

As elsewhere, putting mandatory user-specified arguments before optional ones, and putting user-specified arguments before computed things like SSL socket factories and hostname verifiers.

con.connect();

if (con instanceof HttpURLConnection) {
Expand Down
47 changes: 29 additions & 18 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 @@ -34,6 +34,7 @@
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

import javax.net.ssl.HostnameVerifier;
import javax.net.ssl.HttpsURLConnection;
import javax.net.ssl.SSLSocketFactory;
import java.io.IOException;
Expand Down Expand Up @@ -85,17 +86,19 @@ public class JnlpAgentEndpointResolver extends JnlpEndpointResolver {
@NonNull
private final List<String> jenkinsUrls;

private SSLSocketFactory sslSocketFactory;
private final String agentName;

private String credentials;

private String proxyCredentials;

private String tunnel;

private SSLSocketFactory sslSocketFactory;

private boolean disableHttpsCertValidation;

private final String agentName;
private HostnameVerifier hostnameVerifier;
Comment on lines +89 to +101
Copy link
Member Author

Choose a reason for hiding this comment

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

As elsewhere, putting mandatory user-specified arguments before optional ones, and putting user-specified arguments before computed things like SSL socket factories and hostname verifiers.


/**
* If specified, only the protocols from the list will be tried during the connection.
Expand All @@ -106,15 +109,15 @@ public class JnlpAgentEndpointResolver extends JnlpEndpointResolver {
private static String PROTOCOL_NAMES_TO_TRY =
System.getProperty(JnlpAgentEndpointResolver.class.getName() + ".protocolNamesToTry");

public JnlpAgentEndpointResolver(@NonNull List<String> jenkinsUrls, String credentials, String proxyCredentials,
String tunnel, SSLSocketFactory sslSocketFactory, boolean disableHttpsCertValidation, String agentName) {
public JnlpAgentEndpointResolver(@NonNull List<String> jenkinsUrls, String agentName, String credentials, String proxyCredentials,
String tunnel, SSLSocketFactory sslSocketFactory, boolean disableHttpsCertValidation) {
this.jenkinsUrls = new ArrayList<>(jenkinsUrls);
this.agentName = agentName;
this.credentials = credentials;
this.proxyCredentials = proxyCredentials;
this.tunnel = tunnel;
this.sslSocketFactory = sslSocketFactory;
this.disableHttpsCertValidation = disableHttpsCertValidation;
this.agentName = agentName;
setDisableHttpsCertValidation(disableHttpsCertValidation);
}

public SSLSocketFactory getSslSocketFactory() {
Expand Down Expand Up @@ -175,6 +178,11 @@ public boolean isDisableHttpsCertValidation() {
*/
public void setDisableHttpsCertValidation(boolean disableHttpsCertValidation) {
this.disableHttpsCertValidation = disableHttpsCertValidation;
if (disableHttpsCertValidation) {
this.hostnameVerifier = new NoCheckHostnameVerifier();
} else {
this.hostnameVerifier = null;
}
}

@CheckForNull
Expand All @@ -198,7 +206,7 @@ public JnlpAgentEndpoint resolve() throws IOException {

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

HttpURLConnection con =
(HttpURLConnection) openURLConnection(url, credentials, proxyCredentials, sslSocketFactory, disableHttpsCertValidation, agentName);
(HttpURLConnection) openURLConnection(url, agentName, credentials, proxyCredentials, sslSocketFactory, hostnameVerifier);
con.setConnectTimeout(5000);
con.setReadTimeout(5000);
con.connect();
Expand Down Expand Up @@ -515,9 +523,14 @@ else if(entry.endsWith("*"))
*/
@Restricted(NoExternalUse.class)
@SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD", justification = "Used by the agent for retrieving connection info from the server.")
public static URLConnection openURLConnection(URL url, String credentials, String proxyCredentials,
SSLSocketFactory sslSocketFactory, boolean disableHttpsCertValidation,
@CheckForNull String agentName) throws IOException {
public static URLConnection openURLConnection(
URL url,
@CheckForNull String agentName,
@CheckForNull String credentials,
@CheckForNull String proxyCredentials,
@CheckForNull SSLSocketFactory sslSocketFactory,
@CheckForNull HostnameVerifier hostnameVerifier)
throws IOException {
String httpProxy = null;
// If http.proxyHost property exists, openConnection() uses it.
if (System.getProperty("http.proxyHost") == null) {
Expand All @@ -537,6 +550,9 @@ public static URLConnection openURLConnection(URL url, String credentials, Strin
} else {
con = url.openConnection();
}
if (agentName != null) {
con.setRequestProperty(JnlpConnectionState.CLIENT_NAME_KEY, agentName);
}
if (credentials != null) {
String encoding = Base64.getEncoder().encodeToString(credentials.getBytes(StandardCharsets.UTF_8));
con.setRequestProperty("Authorization", "Basic " + encoding);
Expand All @@ -545,18 +561,13 @@ public static URLConnection openURLConnection(URL url, String credentials, Strin
String encoding = Base64.getEncoder().encodeToString(proxyCredentials.getBytes(StandardCharsets.UTF_8));
con.setRequestProperty("Proxy-Authorization", "Basic " + encoding);
}
if (agentName != null) {
con.setRequestProperty(JnlpConnectionState.CLIENT_NAME_KEY, agentName);
}

if (con instanceof HttpsURLConnection) {
final HttpsURLConnection httpsConnection = (HttpsURLConnection) con;
if (sslSocketFactory != null) {
httpsConnection.setSSLSocketFactory(sslSocketFactory);
}
if (disableHttpsCertValidation) {
LOGGER.log(Level.WARNING, "HTTPs certificate check is disabled for the endpoint.");
httpsConnection.setHostnameVerifier(new NoCheckHostnameVerifier());
if (hostnameVerifier != null) {
httpsConnection.setHostnameVerifier(hostnameVerifier);
}
}
return con;
Expand Down
Loading