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.

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

(#19884) Intermittent SSL handshake error #512

Merged
merged 3 commits into from May 14, 2013

Conversation

grimradical
Copy link
Member

Certain versions of the JDK (1.7u6 and newer) appear to have a bug when
handling Diffie-Hellman exchange during SSL handshaking with OpenSSL
clients. Bug is discussed here:

https://forums.oracle.com/forums/thread.jspa?messageID=10999587

In me tests just having openssl's s_client connect to PuppetDB
repeatedly in a tight loop, we see the handshake error about 0.4% of the
time. So it's not a pervasive problem, but it's certainly annoying and
something users have seen and complained about somewhat regularly.

There are a few options available for fixing the problem:

  1. Wait for Oracle to fix the problem. This is, of course, suboptimal.

  2. Use BouncyCastle JSSE providers instead. This is workable, but in my
    research would require changes to the JDK system policy files, which is
    a huge PITA. There's also some concern about performance, if the BC
    providers are markedly slower than the JSSE ones.

  3. Disable the DHE ciphers.

This patchset implements option #3. The non-DHE cipher suites work fine,
support SSL and TLS, and are on the NIST and FIPS approved list. So...it
sucks that we need to disable the DHE cipher suites, but in an insane
world it's the sanest choice.

I've added a new option to the [jetty] section of the configuration file
called cipher-suites. It allows users, should they have the need, to
override the list of cipher suites used to one of their choosing.

Certain versions of the JDK (1.7u6 and newer) appear to have a bug when
handling Diffie-Hellman exchange during SSL handshaking with OpenSSL
clients. Bug is discussed here:

https://forums.oracle.com/forums/thread.jspa?messageID=10999587

In me tests just having openssl's s_client connect to PuppetDB
repeatedly in a tight loop, we see the handshake error about 0.4% of the
time. So it's not a pervasive problem, but it's certainly annoying and
something users have seen and complained about somewhat regularly.

There are a few options available for fixing the problem:

1) Wait for Oracle to fix the problem. This is, of course, suboptimal.

2) Use BouncyCastle JSSE providers instead. This is workable, but in my
research would require changes to the JDK system policy files, which is
a huge PITA. There's also some concern about performance, if the BC
providers are markedly slower than the JSSE ones.

3) Disable the DHE ciphers.

This patchset implements option #3. The non-DHE cipher suites work fine,
support SSL and TLS, and are on the NIST and FIPS approved list. So...it
sucks that we need to disable the DHE cipher suites, but in an insane
world it's the sanest choice.

I've added a new option to the [jetty] section of the configuration file
called `cipher-suites`. It allows users, should they have the need, to
override the list of cipher suites used to one of their choosing.
@kbarber
Copy link
Contributor

kbarber commented May 13, 2013

+1 happy to review this tomorrow morning @grimradical ... awesome work :-).

@puppetcla
Copy link

CLA Signed by grimradical on 2010-09-07 21:00:00 -0700

(map trim (split txt #","))
acceptable-ciphers)]
(doto (.getSslContextFactory connector)
(.setIncludeCipherSuites (into-array ciphers)))

Choose a reason for hiding this comment

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

Do you have any idea what happens here if there is a bogus ciper name in the list?

Copy link
Member Author

Choose a reason for hiding this comment

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

Jetty ignores ciphers it doesn't recognize. Under the hood, it takes the list of ciphers available in the JVM and does an intersection with the list you supply as the whitelist. The bogus cipher name won't exist in the JVM list, and will hence fall out of the intersection.

@cprice404
Copy link

+1 , we should merge this as soon as we get CI fixed up.

@slippycheeze
Copy link
Contributor

It sucks to have to do this, but it is worse to do it unconditionally as this change appears to do. Would it be within reach to use (System/getProperty "java.version") to limit this to the specific revisions that have problems? (At least, to use this only on X or greater today, and once the fix is confirmed, between X and Y?)

@grimradical
Copy link
Member Author

@daniel-pittman that's an entirely fair point!

@grimradical
Copy link
Member Author

Updated to address @daniel-pittman 's comment.

@slippycheeze
Copy link
Contributor

I don't see compare defined, but I am not sure if this will do the right thing for older versions? That is, does this actually implement correctly "if the version is X or greater"? If so, I have no concerns about this. :)

@cprice404
Copy link

Yeah, I have the same take as Daniel. If that's just a regular string compare, how confident are you that it'll do what we want? e.g., what about for java 1.10? (Admittedly that won't be released for 20 years, but...)

Also it looks like this does allow the user to override on other versions of java? I think that's the right way to do it, but just thought I'd bring it up for discussion.

;; https://issues.apache.org/jira/browse/APLO-287
;;
;; If not running on an affected JVM version, this is nil.
(def acceptable-ciphers
Copy link
Contributor

Choose a reason for hiding this comment

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

Now this could use a test. :) Probably better to make this a function, for testability (so you can stub System/getProperty or something?).

@grimradical
Copy link
Member Author

Okay, I've changed it to a function as @NLEW suggested, and added some tests. I've added a separate function to compare the versions by explicitly splitting the strings and converting the components to integers. From the best info I've found about the format of the strings, it looks like we can expect only numbers, dots, and underscores in the first "field".


Optional. A comma-separated list of cryptographic ciphers to allow for incoming SSL connections. Valid names are listed in the [official JDK cryptographic providers documentation](http://docs.oracle.com/javase/7/docs/technotes/guides/security/SunProviders.html#SupportedCipherSuites); you'll need to use the all-caps cipher suite name.

If not supplied, PuppetDB uses the complete, non-DHE set of ciphers.

Choose a reason for hiding this comment

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

Is it worth mentioning that we do this only for the known affected versions of the JDK?

cprice404 added a commit that referenced this pull request May 14, 2013
(#19884) Intermittent SSL handshake error
@cprice404 cprice404 merged commit 454b9f2 into puppetlabs:master May 14, 2013
@rfay
Copy link

rfay commented Jun 5, 2013

I'm still baffled how the java version fits into something I thought was pure ruby? Running puppet 2.6 and seeing this on both CentOS and Debian. On the debian setup I don't even have java installed (and time is ntp synced ok).

@cprice404
Copy link

@rfay the PuppetDB server is written in Clojure, which is a JVM language. The SSL communication is between a ruby client (the puppet master) and the JVM server (PuppetDB).

@rfay
Copy link

rfay commented Jun 5, 2013

Hmm. That doesn't explain why I have so many of these SSL_Connect failures in my environment, which is just puppet master and puppet agent. I guess I landed in the wrong place, huh? But I see those errors everywhere and often, and there are many baffled people on the web about them... one of those let here. Thanks for the quick response, @cprice-puppet - Much appreciate any pointers you may have to solving this in a simple 2.6 puppet environment.

@kbarber
Copy link
Contributor

kbarber commented Jun 6, 2013

@rfay if you aren't using PuppetDB, and this problem is occurring for Puppet by itself, then this is probably the wrong place to discuss the issue as this is the issue tracker for PuppetDB problems specifically.

You might want to try emailing the details of your problem to the puppet-users mailing list (http://groups.google.com/group/puppet-users) firstly, or perhaps seeking help from someone on the IRC channel: #puppet (on freenode) and if its a confirmed bug you can raise such a thing for Puppet specifically here: http://projects.puppetlabs.com/projects/puppet. I would start with puppet-users or IRC first however, as it might be a known issue. When emailing the mailing list, as there are many different kinds of SSL failures, include the entire dump of the errors from puppet agent -t --trace (or however you run puppet manually) in the email, as it will help people understand the precise SSL issue.

Thanks :-).

@rfay
Copy link

rfay commented Jun 6, 2013

Certainly agreed and understood. Much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants