petrov-mg commented on PR #13751:
URL: https://github.com/apache/ignite/pull/13751#issuecomment-6441139701
I do not think the proposed approach is valid.
1. Since we are implementing certificate reload, the SSL context factory
SHOULD NOT cache the SSL context. Caching should be the job of a separate
holder of the SSL context, the same component that will be responsible for
context reload. The factory should build a new context on every call.
As I have already mentioned, `createNew()` looks awkward in the current
implementation.
2. In my opinion, we should not pass the certificate chain to
`SSLContextWrapper`. Its only purpose is to apply default SSL parameters.
What we do need is a separate class that holds the `SSLContext` together
with the public certificates, e.g. `SslBundle`.
3. `KeyMaterial` is built from both key managers and a key store, which is
confusing, and its purpose is unclear. `KeyMaterial` should represent only a
loaded key store.
The factory then builds the key managers from it, so
`AbstractSslContextFactory` can provide an implementation of
`createKeyManagers()` based on it.
4. It seems that we need to store the certificate chains for all key store
aliases, not only for a single one.
As a result, `AbstractSslContextFactory` gains two abilities:
1. to be built from key material, which is what makes item 2 possible;
2. to return the SSL context together with the certificates it was built
from.
Here is the class structure I think we should consider:
**KeyMaterial**
```
┌────────────────────────────────────────────────────────────────┐
│ KeyMaterial │
├────────────────────────────────────────────────────────────────┤
│ KeyStore store │
│ char[] keyPwd │
│ String algorithm │
├────────────────────────────────────────────────────────────────┤
│ KeyManager[] keyManagers() │
│ Map<String, List<X509Certificate>> certificateChains() │
└────────────────────────────────────────────────────────────────┘
```
**AbstractSslContextFactory**
```
┌────────────────────────────────────────────────────────────────┐
│ AbstractSslContextFactory │
├────────────────────────────────────────────────────────────────┤
│ String proto │
│ String[] cipherSuites │
│ String[] protocols │
├────────────────────────────────────────────────────────────────┤
│ KeyMaterial loadKeyMaterial() │
│ KeyManager[] createKeyManagers() │
│ TrustManager[] createTrustManagers() │
│ void checkParameters() │
│ SslBundle createSslBundle() │
│ SslBundle load() │
│ SSLContext create() │
└────────────────────────────────────────────────────────────────┘
```
**SslBundle**
```
┌────────────────────────────────────────────────────────────────┐
│ SslBundle │
├────────────────────────────────────────────────────────────────┤
│ SSLContext ctx │
│ Map<String, List<X509Certificate>> chains │
├────────────────────────────────────────────────────────────────┤
│ SSLContext context() │
│ Map<String, List<X509Certificate>> certificateChains() │
└────────────────────────────────────────────────────────────────┘
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
