Jenkins plugin has TLS server hostname checking disabled?

[smarterclayton]

The default config had it set to off, which is bad. Also, it looked like SSHD was on - that's also bad.

[gabemontero]

@smarterclayton I may need a clarification, but I'm not seeing the first piece. By default, the jenkins plugin pulls the cert mounted in the pod, or explicitly configured in the job step, creates a Java X509Certificate, loads it in the Java trust store, and enables the openshift-restclient-java component with that trust store such that all REST invocations are validated. You must explicitly set a non default SKIP_TLS env var to disable things.

What exactly are you looking at?

As to SSHD, I'm assuming that is an openshift jenkins image statement. I'm unfamiliar with what is going on there. @bparees - can you assist?

[bparees]

we don't do anything w/ sshd in the jenkins image, and i don't see how it could be started since we just start the jenkins process, so unless jenkins magically starts sshd, it wouldn't be running.

[gabemontero]

thanks @bparees

back to the tls hostname thing .... i think i see it now (though may not be 100% sure what do do yet) ... in addition to all the trust store and certificate validation I did add, the openshift-restclient-java SSLCertificateCallback plug point has an allowHostname method. See https://github.com/openshift/jenkins-plugin/blob/master/src/main/java/com/openshift/jenkins/plugins/pipeline/Auth.java#L150-L154. I just mimicked what the jboss tooling did way back when and forgot about it. A google on "tls hostname verification" revealed plenty of hits like http://security.stackexchange.com/questions/61699/is-cn-hostname-verification-against-ssl-certificate-required-for-non-browser-sof

Assuming that is what @smarterclayton is referring to, I'll investigate what openshift-restclient-java returns to us and see about working up a sensible validation.

[smarterclayton]

There's a Java SSHD implementation that I think they use. It looked turned on in the default image.

I saw a checkbox in the Kubernetes plugin that says "skip TLS certificate verification" and it's checked by default.

On Fri, Jan 13, 2017 at 9:52 AM, Gabe Montero notifications@github.com wrote:

thanks @bparees https://github.com/bparees

back to the tls hostname thing .... i think i see it now (though may not be 100% sure what do do yet) ... in addition to all the trust store and certificate validation I did add, the openshift-restclient-java SSLCertificateCallback plug point has an allowHostname method. See https://github.com/openshift/jenkins-plugin/blob/master/ src/main/java/com/openshift/jenkins/plugins/pipeline/Auth.java#L150-L154. I just mimicked what the jboss tooling did way back when and forgot about it. A google on "tls hostname verification" revealed plenty of hits like http://security.stackexchange.com/questions/61699/is-cn- hostname-verification-against-ssl-certificate-required-for-non-browser-sof

Assuming that is what @smarterclayton https://github.com/smarterclayton is referring to, I'll investigate what openshift-restclient-java returns to us and see about working up a sensible validation.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openshift/origin/issues/12476#issuecomment-272461025, or mute the thread https://github.com/notifications/unsubscribe-auth/ABG_py2ohn9Q0QCAp6Y0ogwKVWjzXQE-ks5rR4-kgaJpZM4Lih7o .

[jim-minter]

@smarterclayton to be precise, do you mean the text "Disable https certificate check"?

This is configured at https://github.com/openshift/jenkins/blob/master/2/contrib/jenkins/kube-slave-common.sh#L144 (and https://github.com/openshift/jenkins/blob/master/1/contrib/jenkins/kube-slave-common.sh#L144)

[gabemontero]

AHHHHH ... thanks for the clarification @smarterclayton.

On the kubernetes plugin skip tls cert verification checkbox, to add to what @jim-minter found, if we turn tls cert check on, we'd have to define a credential that imports the openshift cert we mount and reference it. I don't think the cert id in that config stanza is our cert, but I'll dig a little and report back in a bit.

I'm still a curious about what if anything we should do with our plugin and allowHostname. We discussed a bit during devexp scrum but nothing conclusive arose. I'll still investigate per my earlier comment.

Lastly I haven't stumbled across java sshd in the jenkins code during my previous travels, but will make an explicit search and see what I can come up with.

[smarterclayton]

The cert should already be in the service account directory, why would't the kubernetes plugin use that automatically? No kubernetes cluster should ever need to turn off cert checking.

On Fri, Jan 13, 2017 at 11:07 AM, Gabe Montero notifications@github.com wrote:

AHHHHH ... thanks for the clarification @smarterclayton https://github.com/smarterclayton.

On the kubernetes plugin skip tls cert verification checkbox, to add to what @jim-minter https://github.com/jim-minter found, if we turn tls cert check on, we'd have to define a credential that imports the openshift cert we mount and reference it. I don't think the cert id in that config stanza is our cert, but I'll dig a little and report back in a bit.

I'm still a curious about what if anything we should do with our plugin and allowHostname. We discussed a bit during devexp scrum but nothing conclusive arose. I'll still investigate per my earlier comment.

Lastly I haven't stumbled across java sshd in the jenkins code during my previous travels, but will make an explicit search and see what I can come up with.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openshift/origin/issues/12476#issuecomment-272480260, or mute the thread https://github.com/notifications/unsubscribe-auth/ABG_p9pc9pR6d7JM-W55ACcnD2jso5nKks5rR6FGgaJpZM4Lih7o .

[gabemontero]

You'd think, but they don't provide a default like that. They make you explicitly configure the credential. Short term, we can provide the configuration so their code leverages what is available. Long term, we could supply a pull for them which does the defaulting automatically.

On Fri, Jan 13, 2017 at 11:09 AM, Clayton Coleman notifications@github.com wrote:

The cert should already be in the service account directory, why would't the kubernetes plugin use that automatically? No kubernetes cluster should ever need to turn off cert checking.

On Fri, Jan 13, 2017 at 11:07 AM, Gabe Montero notifications@github.com wrote:

AHHHHH ... thanks for the clarification @smarterclayton https://github.com/smarterclayton.

On the kubernetes plugin skip tls cert verification checkbox, to add to what @jim-minter https://github.com/jim-minter found, if we turn tls cert check on, we'd have to define a credential that imports the openshift cert we mount and reference it. I don't think the cert id in that config stanza is our cert, but I'll dig a little and report back in a bit.

I'm still a curious about what if anything we should do with our plugin and allowHostname. We discussed a bit during devexp scrum but nothing conclusive arose. I'll still investigate per my earlier comment.

Lastly I haven't stumbled across java sshd in the jenkins code during my previous travels, but will make an explicit search and see what I can come up with.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/openshift/origin/issues/12476#issuecomment-272480260 , or mute the thread https://github.com/notifications/unsubscribe-auth/ABG_p9pc9pR6d7JM- W55ACcnD2jso5nKks5rR6FGgaJpZM4Lih7o .

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/openshift/origin/issues/12476#issuecomment-272480642, or mute the thread https://github.com/notifications/unsubscribe-auth/ADbadBAntkAYHX-6n345xxFCHbeipo4eks5rR6GtgaJpZM4Lih7o .

[gabemontero]

OK ... the default credential associated with the jenkins image k8s cloud is for the kubernetes service account, but just the token.

For the cert, they don't use Jenkins Credentials. And they don't automatically pull the cert from the service account directory. They only support explicit configuration (i.e. you have to copy/paste the contents into the config key). So we can't set this during image construction.

Amended roadmap:

• short term: we could modify our plugin to fetch their cloud instance on start up, read the cert and modify their config to have it populated
• long term: if we want, submit a pull to their code to read both the service account cert and token by default

[bparees]

short term: we could modify our plugin to fetch their cloud instance on start up, read the cert and modify their config to have it populated

yes sounds like we should amend our jenkins startup script (the one that sets up the config.xml) to fetch and inject the cert when we're running in a pod.

[gabemontero]

Submitted the above jenkins image pull to update the k8s plugin config in the image;

on the alloweHostname front, based on what I found in fabric8 and jenkins master/slave in this space, @bparees and I agreed/decided to leave the jenkins-plugin as-is for now.

On sshd in jenkins, I did find an entry for it in the config, and does in fact seem to be enabled. I'll create a separate PR for updating the openshift jenkins image to disable that, have any further discussions needed on that, and note it in this issue.

When both https://github.com/openshift/jenkins PRs are resolved, I'll close this issue out.

[smarterclayton]

Looking at that code in the jenkins plugin... that looks completely wrong (insecure) in the same way "disable SSL hostname checking" is insecure

This code below is a security vulnerability in all places that it is used - any code that has it needs to be immediately fixed:

@Override
public boolean allowHostname(String hostname, SSLSession sslSession) {
//mimic SSLCertificateCallback implementation from jbosstools-openshift repo - we can noop
//also lines up with what was observed in k8s jennkins plugin
return true;

On Fri, Jan 13, 2017 at 3:27 PM, Gabe Montero notifications@github.com wrote:

Submitted the above jenkins image pull to update the k8s plugin config in the image;

on the alloweHostname front, based on what I found in fabric8 and jenkins master/slave in this space, @bparees https://github.com/bparees and I agreed/decided to leave the jenkins-plugin as-is for now.

On sshd in jenkins, I did find an entry for it in the config, and does in fact seem to be enabled. I'll create a separate PR for updating the openshift jenkins image to disable that, have any further discussions needed on that, and note it in this issue.

When both https://github.com/openshift/jenkins PRs are resolved, I'll close this issue out.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openshift/origin/issues/12476#issuecomment-272539457, or mute the thread https://github.com/notifications/unsubscribe-auth/ABG_p1rjvipGj96LwlqSovZnrMm9vxAtks5rR94jgaJpZM4Lih7o .

[smarterclayton]

@jcantrill we need to do a full audit of all java client code in use across our entire stack. Any place where we are overriding hostname verification (there are three or four ways to do this) is insecure and must be fixed.

[gabemontero]

OK yes that is what I have been referring to. If @jcantrill can drive a default/generic implementation of hostname verification in openshift-restclient (and its use of okhttp3 or jboss api), that would be better of course for consumers like jenkins-plugin.

If that is not going to be available near term I can certainly do a compare of the hostname and sslSession (and SSLSesion.getPeerHost()) or whatever else is the proscribed way to go.

[smarterclayton]

This is an override - so the correct behavior is to fall back to the base class. Generally you'd only override this method if you had a config value that says "I don't really need security but I want to pretend like I do" - if that value was false, then you'd call the super.

[smarterclayton]

All HTTP frameworks should be delegating to Java by default for this code - it's really not appropriate for anyone to override this behavior unless they are doing it in a very limited testing scenario.

[gabemontero]

In this case, ISSLCertificateCallback is an interface which Auth in Auth.java is implementing. So currently within the context of this there is not a default implementation.

Taking it further, ISSLCertificateCallback extends the interface javax.net.ssl.HostnameVerifier.

[gabemontero]

my calling super.allowHostname(hostname, sslSession) does not compile

[gabemontero]

Submitted the above pull in the jenkins image to disable sshd in the openshift jenkins images.

[smarterclayton]

What does the doc say about it not being present? I would expect every SSL library to have a default implementation - this is the exception rather than the rule.

What is it @ Override'ing

On Jan 13, 2017, at 4:45 PM, Gabe Montero notifications@github.com wrote:

Submitted the above pull in the jenkins image to disable sshd in the openshift jenkins images.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openshift/origin/issues/12476#issuecomment-272556289, or mute the thread https://github.com/notifications/unsubscribe-auth/ABG_pxJyd0-_VZVbU9oNtpEX1KLzTuM-ks5rR_B0gaJpZM4Lih7o .

[gabemontero]

I think eclipse just added the override annotation by default based on the widget i used to add the methods of the interface needing an implementation.

I'll remove it to eliminate the misleading implication and confirm everything works as before.

I'll look up the javadoc for the javax.net interface i noted earlier when I'm back in front of my laptop later this weekend and report back with what is says re any default implementations etc.

On Fri, Jan 13, 2017 at 7:50 PM Clayton Coleman notifications@github.com wrote:

What does the doc say about it not being present? I would expect every SSL

library to have a default implementation - this is the exception rather

than the rule.

What is it @ Override'ing

On Jan 13, 2017, at 4:45 PM, Gabe Montero notifications@github.com wrote:

Submitted the above pull in the jenkins image to disable sshd in the

openshift jenkins images.

—

You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub

https://github.com/openshift/origin/issues/12476#issuecomment-272556289,

or mute the thread

< https://github.com/notifications/unsubscribe-auth/ABG_pxJyd0-_VZVbU9oNtpEX1KLzTuM-ks5rR_B0gaJpZM4Lih7o

.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/openshift/origin/issues/12476#issuecomment-272586502, or mute the thread https://github.com/notifications/unsubscribe-auth/ADbadIOQr6T8KKRZ4lhqrjJEt3YH7ItOks5rSBu_gaJpZM4Lih7o .

[gabemontero]

Per the javadoc at https://docs.oracle.com/javase/7/docs/api/javax/net/ssl/HostnameVerifier.html, there are no default implementations in the JDK. I also see similar discussions in http://stackoverflow.com/questions/6031258/java-ssl-how-to-disable-hostname-verification .. ("There is no hostname verification in standard Java SSL sockets or indeed SSL, so that's why you can't set it at that level. Hostname verification is part of HTTPS (RFC 2818): that's why it manifests itself as javax.net.ssl.HostnameVerifier, which is applied to an HttpsURLConnection.")

There seems to be an inconsistency between what the HostnameVerifier javadoc says (dating back to java 1.4), spefically:

During handshaking, if the URL's hostname and the server's identification hostname mismatch, the verification mechanism can call back to implementers of this interface to determine if this connection should be allowed.

And what I see in the lower level http libraries that openshift-restclient and fabric8 use (okhttp3). If you look at https://github.com/square/okhttp/blob/master/okhttp/src/main/java/okhttp3/internal/connection/RealConnection.java#L266-L277, it calls verify irregardless of the outcome of the handshake.

Without a revamp of dependencies beyond our immediate ownership, it would seem top end users of this stack (like the jenkins-plugin) would appear to have the onus of say comparing the peer host name in the SSLSession with the hostname they initiated the request against to provide the verification being discussed here.

[smarterclayton]

Ok. Can we file bugs against the upstream components and then build the right checks in at our end,

On Jan 15, 2017, at 12:09 PM, Gabe Montero notifications@github.com wrote:

Per the javadoc at https://docs.oracle.com/javase/7/docs/api/javax/net/ssl/HostnameVerifier.html, there are no default implementations in the JDK. I also see similar discussions in http://stackoverflow.com/questions/6031258/java-ssl-how-to-disable-hostname-verification .. ("There is no hostname verification in standard Java SSL sockets or indeed SSL, so that's why you can't set it at that level. Hostname verification is part of HTTPS (RFC 2818): that's why it manifests itself as javax.net.ssl.HostnameVerifier, which is applied to an HttpsURLConnection.")

There seems to be an inconsistency between what the HostnameVerifier javadoc says (dating back to java 1.4), spefically:

During handshaking, if the URL's hostname and the server's identification hostname mismatch, the verification mechanism can call back to implementers of this interface to determine if this connection should be allowed.

And what I see in the lower level http libraries that openshift-restclient and fabric8 use (okhttp3). If you look at https://github.com/square/okhttp/blob/master/okhttp/src/main/java/okhttp3/internal/connection/RealConnection.java#L266-L277, it calls verify irregardless of the outcome of the handshake.

Without a revamp of dependencies beyond our immediate ownership, it would seem top end users of this stack (like the jenkins-plugin) would appear to have the onus of say comparing the peer host name in the SSLSession with the hostname they initiated the request against to provide the verification being discussed here.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openshift/origin/issues/12476#issuecomment-272708625, or mute the thread https://github.com/notifications/unsubscribe-auth/ABG_p5aX9QPsRiiaq9z4oUM3tT2LuXnwks5rSlKzgaJpZM4Lih7o .

[gabemontero]

Discovered today that while the jdk does not have a default hostname verification, the okhttp3 component that openshift-restclient-java uses does. And it looks good on what it does based on discussions with @liggitt.

I've submitted a pull for openshift-restclient-java so that the jenkins-plugin can leverage the okhttp3 default hostname verifier.

[gabemontero]

OK, with this https://github.com/openshift/jenkins/pull/235 merging, the openshift jenkins image has been updated to address the allow hostname discussion above. Also confirmed any other consumers of okhttp3 (i.e. fabric08 k8s client) defer to the default hostname verifier correctly.

With that, and the original two items (disabling of sshd and turning off skip tls for the k8s plugin) being previously addressed in the openshift jenkins image, we're done with this one.

[smarterclayton]

Thanks for chasing this down Gabe.

On Mon, Jan 23, 2017 at 10:44 PM, Gabe Montero notifications@github.com wrote:

Closed #12476 https://github.com/openshift/origin/issues/12476.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openshift/origin/issues/12476#event-933906292, or mute the thread https://github.com/notifications/unsubscribe-auth/ABG_p4-s0baUsUhNg2BqzQARQsTJgzIPks5rVXOogaJpZM4Lih7o .

[gabemontero]

My pleasure Clayton

On Mon, Jan 23, 2017 at 11:35 PM Clayton Coleman notifications@github.com wrote:

Thanks for chasing this down Gabe.

On Mon, Jan 23, 2017 at 10:44 PM, Gabe Montero notifications@github.com

wrote:

Closed #12476 https://github.com/openshift/origin/issues/12476.

—

You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub

https://github.com/openshift/origin/issues/12476#event-933906292, or mute

the thread

< https://github.com/notifications/unsubscribe-auth/ABG_p4-s0baUsUhNg2BqzQARQsTJgzIPks5rVXOogaJpZM4Lih7o

.

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/openshift/origin/issues/12476#issuecomment-274707543, or mute the thread https://github.com/notifications/unsubscribe-auth/ADbadOF1vAnTzz7mod2I5O6l73r2eGlfks5rVX96gaJpZM4Lih7o .