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

Allow to validate sni hostname with underscore #8150

Merged
merged 1 commit into from Jul 26, 2018

Conversation

Projects
None yet
3 participants
@normanmaurer
Copy link
Member

commented Jul 25, 2018

Motivation:

We should allow to also validate sni hostname which contains for example underscore when using our native SSL impl. The JDK implementation does this as well.

Modifications:

  • Construct the SNIHostName via byte[] and not String.
  • Add unit test

Result:

Fixes #8144.

Allow to validate sni hostname with underscore
Motivation:

We should allow to also validate sni hostname which contains for example underscore when using our native SSL impl. The JDK implementation does this as well.

Modifications:

- Construct the SNIHostName via byte[] and not String.
- Add unit test

Result:

Fixes #8144.
@ejona86
Copy link
Member

left a comment

Why does byte[] vs String have anything to do with whether underscores are handled?

@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2018

@ejona86 because of how SNIHostName constructor is implemented.

@ejona86
Copy link
Member

left a comment

Okay, so it appears the byte[] version is only being used for inbound data. That seems fair and agrees with the documentation. It seems the inbound path relies heavily on SNIMatcher, which... the application is providing? Seems likely.

Outbound data should probably still be using the String version or we need to handle IDN manually. That path seems like it could be broken by underscores. Maybe avoiding the USE_STD3_ASCII_RULES flag could "fix" that, but I'd be pretty cautious if we went with that approach.

@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2018

@ejona86 yep exactly...

@normanmaurer normanmaurer requested a review from carl-mastrangelo Jul 26, 2018

@normanmaurer normanmaurer merged commit 620dad0 into 4.1 Jul 26, 2018

1 check passed

continuous-integration/teamcity Finished TeamCity Build pull requests :: netty : Tests passed: 13842, ignored: 125
Details

@normanmaurer normanmaurer deleted the sni_underscore branch Jul 26, 2018

@@ -244,7 +245,8 @@ void verify(ReferenceCountedOpenSslEngine engine, X509Certificate[] peerCerts, S
public boolean match(long ssl, String hostname) {
ReferenceCountedOpenSslEngine engine = engineMap.get(ssl);
if (engine != null) {
return engine.checkSniHostnameMatch(hostname);
// TODO: In the next release of tcnative we should pass the byte[] directly in and not use a String.

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Jul 27, 2018

Member

Is there an issue on tcnarive for this?

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jul 27, 2018

Author Member

Not yet... will open one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.