Add insecure flag to allow bypassing SSL hostname verification and cert checks #205
Conversation
|
|
||
|
|
||
|
|
||
| public boolean isInsecure() { |
There was a problem hiding this comment.
Methods should not be listed before fields. It will confuse other contributors
There was a problem hiding this comment.
New API methods also should be documented
| @@ -80,6 +89,16 @@ | |||
|
|
|||
| private String tunnel; | |||
|
|
|||
| private boolean insecure; | |||
|
|
|||
| public boolean isInsecure() { | |||
| @@ -141,7 +160,7 @@ public void setTunnel(String tunnel) { | |||
| public JnlpAgentEndpoint resolve() throws IOException { | |||
| IOException firstError = null; | |||
| for (String jenkinsUrl : jenkinsUrls) { | |||
|
|
|||
There was a problem hiding this comment.
Please do not do unrelated whitespace changes. It may cause undesired merge conflicts
|
Ok - repushed with amended commit. |
| @@ -156,6 +153,23 @@ public void run() { | |||
|
|
|||
|
|
|||
|
|
|||
| /** | |||
| * Determines if JNLPAgentEndpointResolver will not perform certificate validation | |||
| * @return | |||
There was a problem hiding this comment.
Please do not keep empty javadoc tags.
@since TODO would be useful
| * Sets if JNLPAgentEndpointResolver will not perform certificate validation | ||
| * | ||
| * @param insecure | ||
| */ |
There was a problem hiding this comment.
same, also strange line breaks
| @@ -92,6 +89,10 @@ | |||
| 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.") | |||
There was a problem hiding this comment.
Exactly the type of comment I would like to see more often on such option :) 👍
| * @return if insecure, endpoint check is ignored | ||
| */ | ||
|
|
||
| public boolean isInsecure() { |
| if (con instanceof HttpsURLConnection && sslSocketFactory != null) { | ||
|
|
||
| if (insecure && con instanceof HttpsURLConnection) { | ||
| System.out.println(String.format("Insecure Status: %s", insecure)); |
There was a problem hiding this comment.
Better to use a logger instead of System.out.println.
There was a problem hiding this comment.
It requires a wider refactoring, the option is being used in other places
| }; | ||
|
|
||
| ((HttpsURLConnection) con).setHostnameVerifier(allHostsValid); | ||
| ((HttpsURLConnection) con).setSSLSocketFactory(sslSocketFactory); |
There was a problem hiding this comment.
Double casting could be avoided by using a local variable just above like
HttpsURLConnection httpsCon = (HttpsURLConnection) con;
httpsCon.setHostnameVerifier(allHostsValid);
httpsCon.setSSLSocketFactory(sslSocketFactory);
| return true; | ||
| } | ||
| }; | ||
| ((HttpsURLConnection) con).setHostnameVerifier(allHostsValid); |
|
Ok - is there any change you need from me right now?
…On Fri, Nov 3, 2017 at 8:13 AM, Oleg Nenashev ***@***.***> wrote:
***@***.**** requested changes on this pull request.
After some deeper review I cannot accept this implementation. I will
create a follow-up PR with some refactoring and comments
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#205 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAUwfZjCJQ09Dz4ZfiziC1NQ-cQjl_pLks5syxFpgaJpZM4P85w9>
.
|
Initial proposal - Let me know what sort of unit tests you'd like to see or if you want the whitespace changes reverted.