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

Added HTTPS/SSL Support #68

Closed
wants to merge 1 commit into from
Closed

Conversation

steve-oakey
Copy link
Contributor

I had the requirement to use with application with SSL Client Authentication.
The SimpleHostRoutingFilter was not able to provide this, nor was
it extensible to allow a proper subclass. I provided an implementation
which uses most aspects of SimpleHostRoutingFilter, but builds the
HTTP client using current API as well as building a correct SSLContext
based on the javax.net.ssl JVM properties.

The client auto-configuration now provides a RestTemplate bean that builds
an SSLContext if SSL is enabled for the client application. If not, the
original RestTemplate is used.

A modification was needed to the AdminClientProperties to determine if
the connection requires https instead of http (checks ServerProperties
for an instance of Ssl).

I also added spring-configuration-processor as a dependency as to generate
property file metadata.

@coveralls
Copy link

Coverage Status

Coverage decreased (-6.29%) to 80.33% when pulling e4603bf on steve-oakey:master into 621ff8d on codecentric:master.

@steve-oakey
Copy link
Contributor Author

I will address the drop in test coverage tomorrow evening.

@joshiste
Copy link
Collaborator

joshiste commented May 1, 2015

The autodetection of https on client is a good idea.
But I think the changes to the resttemplate and securehostroutingfilter aren't necessary. Did you have a look at #66?

@steve-oakey
Copy link
Contributor Author

Sorry, I did not. I will re-investigate tonight. I will revert those
changes and see if the issues I was having still occur.

It would be great if the SecureHostRoutingFilter was not necessary, but I
thought the custom SSLSocketFactory in the class was not not loading the
trust information correctly. As well it uses the
'AllowAllHostnameVerifier' with the connection manager. If the client
requires x509 authentication via Spring Security (as well as the server) I
don't think it will work with just the JVM properties alone.

I will look at this tonight. Thank you for your comments and consideration.
On May 1, 2015 3:14 AM, "Johannes Stelzer" notifications@github.com wrote:

The autodetection of https on client is a goog idea.
But I think the changes to the resttemplate and securehostroutingfilter
aren't necessary. Did you have a look at #66
#66?


Reply to this email directly or view it on GitHub
#68 (comment)
.

@steve-oakey
Copy link
Contributor Author

You were correct about not needing a new RestTemplate. Adding the java.net.ssl JVM properties was enough for the client application to ping the server successfully with x509 client authentication.

However the server cannot complete the requests back to the client using the SimpleHostRoutingFilter. It does not build the HTTP client with an SSLContext that can make a secure connection back to the client. It uses a custom SSLSocket which (appears) not to load the key or trust store information. This may be an issue I can bring up with the spring-cloud-netflix project.

Shall I close this pull request? I can maintain a fork and use it until Spring Cloud can update that class, but as it stands now it will not work when the client requires x509 authentication.

@joshiste
Copy link
Collaborator

joshiste commented May 3, 2015

I'd be keen to do the changes to AdminClientProperties.
And the SecureHostRoutingFilter should be addressed in https://github.com/spring-cloud/spring-cloud-netflix

@steve-oakey
Copy link
Contributor Author

Sounds good. I will submit a new pull request as not to pollute the commit
history. This one can be closed.

-Steve
On May 3, 2015 1:34 PM, "Johannes Stelzer" notifications@github.com wrote:

I'd bee keen to do the changes to AdminClientProperties.
And the SecureHostRoutingFilter should be addressed in
https://github.com/spring-cloud/spring-cloud-netflix


Reply to this email directly or view it on GitHub
#68 (comment)
.

@joshiste
Copy link
Collaborator

joshiste commented May 3, 2015

Just git reset to codecentric/master do your changes and force push 'em.
The PR will be updated. No need for a new one
Am 03.05.2015 22:21 schrieb "Steve Oakey" notifications@github.com:

Sounds good. I will submit a new pull request as not to pollute the commit
history. This one can be closed.

-Steve
On May 3, 2015 1:34 PM, "Johannes Stelzer" notifications@github.com
wrote:

I'd bee keen to do the changes to AdminClientProperties.
And the SecureHostRoutingFilter should be addressed in
https://github.com/spring-cloud/spring-cloud-netflix


Reply to this email directly or view it on GitHub
<
#68 (comment)

.


Reply to this email directly or view it on GitHub
#68 (comment)
.

Checks for the presence of Ssl in ServerProperties and that it is
enabled, uses https in that case for client URL. Added test case for
option.

Added spring-boot-configuration-processor as optional dependency which
will generate ConfigurationProperty metadata.
@steve-oakey
Copy link
Contributor Author

Done, updated with test case. Thanks.

@joshiste
Copy link
Collaborator

joshiste commented May 4, 2015

Merged your commit. Thanks!
I divided the configuration-annotation-processor into a second commit and added descriptions to the properties.

@joshiste joshiste closed this May 4, 2015
@steve-oakey
Copy link
Contributor Author

Great, thanks!

  • Steve

On Mon, May 4, 2015 at 3:20 PM, Johannes Stelzer notifications@github.com
wrote:

Merged your commit. Thanks!
I divided the configuration-annotation-processor into a second commit and
added descriptions to the properties.


Reply to this email directly or view it on GitHub
#68 (comment)
.

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

3 participants