Allow disabling HTTPs cert validation of JNLP endpoint when starting Remoting from Main #210
Conversation
|
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. |
| @@ -242,6 +251,12 @@ public Engine createEngine() { | |||
| engine.setJarCache(new FileSystemJarCache(jarCache,true)); | |||
| engine.setNoReconnect(noReconnect); | |||
| engine.setKeepAlive(!noKeepAlive); | |||
|
|
|||
| if (disableHttpsCertValidation) { | |||
| LOGGER.log(WARNING, "Certificate validation for HTTPs endpoints is disabled"); | |||
There was a problem hiding this comment.
Could be nice to make the rest of the calls also consistent. In that class there are currently 3 ways to create logs : LOGGER.fine(...), LOGGER.log(INFO,..., and LOGGER.log(Level.WARNING,...
There was a problem hiding this comment.
Let's do refactoring as a follow-up
| httpsConnection.setHostnameVerifier(new NoCheckHostnameVerifier()); | ||
| httpsConnection.setSSLSocketFactory(sslSocketFactory); | ||
| } catch (KeyManagementException | NoSuchAlgorithmException ex) { | ||
| System.err.println(String.format("Error setting disableHttpsCertValidation; %s", ex.getMessage())); |
There was a problem hiding this comment.
Is there a reason to use System.err here instead of LOGGER ?
| ((HttpsURLConnection) con).setSSLSocketFactory(sslSocketFactory); | ||
|
|
||
| if (con instanceof HttpsURLConnection) { | ||
| final HttpsURLConnection httpsConnection = (HttpsURLConnection) con; |
There was a problem hiding this comment.
I think the final does not add value here
There was a problem hiding this comment.
It prevents somebody from modifying a field in the logic just in case
| if (con instanceof HttpsURLConnection) { | ||
| final HttpsURLConnection httpsConnection = (HttpsURLConnection) con; | ||
| if (insecure) { | ||
| System.out.println(String.format("Insecure Status: %s", insecure)); |
There was a problem hiding this comment.
The use of LOGGER.fine / info should be preferred no ?
There was a problem hiding this comment.
I think so, but the file is full of stdout/stderr. @MattLud has followed the current implementation. I think it makes sense to stick to it for a while, but I will create a follow-up ticket to rework it
|
@reviewbybees done. Will resolve the merge conflict and release |
|
I decided to exclude it from the 3.14 release, because I want to do more evaluation + maybe add documentation for this flag. |
| @@ -345,6 +351,10 @@ public void run() throws Exception { | |||
| jnlpArgs.add(c); | |||
| } | |||
| } | |||
| if (noCertificateCheck) { | |||
| //TODO: Rename option? | |||
|
I tested it manually on the weekend. LGTM, will merge once the CI passes |
|
Merging without squash, no plans to backport it |
It is an attempt to improve #205 and to reuse existing code from @stephenc . Originally the HTTPs certificate validation has been done in
hudson.remoting.Launcher, but it was missing inhudson.remoting.jnlp.Main. So it was not possible to start a Java Web Start agent in the insecure mode. In #205 @MattLud needs that.How does it work now?
-disableHttpsCertValidationis passed fromMain, the JNLP Endpoint HTTPs cert verification is disabled.-noCertificateCheckis passedI also did some refactoring in order to prevent code duplication.
@reviewbybees @MattLud @Wadeck