SSL Transport configured in wantClientAuth mode never asks for the client certificate during the SSL Handshake

• Key: AMQ-1659
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: David Jencks
• Reporter: Eric White
• Component/s: Transport
• Affects Version/s: 4.1.1, 5.0.0
• Fix Version/s: 4.1.2, 5.1.0

Environment

I think this is for all environments, it may be JDK dependent though.

I tested on:
Linux 2.6.20-gentoo-r7
java version "1.6.0"
Java(TM) SE Runtime Environment (build 1.6.0-b105)
Java HotSpot(TM) 64-Bit Server VM (build 1.6.0-b105, mixed mode)

Description

See: http://java.sun.com/javase/6/docs/api/javax/net/ssl/SSLServerSocket.html#setWantClientAuth(boolean)

"
A socket's client authentication setting is one of the following:

• client authentication required
• client authentication requested
• no client authentication desired
  "

In the API it indicates that if you call either setWantClientAuth, or setNeedClientAuth it will override the call to the other.

Therefor I believe the following code only allows for ActiveMQ to be in two states:

• Client Authentication Required (needClientAuth==true)
• No client Authentication Desired (needClientAuth==false)

activemq-core/src/main/java/org/apache/activemq/transport/tcp/SslTransportServer.java

As setWantClientAuth is overridden by setNeedClientAuth.
public void bind() throws IOException {
super.bind();
((SSLServerSocket)this.serverSocket).setWantClientAuth(wantClientAuth);
((SSLServerSocket)this.serverSocket).setNeedClientAuth(needClientAuth);
}

I believe this the same issue as this Jetty issue: http://jira.codehaus.org/browse/JETTY-86

Comments

[Eric White]

 
activemq-core/src/main/java/org/apache/activemq/transport/tcp/SslTransportServer.java
public void bind() throws IOException { 
    super.bind();
    ((SSLServerSocket)this.serverSocket).setWantClientAuth(wantClientAuth);
    ((SSLServerSocket)this.serverSocket).setNeedClientAuth(needClientAuth); <--- This overrides setWantClientAuth
}

[Eric White]

https://issues.apache.org/activemq/secure/attachment/16326/amq-411-complex-version.patch

This file is for ActiveMQ 4.1.1

I have tested this file locally and it works for me.

In this version Boolean properties are used instead of just boolean. This makes it possible to distinguish between true, false, and null. Corresponding to the underlying properties being set to true, false or not set at all.

[Eric White]

https://issues.apache.org/activemq/secure/attachment/16327/amq-411-simple-version.patch

This file is for ActiveMQ 4.1.1

I have tested this file locally and it works for me.

In this version only boolean properties are used. This make it impossible to know if the property was set on the Transport URL or not.

My personal belief is the complex version is better, but I supplied both as I'm unsure how the maintainers would like to see the issue resolved.

[Eric White]

https://issues.apache.org/activemq/secure/attachment/16328/amq-500-complex-version.patch

This file is for ActiveMQ 5.0.0

I have NOT TESTED this on ActiveMQ 5, but the looking at the source code, I think the issue is there. This patch does compile.

In this version Boolean properties are used instead of just boolean. This makes it possible to distinguish between true, false, and null. Corresponding to the underlying properties being set to true, false or not set at all.

[Eric White]

https://issues.apache.org/activemq/secure/attachment/16329/amq-500-simple-version.patch

This file is for ActiveMQ 5.0.0

I have NOT TESTED this on ActiveMQ 5, but the looking at the source code, I think the issue is there. This patch does compile.

In this version only boolean properties are used. This make it impossible to know if the property was set on the Transport URL or not.

My personal belief is the complex version is better, but I supplied both as I'm unsure how the maintainers would like to see the issue resolved.

[David Jencks]

In the past I've fixed this bug (in other projects) like this:

public void bind() throws IOException {
super.bind();
if (needClientAuth) { ((SSLServerSocket)this.serverSocket).setNeedClientAuth(true); } else if (wantClientAuth) { ((SSLServerSocket)this.serverSocket).setWantClientAuth(true); }
}

which to me corresponds better to natural language usage like "I not only want client auth, I need it!"

Is there some reason you think that setting both flags true should be disallowed?

[David Jencks]

with luck 4.1.2 is out the door.... should be able to get it in 5.1.0 though

[David Jencks]

looks like it will make it into 4.1.2

[David Jencks]

The test needed quite a bit of fixing up also. The proposed patches would definitely have broken the tests as they test all 4 combinations, and do not expect an exception if both flags are true.
4.1 rev 646928
trunk rev 646936

[Eric White]

The reason I though throwing an exception was a good idea was, in
reading the JDK API, these two options are really mutually exclusive,
if you set one the other is unset. So, my thinking was URIs
containing: needClientAuth=true&wantClientAuth=true are technically
incorrect.

That being said, I prefer the approach that you took. Because from
the end users perspective it is very difficult to know that
needClientAuth and wantClientAuth override each other deep down inside
of the JDK. So as you say if the user configuring ActiveMQ and sets
needClientAuth then that should take precedence over wantClientAuth.
This is because needClientAuth is more restrictive of the two.

I'm sorry I didn't run the tests, that was a lapse of judgment on my part.

Would it be possible to update this page:
http://activemq.apache.org/contributing.html
To include something like this in the "Submitting patches" section:

Quick Check List:
1. Does the patch apply clean to the version it is supposed to fix.
2. Does the resutling patched code complie
3. Do the Unit tests run cleanly

All of these are obvious, but it never hurts, to remind everyone.

Thank you very much for fixing this in time for ActiveMQ 4.1.2. I
really appreciate the quick turn around.

Regards,
Eric