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

Fix Java9SslEngine implementation of ApplicationProtocolAccessor and … #7258

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

@normanmaurer
Member

normanmaurer commented Sep 28, 2017

…so fix ApplicationProtocolNegationHandler

Motivation:

Java9SslEngine did not correctly implement ApplicationProtocolAccessor and so returned an empty String when no extension was used while the interface contract is to return null. This lead to the situation that ApplicationProtocolNegationHandler did not correct work.

Modifications:

  • Rename ApplicationProtocolAccessor.getApplicationProtocol() to getNegotiatedApplicationProtocol() which resolves the clash with the method exposed by Java9s SSLEngine.
  • Correctly implement getNegotiatedApplicationProtocol() for Java9Sslengine
  • Add delegate in Java9Sslengine.getApplicationProtocol() which is provided by Java9
  • Adjust tests to test the correct behaviour.

Result:

Fixes [#7251].

Fix Java9SslEngine implementation of ApplicationProtocolAccessor and …
…so fix ApplicationProtocolNegationHandler

Motivation:

Java9SslEngine did not correctly implement ApplicationProtocolAccessor and so returned an empty String when no extension was used while the interface contract is to return null. This lead to the situation that ApplicationProtocolNegationHandler did not correct work.

Modifications:

- Rename ApplicationProtocolAccessor.getApplicationProtocol() to getNegotiatedApplicationProtocol() which resolves the clash with the method exposed by Java9s SSLEngine.
- Correctly implement getNegotiatedApplicationProtocol() for Java9Sslengine
- Add delegate in Java9Sslengine.getApplicationProtocol() which is provided by Java9
- Adjust tests to test the correct behaviour.

Result:

Fixes [#7251].

@normanmaurer normanmaurer requested review from ejona86 and Scottmitch Sep 28, 2017

@normanmaurer normanmaurer self-assigned this Sep 28, 2017

@normanmaurer normanmaurer requested a review from nmittler Sep 28, 2017

@normanmaurer normanmaurer added the defect label Sep 28, 2017

@normanmaurer normanmaurer added this to the 4.0.53.Final milestone Sep 28, 2017

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer
Member

normanmaurer commented Sep 28, 2017

@vietj PTAL

@normanmaurer normanmaurer requested a review from carl-mastrangelo Sep 29, 2017

@vietj

vietj approved these changes Sep 29, 2017

@carl-mastrangelo

LGTM, no concerns here.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Sep 29, 2017

Member

@vietj did you verify the fix as well with your testcase ?

Member

normanmaurer commented Sep 29, 2017

@vietj did you verify the fix as well with your testcase ?

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 2, 2017

Member

Cherry-picked into 4.1 (bb1833f) and 4.0 (5500a01)

Member

normanmaurer commented Oct 2, 2017

Cherry-picked into 4.1 (bb1833f) and 4.0 (5500a01)

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