SSLEngineWebSocketServerFactory allows more customization #839
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PhilipRoman
reviewed
Jan 8, 2019
| this(sslEngine, Executors.newSingleThreadScheduledExecutor()); | ||
| } | ||
|
|
||
| private SSLEngineWebSocketServerFactory(SSLEngine sslEngine, ExecutorService exec) { |
There was a problem hiding this comment.
Is there any specific reason why this constructor is private and not public?
PhilipRoman
reviewed
Jan 8, 2019
|
|
||
| @Override | ||
| public void close() { | ||
|
|
There was a problem hiding this comment.
Looks like DefaultSSLWebSocketServerFactory closes the executor service in it's close method. Maybe it should be done here as well.
There was a problem hiding this comment.
maybe even extend from DefaultSSLWebSocketServerFactory?
marci4
requested changes
Jan 9, 2019
|
Comments addressed, simple unit test included. |
marci4
requested changes
Jan 17, 2019
| * | ||
| * @param sslContext - can not be <code>null</code> | ||
| * @param executerService - can not be <code>null</code> | ||
| * @param sslParameters - sslParameters |
There was a problem hiding this comment.
sslParameters cannot be null, could you please adjust the JavaDoc
| public ByteChannel wrapChannel(SocketChannel channel, SelectionKey key) throws IOException { | ||
| SSLEngine e = sslcontext.createSSLEngine(); | ||
| e.setUseClientMode(false); | ||
| if (sslParameters != null) { |
There was a problem hiding this comment.
No check needed since we throw an IllegalArgumentException()
|
@BroHammie sorry for again requesting changes :( |
marci4
approved these changes
Jan 21, 2019
|
@BroHammie thank you again for your contribution! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Introduces a new class SSLEngineWebSocketServerFactory that takes a SSLEngine as parameter allowing for more customization.
Related Issue
#838
Motivation and Context
Now I can create a WebSocketServerFactory with a SSLEngine that has NeedClientAuth set to true, thus making Server request a client certificate on connect.
How Has This Been Tested?
Created a TwoWaySSLServerExample(included in PR) that sets the value. Running wireshark with and without that flag set I can see a difference and the server sending a "client certificate request"
Types of changes
Checklist: