-
Notifications
You must be signed in to change notification settings - Fork 912
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
Provide a way to dynamically update TLS certificates #5228
base: main
Are you sure you want to change the base?
Conversation
Hi, @ikhoon ! |
core/src/main/java/com/linecorp/armeria/client/HttpClientFactory.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/TlsProviderBuilder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/TlsProviderBuilder.java
Outdated
Show resolved
Hide resolved
The priority of this issue has been raised. Please let me finish this PR. 🙇♂️ |
I'm sorry to be late for this reply. |
No worries. I’m really grateful for your contribution. 🙇♂️ |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5228 +/- ##
============================================
- Coverage 74.75% 73.62% -1.14%
+ Complexity 21409 20168 -1241
============================================
Files 1877 1748 -129
Lines 79126 74634 -4492
Branches 10201 9526 -675
============================================
- Hits 59152 54946 -4206
+ Misses 15179 15139 -40
+ Partials 4795 4549 -246 ☔ View full report in Codecov by Sentry. |
This PR is ready to review. PTAL. The PR description has also been updated for the API design and implementation details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! Server side looks good to me 👍 Left some minor questions on the client side 🙇
core/src/main/java/com/linecorp/armeria/common/metric/EventLoopMetrics.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/HttpChannelPool.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/HttpChannelPool.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks promising! 👍
core/src/main/java/com/linecorp/armeria/common/metric/EventLoopMetrics.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some preliminary comments, looks good overall 👍
core/src/main/java/com/linecorp/armeria/internal/common/TlsProviderUtil.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/metric/CertificateMetrics.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went through the client-side changes. Will take a look at the rest of the code next week 🙇
|
||
Bootstraps(HttpClientFactory clientFactory, EventLoop eventLoop, SslContext sslCtxHttp1Or2, | ||
SslContext sslCtxHttp1Only) { | ||
Bootstraps(HttpClientFactory clientFactory, EventLoop eventLoop, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: At first I wasn't sure whether TlsProvider
should be called 1) on each connection open 2) or on each request (so TlsCacheContext
is added to PoolKey
).
After thinking about the most common use-cases, I think the current approach is more performant and probably covers most use-cases.
i.e.
- If a server is simply advertising its validity, then I users can just use
maxConnectionAge
and allow clients to change the used tls certificate gradually. - If mTLS is used for authorization, new connections will probably be authorized anyways.
It may be difficult to add a tls
related API to the WebClient
level with this approach though. (we may also consider a hybrid approach where both the connection and poolkey is taken into account if needed in the future)
@Deprecated | ||
default boolean allowsUnsafeCiphers() { | ||
return false; | ||
} | ||
|
||
/** | ||
* Returns the {@link Consumer} which can arbitrarily configure the {@link SslContextBuilder} that will be | ||
* applied to the SSL session. | ||
*/ | ||
default Consumer<SslContextBuilder> tlsCustomizer() { | ||
return builder -> {}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question) What do you think of modifying the purpose of TlsProvider
to return a SslContext
instead of a KeyPair
?
By taking this approach, we can 1) delegate the cache logic in TlsProviderUtil
to users if needed 2) we potentially don't need to make breaking changes in ClientFactory
3) maybe just me, but the purpose of this interface seems clearer.
e.g.
class TlsProvider {
SslContext find(String hostname, boolean allowsUnsafeCiphers, Consumer<SslContextBuilder> tlsCustomizer);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will move the responsibility of managing the life cycle of SslContext
to TlsProvider
, which may complicate things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several difficulties in doing so.
TlsProvider
is designed to be used at both client and server. However,SslContext
has different properties for them, e.g.,SslContextBuilder.forClient()
andSslContextBuilder.forServer()
.SslContext
for a client should not be returned for a server.- Creating a
SslContext
in Armeria optimized for HTTP/2 and TLS 1.3. Users who implement other ownTlsProvder
may feel difficult to buildSslContext
or we need to expose a factory method to easily create it. Instead, creatingTlsKeyPair
is straightforward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will move the responsibility of managing the life cycle of SslContext to TlsProvider, which may complicate things.
What do you think of modifying the method for creating the SSL context so that the TlsProvider
doesn't have to manage the lifecycle at all? Here's a proposed interface change:
interface TlsProvider {
SslContext createSslContext(String hostname, SessionProtocol sessionProtocol, TlsEngineType tlsEngineType);
}
I'm suggesting this because it seems like the current TlsProvider
functions more as a data class than as a provider.
TlsProvider is designed to be used at both client and server.
At present, we can use TlsProvider.ofServer()
for the client side as well. By validating the TlsProvider
when it's assigned to a client or a server, we can prevent this misuse.
If you still prefer to use TlsProvider
for both, we could add enum parameters to TlsEngineType
such as CLIENT_JDK
, SERVER_JDK
, CLIENT_OPENSSL
, etc.
Users who implement their own TlsProvider may find it difficult to build SslContext, or we might need to expose a factory method to make it easier.
That is true. I also worry about exposing Netty's SslContext API to the public. However, creating an SslContext might not be too difficult since Netty provides a well-designed SslContextBuilder API. To address the issue of exposing the API, we could provide our own abstraction, similar to what we did with DnsEndpointGroupBuilder
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still questioning about exposing an API that creates SslContext
.
- The API forces all users who want to implement
TlsProvider
to learn how to buildSslContext
. - With the current API, users only need to convert their certificates using
TlsKeyPair
's factory method for basic usage. - Advanced users may change SslContext using the customizer.
I prefer the current style because would be more convenient for most users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are a couple of points here:
On the argument of returning SslContext
vs TlsKeyPair
Honestly I'm find with either way. Later on, I think we can allow a higher level API if users want
On TlsProvider
methods
Say that I am a user implementing a custom TlsProvider
- I'm not sure if it is possible to gracefully change certificates.
e.g. If I want from TlsKeyPair1
where allowsUnsafeCiphers=true
should be guaranteed, to TlsKeyPair2
where allowsUnsafeCiphers=false
, can I do this atomically? Is it possible that TlsKeyPair2
is sent with allowsUnsafeCiphers=true
?
public interface TlsProvider {
boolean allowsUnsafeCiphers()
TlsKeyPair find(String hostname)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I split the TLS configuration parts from TlsProvider
and added ServerTlsConfig
and ClientTlsConfig
. Those configurations are immutable objects which return a constant allowsUnsafeCiphers
and other values.
@Deprecated | ||
default boolean allowsUnsafeCiphers() { | ||
return false; | ||
} | ||
|
||
/** | ||
* Returns the {@link Consumer} which can arbitrarily configure the {@link SslContextBuilder} that will be | ||
* applied to the SSL session. | ||
*/ | ||
default Consumer<SslContextBuilder> tlsCustomizer() { | ||
return builder -> {}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will move the responsibility of managing the life cycle of SslContext
to TlsProvider
, which may complicate things.
core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/AbstractTlsProviderBuilder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java
Outdated
Show resolved
Hide resolved
@Deprecated | ||
default boolean allowsUnsafeCiphers() { | ||
return false; | ||
} | ||
|
||
/** | ||
* Returns the {@link Consumer} which can arbitrarily configure the {@link SslContextBuilder} that will be | ||
* applied to the SSL session. | ||
*/ | ||
default Consumer<SslContextBuilder> tlsCustomizer() { | ||
return builder -> {}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will move the responsibility of managing the life cycle of SslContext to TlsProvider, which may complicate things.
What do you think of modifying the method for creating the SSL context so that the TlsProvider
doesn't have to manage the lifecycle at all? Here's a proposed interface change:
interface TlsProvider {
SslContext createSslContext(String hostname, SessionProtocol sessionProtocol, TlsEngineType tlsEngineType);
}
I'm suggesting this because it seems like the current TlsProvider
functions more as a data class than as a provider.
TlsProvider is designed to be used at both client and server.
At present, we can use TlsProvider.ofServer()
for the client side as well. By validating the TlsProvider
when it's assigned to a client or a server, we can prevent this misuse.
If you still prefer to use TlsProvider
for both, we could add enum parameters to TlsEngineType
such as CLIENT_JDK
, SERVER_JDK
, CLIENT_OPENSSL
, etc.
Users who implement their own TlsProvider may find it difficult to build SslContext, or we might need to expose a factory method to make it easier.
That is true. I also worry about exposing Netty's SslContext API to the public. However, creating an SslContext might not be too difficult since Netty provides a well-designed SslContextBuilder API. To address the issue of exposing the API, we could provide our own abstraction, similar to what we did with DnsEndpointGroupBuilder
.
core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java
Show resolved
Hide resolved
This PR is ready to review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! 👍 👍 👍 Left minor comments and questions. 🙇
core/src/main/java/com/linecorp/armeria/client/ClientTlsConfigBuilder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/TlsProvider.java
Outdated
Show resolved
Hide resolved
|
||
final ImmutableList<X509Certificate> trustedCerts = x509CertificateBuilder.build(); | ||
if (keyPairMappings.size() == 1 && keyPairMappings.containsKey("*")) { | ||
return new StaticTlsProvider(keyPairMappings.get("*"), trustedCerts); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To pass the NullAway?
return new StaticTlsProvider(keyPairMappings.get("*"), trustedCerts); | |
final TlsKeyPair tlsKeyPair = keyPairMappings.get("*"); | |
assert tlsKeyPair != null; | |
return new StaticTlsProvider(tlsKeyPair, trustedCerts); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me keep it as is since NullAway didn't complain about this.
core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/internal/common/SslContextFactory.java
Show resolved
Hide resolved
|
||
final Bootstrap baseBootstrap = isDomainSocket ? unixBaseBootstrap : inetBaseBootstrap; | ||
assert baseBootstrap != null; | ||
return newBootstrap(baseBootstrap, remoteAddress, desiredProtocol, serializationFormat); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, we should create bootstrap every time when connecting if sslContextFactory
is set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Because we need a new ChannelInitializer
for a different SslContext
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, I'm a bit worried about the performance. 🤔
Maybe @trustin can give some idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I saw Bootstrap
, I thought it was a simple class and did not refer to heavy objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 🙇 🙇 🙇
Motivation:
#5033
API design note:
TlsKeyPair
represents a pair ofPrivateKey
andX509Certificate
chain.TlsSetters
have been deprecated in favor ofTlsSetters tls(TlsKeyPair)
.TlsProvider
dynamically resolves aTlsKeyPair
for the given hostname when a connection is established.*
is used as a special hostname to get theTlsKeyPair
for the default virtual host.TlsKeyPair
, a customTlsProvider
can be implemented.ServerTlsConfig
andClientTlsConfig
are added to override the default values and customizeSslContextBuilder
.TlsProvider
,*TlsConfig
are immutable so allTlsKeyPair
s returned by aTlsProvider
buildSslContext
with the same configuration.TlsProvider
andTlsKeyPair
for TLS configurations.SslContext
dynamically.Modifications:
TlsProviderMapping
that convertsTlsProvider
into SslContextMapping
forSniHandler
.TlsProvider
can be used to update the certificates withoutServer.reconfigure()
.TlsProvider
toServerBuilder
.VirtualHost
isn't added because aTlsProvider
can contain multiple certificates.TlsProvider
at the virtual host level later.Bootstraps
to create aBootstrap
with aTlsKeyPair
returned byTlsProvider
when a new connection is created.TlsProvider
is set, the original behavior that returns predefinedBootStraap
is used.TlsProvider
toClientFactoryBuilder
.TlsProvider
provides separate builders for the client and server.TlsKeyPair
provides various factory methods to easily create a key pair from different resources.SslContext
s and expire them after 1 hour of inactivity.CloseableMeterBinder
to unregister when the associated resource is unused.TlsSetters tls(File keyCertChainFile, File keyFile)
TlsSetters tls(File keyCertChainFile, File keyFile, @Nullable String keyPassword)
TlsSetters tls(InputStream keyCertChainInputStream, InputStream keyInputStream)
TlsSetters tls(InputStream keyCertChainInputStream, InputStream keyInputStream, @Nullable String keyPassword)
TlsSetters tls(PrivateKey key, X509Certificate... keyCertChain)
TlsSetters tls(PrivateKey key, Iterable<? extends X509Certificate> keyCertChain)
TlsSetters tls(PrivateKey key, @Nullable String keyPassword, X509Certificate... keyCertChain)
TlsSetters tls(PrivateKey key, @Nullable String keyPassword, Iterable<? extends X509Certificate> keyCertChain)
Result:
ClientFactory
#5033TlsProvider