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.

Sign up for GitHub

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

Jump to bottom

Add SslConfig APIs and fix SNI and SSL hostname bugs #1387

Merged
merged 14 commits into from
Mar 11, 2021
Merged

Add SslConfig APIs and fix SNI and SSL hostname bugs #1387

merged 14 commits into from
Mar 11, 2021

Conversation

Scottmitch
Copy link
Member

Motivation:
The server SSL config APIs for SNI didn't allow configuring SSL
associated with each individual SNI host. The client APIs coupled the
host name verification algorithm with the non-authoritative peer
host/port. This doesn't correctly represent the semantics of SSL.

Modifications:

  • Deprecate all APIs related to SecurityConfigurator and secure()
    builder method. The secure() builder methods require that users call
    commit() which is easy to miss if the builder is used in a conditional
    block, and prototypes of a composable server SNI API results in code
    that is difficult to read and understand which configurations apply.
  • Add SslConfig interface types and builders that can be used to create
    the ssl configuration independent of the protocol builders. This
    allows for folks to configure ssl independently and we can overlay any
    values which maybe automatically inferred in the protocol builder
    (e.g. peerHost on the client, alpn protocols, etc.).
  • Server SSL allows for providing SNI configuration, and Netty's
    DomainWildcardMappingBuilder is used to match hostnames.
  • Client side SSL config object removes methods which couple the host
    name verification algorithm to the non-authoritative peer host/port,
    and adds new methods which allow setting them independently.
  • Update all tests to enable host name verification, and avoid
    deprecated method use when possible.

Result:
SSL config APIs now correctly represent SNI semantics for client and
server. The client SSL config decouples the non-authoritative peer
host/port from the verification algorithm.

The deprecated types and methods will be removed in a future release.

@Scottmitch Scottmitch mentioned this pull request Feb 25, 2021
Closed
@bondolo
Copy link
Contributor

bondolo commented Feb 25, 2021

I like the new API.

idelpivnitskiy
idelpivnitskiy requested changes Feb 27, 2021
View reviewed changes
Copy link
Member

@idelpivnitskiy idelpivnitskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great that the new approach solves our issues, like it!

...-transport-api/src/main/java/io/servicetalk/transport/api/DefaultClientSslConfigBuilder.java Outdated Show resolved Hide resolved
...-transport-api/src/main/java/io/servicetalk/transport/api/DefaultClientSslConfigBuilder.java Outdated Show resolved Hide resolved
...-transport-api/src/main/java/io/servicetalk/transport/api/DefaultClientSslConfigBuilder.java Outdated Show resolved Hide resolved
...-transport-api/src/main/java/io/servicetalk/transport/api/DefaultServerSslConfigBuilder.java Outdated Show resolved Hide resolved
...-transport-api/src/main/java/io/servicetalk/transport/api/DefaultClientSslConfigBuilder.java Outdated Show resolved Hide resolved
servicetalk-http-api/src/main/java/io/servicetalk/http/api/MultiAddressHttpClientBuilder.java Show resolved Hide resolved
servicetalk-http-api/src/main/java/io/servicetalk/http/api/MultiAddressHttpClientBuilder.java Outdated Show resolved Hide resolved
servicetalk-http-api/src/main/java/io/servicetalk/http/api/PartitionedHttpClientBuilder.java Outdated Show resolved Hide resolved
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/AlpnServerContext.java Outdated Show resolved Hide resolved
...p-netty/src/main/java/io/servicetalk/http/netty/DefaultMultiAddressUrlHttpClientBuilder.java Outdated Show resolved Hide resolved
@Scottmitch
Copy link
Member Author

build failure attributed to #1224

@Scottmitch
Copy link
Member Author

build failure attributed to #999

idelpivnitskiy
idelpivnitskiy approved these changes Mar 8, 2021
View reviewed changes
Copy link
Member

@idelpivnitskiy idelpivnitskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after the last comments addressed:

servicetalk-http-api/src/main/java/io/servicetalk/http/api/MultiAddressHttpClientBuilder.java Show resolved Hide resolved
...p-netty/src/main/java/io/servicetalk/http/netty/DefaultMultiAddressUrlHttpClientBuilder.java Show resolved Hide resolved
...p-netty/src/main/java/io/servicetalk/http/netty/DefaultMultiAddressUrlHttpClientBuilder.java Outdated Show resolved Hide resolved
...p-netty/src/main/java/io/servicetalk/http/netty/DefaultMultiAddressUrlHttpClientBuilder.java Show resolved Hide resolved
...ttp-netty/src/main/java/io/servicetalk/http/netty/DefaultSingleAddressHttpClientBuilder.java Outdated Show resolved Hide resolved
...etalk-transport-api/src/main/java/io/servicetalk/transport/api/AbstractSslConfigBuilder.java Show resolved Hide resolved
...etalk-transport-api/src/main/java/io/servicetalk/transport/api/AbstractSslConfigBuilder.java Show resolved Hide resolved
...etalk-transport-api/src/main/java/io/servicetalk/transport/api/AbstractSslConfigBuilder.java Outdated Show resolved Hide resolved
...icetalk-transport-api/src/main/java/io/servicetalk/transport/api/ClientSslConfigBuilder.java Show resolved Hide resolved
Scottmitch added 11 commits March 10, 2021 10:38
@Scottmitch
Add SslConfig APIs and fix SNI and SSL hostname bugs
79bbcf5 
Motivation:
The server SSL config APIs for SNI didn't allow configuring SSL
associated with each individual SNI host. The client APIs coupled the
host name verification algorithm with the non-authoritative peer
host/port. This doesn't correctly represent the semantics of SSL.

Modifications:
- Deprecate all APIs related to SecurityConfigurator and secure()
  builder method. The secure() builder methods require that users call
  commit() which is easy to miss if the builder is used in a conditional
  block, and prototypes of a composable server SNI API results in code
  that is difficult to read and understand which configurations apply.
- Add SslConfig interface types and builders that can be used to create
  the ssl configuration independent of the protocol builders. This
  allows for folks to configure ssl independently and we can overlay any
  values which maybe automatically inferred in the protocol builder
  (e.g. peerHost on the client, alpn protocols, etc.).
- Server SSL allows for providing SNI configuration, and Netty's
  DomainWildcardMappingBuilder is used to match hostnames.
- Client side SSL config object removes methods which couple the host
  name verification algorithm to the non-authoritative peer host/port,
  and adds new methods which allow setting them independently.
- Update all tests to enable host name verification, and avoid
  deprecated method use when possible.

Result:
SSL config APIs now correctly represent SNI semantics for client and
server. The client SSL config decouples the non-authoritative peer
host/port from the verification algorithm.

The deprecated types and methods will be removed in a future release.
@Scottmitch
fix javadoc checkstyle issue
7bcf725
@Scottmitch
review comments
345a14d
@Scottmitch
update javadocs to clarify reuse is expected
6daa37b
@Scottmitch
more javadoc updates
f3e5aa9
@Scottmitch
review comments
680d52c
@Scottmitch
review comments
1ebda05
@Scottmitch
update javadocs
cf000b2
@Scottmitch
review comments
651da05
@Scottmitch
always apply default trust manager for multi-client
ba1d565
@Scottmitch
remove unused imports
c4be97a
This was referenced Mar 10, 2021
Closed
Open
@Scottmitch Scottmitch merged commit 50079f7 into apple:main Mar 11, 2021
@Scottmitch Scottmitch deleted the http_sni2 branch March 11, 2021 01:19
idelpivnitskiy added a commit to idelpivnitskiy/servicetalk that referenced this pull request Mar 9, 2022
@idelpivnitskiy
Decouple multi-address and partitioned client builders from HttpClien…
e138041 
…tBuildContext

Motivation:

`DefaultMultiAddressHttpClientBuilder` and
`DefaultPartitionedHttpClientBuilder` use
`DefaultSingleAddressHttpClientBuilder` as a template for underlying
single-address clients. It was helpful before we introduced a
`SingleAddressInitializer` in apple#1387. Before that, all builders had the
same set of methods and it was nice to reuse the single-address builder
instead of maintaining a similar state at higher level builders. Now,
`SingleAddressInitializer` helps to avoid duplication between builders.
We don't have to use `DefaultSingleAddressHttpClientBuilder` as a
template and can simplify its state by using a factory for
`SingleAddressHttpClientBuilder`.

Modifications:

- `DefaultMultiAddressUrlHttpClientBuilder` and
`DefaultPartitionedHttpClientBuilder` use a factory for
`SingleAddressHttpClientBuilder` instead of
`DefaultSingleAddressHttpClientBuilder` as a template and
`HttpExecutionContextBuilder` for context-related items;
- Remove and hide methods from `DefaultSingleAddressHttpClientBuilder`
that are not used by other builders anymore;
- Add a test to verify `DefaultMultiAddressUrlHttpClientBuilder` sets
execution context items for underlying single-address builders;

Result:

`DefaultSingleAddressHttpClientBuilder` has less pkg-private API,
`DefaultMultiAddressUrlHttpClientBuilder` and
`DefaultPartitionedHttpClientBuilder` create a new single-address
builder on demand, similar to grpc builders.
@idelpivnitskiy idelpivnitskiy mentioned this pull request Mar 9, 2022
Merged
idelpivnitskiy added a commit that referenced this pull request Mar 9, 2022
@idelpivnitskiy
Decouple multi-address and partitioned client builders from `HttpClie…
a93fc9b 
…ntBuildContext` (#2136)

Motivation:

`DefaultMultiAddressHttpClientBuilder` and
`DefaultPartitionedHttpClientBuilder` use
`DefaultSingleAddressHttpClientBuilder` as a template for underlying
single-address clients. It was helpful before we introduced a
`SingleAddressInitializer` in #1387. Before that, all builders had the
same set of methods and it was nice to reuse the single-address builder
instead of maintaining a similar state at higher level builders. Now,
`SingleAddressInitializer` helps to avoid duplication between builders.
We don't have to use `DefaultSingleAddressHttpClientBuilder` as a
template and can simplify its state by using a factory for
`SingleAddressHttpClientBuilder`.

Modifications:

- `DefaultMultiAddressUrlHttpClientBuilder` and
`DefaultPartitionedHttpClientBuilder` use a factory for
`SingleAddressHttpClientBuilder` instead of
`DefaultSingleAddressHttpClientBuilder` as a template and
`HttpExecutionContextBuilder` for context-related items;
- Remove and hide methods from `DefaultSingleAddressHttpClientBuilder`
that are not used by other builders anymore;
- Add a test to verify `DefaultMultiAddressUrlHttpClientBuilder` sets
execution context items for underlying single-address builders;

Result:

`DefaultSingleAddressHttpClientBuilder` has less pkg-private API,
`DefaultMultiAddressUrlHttpClientBuilder` and
`DefaultPartitionedHttpClientBuilder` create a new single-address
builder on demand, similar to grpc builders.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@idelpivnitskiy idelpivnitskiy idelpivnitskiy approved these changes

@bondolo bondolo bondolo approved these changes

@tkountis tkountis Awaiting requested review from tkountis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Scottmitch @bondolo @idelpivnitskiy