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

Set initial SNI hostname when creating an OpenSSL engine in client mode #8178

Merged
merged 1 commit into from Aug 9, 2018

Conversation

Projects
None yet
2 participants
@jianglai
Contributor

jianglai commented Aug 7, 2018

Motivation:

When using the JDK SSL provider in client mode, the SNI host names (called serverNames in SslEngineImpl) is set to the peerHost (if available) that is used to initialize the SSL Engine:

http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/sun/security/ssl/SSLEngineImpl.java#l377

This allows one to call SslEngine.getSSLParameters() and inspect what is the SNI name to be sent. The same should be done in the OpenSSL provider as well. Currently even though the the SNI name is sent by the OpenSSL provider during handshake when the peerHost is specified, it is missing from the parameters.

Modification:

Set the sniHostNames field when SNI is to be used. Also verifies the peer is actually a hostname before setting it as the SNI name, which is consistent with JDK SSL provider's behavior.

Result:

SslEngine using the OpenSSL provider created in client mode with peerHost should initialize sniHostNames with the peerHost.

Calling SslEngine.getSSLParameters().getServerNames() should return a list that contains that name.

PS:

I'm not sure where to put tests. Please advise.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Aug 8, 2018

Member

@jianglai I think we should put tests in SSLEngineTest. Can you check ? If you can not come up with a test I can also do it as a followup.

Member

normanmaurer commented Aug 8, 2018

@jianglai I think we should put tests in SSLEngineTest. Can you check ? If you can not come up with a test I can also do it as a followup.

@normanmaurer

One thing... after that ready to be merged

@jianglai

This comment has been minimized.

Show comment
Hide comment
@jianglai

jianglai Aug 8, 2018

Contributor

PTAL. All tests passed.

Contributor

jianglai commented Aug 8, 2018

PTAL. All tests passed.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Aug 9, 2018

Member

@jianglai please address the last one comment and squash. Will merge after this was done.

Member

normanmaurer commented Aug 9, 2018

@jianglai please address the last one comment and squash. Will merge after this was done.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Aug 9, 2018

Member

@jianglai also please sign the ICLA:

https://netty.io/s/icla

Member

normanmaurer commented Aug 9, 2018

@jianglai also please sign the ICLA:

https://netty.io/s/icla

@jianglai

This comment has been minimized.

Show comment
Hide comment
@jianglai

jianglai Aug 9, 2018

Contributor

Removed reference to internal methods and squashed. PTAL.

Regarding the CLA, I am contributing as an employee of Google. Do I still need to sign an individual CLA? Please advise. Thank you!

Contributor

jianglai commented Aug 9, 2018

Removed reference to internal methods and squashed. PTAL.

Regarding the CLA, I am contributing as an employee of Google. Do I still need to sign an individual CLA? Please advise. Thank you!

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Aug 9, 2018

Member

@jianglai yes you need to do... @nmittler @ejona86 @carl-mastrangelo etc all have signed the ICLA.

Member

normanmaurer commented Aug 9, 2018

@jianglai yes you need to do... @nmittler @ejona86 @carl-mastrangelo etc all have signed the ICLA.

@jianglai

This comment has been minimized.

Show comment
Hide comment
@jianglai

jianglai Aug 9, 2018

Contributor

Understood. ICLA signed.

Contributor

jianglai commented Aug 9, 2018

Understood. ICLA signed.

@normanmaurer normanmaurer self-assigned this Aug 9, 2018

@normanmaurer normanmaurer added the defect label Aug 9, 2018

@normanmaurer normanmaurer added this to the 4.1.29.Final milestone Aug 9, 2018

@normanmaurer normanmaurer merged commit dbc9ec1 into netty:4.1 Aug 9, 2018

1 check passed

continuous-integration/teamcity Finished TeamCity Build pull requests :: netty : Tests passed: 13853, ignored: 130
Details
@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer
Member

normanmaurer commented Aug 9, 2018

@jianglai thanks!

@jianglai jianglai deleted the jianglai:ssl-sni branch Aug 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment