[Client] Switch to rely on SslEngine for Hostname Verification #3310
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
michaeljmarshall
changed the title
|
rerun failure checks |
shoothzj
approved these changes
Jun 5, 2022
nicoloboschi
pushed a commit
that referenced
this pull request
Jun 6, 2022
### Motivation Currently, we initiate hostname verification for the Bookkeeper Client in the `PerChannelBookieClient` class. In order to simplify the code, I propose that we refactor the client so it relies on Netty, its SslHandler/SslEngine, and the JVM, to perform the hostname verification. When HTTPS is configured as the endpoint verification algorithm, it uses [RFC 2818](https://datatracker.ietf.org/doc/html/rfc2818) to perform hostname verification. This is defined by the Java Security Standard Algorithm Names documentation for JDK versions 8, 11, and 17. Here are the official docs: * https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html * https://docs.oracle.com/en/java/javase/11/docs/specs/security/standard-names.html * https://docs.oracle.com/en/java/javase/17/docs/specs/security/standard-names.html ### Changes * Rely on Netty and the SslEngine to perform hostname verification. With this change, CN matching is now deprecated, which brings the bookkeeper client in alignment with RFC 2818. * Add new method to the `SecurityHandlerFactory` interface. It is named `newTLSHandler` and takes the `host` and `port` of the remote peer when creating a new SslEngine. To ensure backwards compatibility, the default implementation will call the original method. Note that the remote host and port are only needed when a client is using them for hostname verification. (cherry picked from commit 6b22f85)
nicoloboschi
pushed a commit
that referenced
this pull request
Jun 6, 2022
### Motivation Currently, we initiate hostname verification for the Bookkeeper Client in the `PerChannelBookieClient` class. In order to simplify the code, I propose that we refactor the client so it relies on Netty, its SslHandler/SslEngine, and the JVM, to perform the hostname verification. When HTTPS is configured as the endpoint verification algorithm, it uses [RFC 2818](https://datatracker.ietf.org/doc/html/rfc2818) to perform hostname verification. This is defined by the Java Security Standard Algorithm Names documentation for JDK versions 8, 11, and 17. Here are the official docs: * https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html * https://docs.oracle.com/en/java/javase/11/docs/specs/security/standard-names.html * https://docs.oracle.com/en/java/javase/17/docs/specs/security/standard-names.html ### Changes * Rely on Netty and the SslEngine to perform hostname verification. With this change, CN matching is now deprecated, which brings the bookkeeper client in alignment with RFC 2818. * Add new method to the `SecurityHandlerFactory` interface. It is named `newTLSHandler` and takes the `host` and `port` of the remote peer when creating a new SslEngine. To ensure backwards compatibility, the default implementation will call the original method. Note that the remote host and port are only needed when a client is using them for hostname verification. (cherry picked from commit 6b22f85)
nicoloboschi
pushed a commit
to datastax/bookkeeper
that referenced
this pull request
Jun 6, 2022
### Motivation Currently, we initiate hostname verification for the Bookkeeper Client in the `PerChannelBookieClient` class. In order to simplify the code, I propose that we refactor the client so it relies on Netty, its SslHandler/SslEngine, and the JVM, to perform the hostname verification. When HTTPS is configured as the endpoint verification algorithm, it uses [RFC 2818](https://datatracker.ietf.org/doc/html/rfc2818) to perform hostname verification. This is defined by the Java Security Standard Algorithm Names documentation for JDK versions 8, 11, and 17. Here are the official docs: * https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html * https://docs.oracle.com/en/java/javase/11/docs/specs/security/standard-names.html * https://docs.oracle.com/en/java/javase/17/docs/specs/security/standard-names.html ### Changes * Rely on Netty and the SslEngine to perform hostname verification. With this change, CN matching is now deprecated, which brings the bookkeeper client in alignment with RFC 2818. * Add new method to the `SecurityHandlerFactory` interface. It is named `newTLSHandler` and takes the `host` and `port` of the remote peer when creating a new SslEngine. To ensure backwards compatibility, the default implementation will call the original method. Note that the remote host and port are only needed when a client is using them for hostname verification. (cherry picked from commit 6b22f85) (cherry picked from commit 315195e)
shoothzj
pushed a commit
that referenced
this pull request
Jun 9, 2022
### Motivation While testing #3310, I noticed that the `PerChannelBookieClient#exceptionCaught` logic contains redundant logs when the exception is an `SSLException`. This PR removes a redundant log from client. Based on reading through the rest of the method, this should be a trivial change with no other side effects. My one question is how closing a channel and closing the context differ. Technically, returning early skips the `ctx.close()`, which could change the behavior. ### Changes * Return early to prevent a redundant log
zymap
pushed a commit
that referenced
this pull request
Aug 1, 2022
### Motivation While testing #3310, I noticed that the `PerChannelBookieClient#exceptionCaught` logic contains redundant logs when the exception is an `SSLException`. This PR removes a redundant log from client. Based on reading through the rest of the method, this should be a trivial change with no other side effects. My one question is how closing a channel and closing the context differ. Technically, returning early skips the `ctx.close()`, which could change the behavior. ### Changes * Return early to prevent a redundant log (cherry picked from commit bd82797)
hangc0276
pushed a commit
to hangc0276/bookkeeper
that referenced
this pull request
Nov 5, 2022
### Motivation While testing apache#3310, I noticed that the `PerChannelBookieClient#exceptionCaught` logic contains redundant logs when the exception is an `SSLException`. This PR removes a redundant log from client. Based on reading through the rest of the method, this should be a trivial change with no other side effects. My one question is how closing a channel and closing the context differ. Technically, returning early skips the `ctx.close()`, which could change the behavior. ### Changes * Return early to prevent a redundant log (cherry picked from commit bd82797)
hangc0276
pushed a commit
to hangc0276/bookkeeper
that referenced
this pull request
Nov 7, 2022
### Motivation While testing apache#3310, I noticed that the `PerChannelBookieClient#exceptionCaught` logic contains redundant logs when the exception is an `SSLException`. This PR removes a redundant log from client. Based on reading through the rest of the method, this should be a trivial change with no other side effects. My one question is how closing a channel and closing the context differ. Technically, returning early skips the `ctx.close()`, which could change the behavior. ### Changes * Return early to prevent a redundant log (cherry picked from commit bd82797)
nicoloboschi
pushed a commit
to datastax/bookkeeper
that referenced
this pull request
Jan 11, 2023
### Motivation While testing apache#3310, I noticed that the `PerChannelBookieClient#exceptionCaught` logic contains redundant logs when the exception is an `SSLException`. This PR removes a redundant log from client. Based on reading through the rest of the method, this should be a trivial change with no other side effects. My one question is how closing a channel and closing the context differ. Technically, returning early skips the `ctx.close()`, which could change the behavior. ### Changes * Return early to prevent a redundant log (cherry picked from commit bd82797) (cherry picked from commit e3091df)
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.
Motivation
Currently, we initiate hostname verification for the Bookkeeper Client in the
PerChannelBookieClientclass. In order to simplify the code, I propose that we refactor the client so it relies on Netty, its SslHandler/SslEngine, and the JVM, to perform the hostname verification.When HTTPS is configured as the endpoint verification algorithm, it uses RFC 2818 to perform hostname verification. This is defined by the Java Security Standard Algorithm Names documentation for JDK versions 8, 11, and 17. Here are the official docs:
Changes
SecurityHandlerFactoryinterface. It is namednewTLSHandlerand takes thehostandportof the remote peer when creating a new SslEngine. To ensure backwards compatibility, the default implementation will call the original method. Note that the remote host and port are only needed when a client is using them for hostname verification.