openshift / jenkins-plugin

Apache License 2.0
81 stars 50 forks source link

provide some form of ssl hostname validation #123

Closed gabemontero closed 7 years ago

gabemontero commented 7 years ago

@bparees @csrwng PTAL

This stems from the hornets' nest I stirred up in https://github.com/openshift/origin/issues/12476 with comment https://github.com/openshift/origin/issues/12476#issuecomment-272461025

Clayton ultimately chimed in with comment https://github.com/openshift/origin/issues/12476#issuecomment-272547257, asking we do something asap.

Based on my parsing of some SSL related javadoc, and analysis of results from the base oc cluster up start up, I've crafted this change to provide some hostname based validation of the remote/server side of the SSL connection.

Aside from using the hostname that the plugin uses for the api server, trying to interpret some proxy based comments I thought Ben made somewhere, I also added the ability to manually configure a list of hosts that the remote certs needed to ref.

Thoughts?

bparees commented 7 years ago

we should probably get @liggitt or @deads2k to take a look. Superficially it looks ok to me, but I really don't know this space. In particular the proxy host thing may not be the right way to handle the potential interference of proxies (it's probably assumed any proxy would be a pass-through proxy, so no special handling is needed) but it does provide a nice "get out of jail free" way to bypass cert verification for specific hostnames.

gabemontero commented 7 years ago

Going to do an about face on this one. In wrapping up the changes to @liggitt 's comments, I discovered that okhttp3 does in fact have a default hostname verifier !!!

And it appears to go to great lengths around things like the matching rules in https://tools.ietf.org/html/rfc6125#section-6.4. Certainly it was more sophisticated than what I was building here. Assuming it works for us, it seems more prudent to me to rework this logic. And is along the lines of what Clayton was originally hoping for.

I'll have to rework the openshift-restclient slightly to allow for this as an option, but it is the better way to go IMO.

I'll report back once I have some results from the prototype.

gabemontero commented 7 years ago

The prototype checks out with using okhttp3's default hostname verifier... waiting on https://github.com/openshift/openshift-restclient-java/pull/252 to merge, followed by maven publish / release cut, and then I'll update this pull to defer to okhttp3 for ssl hostname verification.

gabemontero commented 7 years ago

Still waiting on the maven publish / release cut for the restclient (the pr there has merged), but I went ahead and pushed the resulting changes in the plugin if folks want to look.

I'll update the pom.xml once the release cut has happened.

gabemontero commented 7 years ago

The cutting of 5.4.0.Final of openshift-restclient-java on the jboss nexus repo apparently hit a openshift push image to docker registry network timeout :-) (https://issues.jboss.org/browse/JBIDE-23771 and https://github.com/openshift/origin/issues/12603)

As such, I can't cite 5.4.0.Final as a maven dependency. We'll hold out a bit and see if they can get this resolved before falling back on using 5.4.0-SNAPSHOT (or possibly getting them to try a 5.5.0.Final). We've used their SNAPSHOT's before, so have some amount of confidence in it.

I've pushed updates that currently uses 5.4.0-SNAPSHOT. I also tweaked the implementation based on what I saw with https://github.com/fabric8io/kubernetes-client wrt bypassing the okhttp3 hostname verifier when in skip tls mode.

gabemontero commented 7 years ago

OK, the extended tests for this PR passed. The job failed (as did the last PR I did) not because of some env hiccup, but because the plugin tests were reworked and the job def isn't quite right anymore ... see The test image was changed, check for openshift/jenkins-plugin-snapshot-test on Dockerhub. There is in fact no such image on docker hub:

gmontero ~ $ docker pull docker.io/openshift/jenkins-plugin-snapshot-test
Using default tag: latest
Trying to pull repository docker.io/openshift/jenkins-plugin-snapshot-test ... 
Pulling repository docker.io/openshift/jenkins-plugin-snapshot-test
Error: image openshift/jenkins-plugin-snapshot-test not found
gmontero ~ $ 

Going to merge this and address the ci.openshift job def separately.