Skip to content

IGNITE-29209 Return fresh SSL bundles with certificate metadata - #13751

Open
anton-vinogradov wants to merge 34 commits into
apache:masterfrom
anton-vinogradov:ignite-29209
Open

anton-vinogradov wants to merge 34 commits into
apache:masterfrom
anton-vinogradov:ignite-29209

Conversation

@anton-vinogradov

@anton-vinogradov anton-vinogradov commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Fixes IGNITE-29209.

Build a fresh SSL context together with the certificate chains from the same loaded key material. Node reload needs this metadata to check replacements and report status; automatic renewal needs the expiry dates. Keeping both results together prevents a second store read from reporting certificates different from those used by the context.

AbstractSslContextFactory builds an SslBundle from loaded KeyMaterial; SslContextFactory supplies the store, key password and manager algorithm. The bundle pairs the initialized context with immutable, nonempty X.509 chains indexed by key-store alias. SSLContextWrapper handles TLS parameters only.

  • A null map means certificate metadata is unknown.
  • An empty map means no nonempty X.509 chains were found for key entries.
  • Every included chain is nonempty; SslBundle rejects empty lists from custom factories too.

KeyMaterial includes only key entries with nonempty chains consisting entirely of X.509 certificates. It skips entries without such a chain and does not extract keys or determine their type for reporting. The metadata does not prove that a private key can be recovered or that a connection will succeed.

Keep the loaded store unchanged while its contexts are in use: some key-manager providers read it during handshakes. An empty loaded store supports client contexts without private keys; null stores are not supported.

create() is the final Factory<SSLContext> entry point and returns createSslBundle().context(). Every call builds a fresh context. Consumers own the context they keep in use.

This changes the factory extension contract: the protected sslCtx cache and no-argument createKeyManagers() hook are removed, and create() can no longer be overridden. Update and recompile affected subclasses.

Move store loading into loadKeyMaterial(). For custom key managers, override createSslBundle() to initialize a fresh context with those managers, configure its trust and TLS settings, and return matching certificate metadata. For decoration of an already built context, call super.createSslBundle() and retain its certificate chains in the returned bundle.

A source providing a ready-made context can use a plain Factory<SSLContext> with unknown metadata. Callers that relied on repeated create() calls returning the same context must retain it themselves.

Communication retains its SSL context between connections. Before IGNITE-29210 introduces shared node ownership, each enabled TLS transport creates and retains its own context at startup; a shared factory therefore loads its stores separately for each transport.

The prepared node integration owns the current bundle and uses this API for replacement. It also accepts a plain Factory<SSLContext> without certificate metadata. Node reload rejects a known empty map and checks the dates of included chains; status reports those chains. A mixed store is not rejected merely because other entries were skipped.

Node reload and status, automatic renewal, and HTTP REST integration remain in IGNITE-29210, IGNITE-29211 and IGNITE-29212 respectively. This PR provides the factory API and metadata. Generated test stores use Bouncy Castle as a test-only dependency.

Local validation on f7e62f67: SslContextMetadataTest (10 tests), SslContextReuseTest (1), SSLContextWrapperTest (1) and TcpDiscoverySslParametersTest (8) passed, as did Checkstyle and Javadoc doclint.

On b21e67e1, which combines the API, manual reload, renewal and HTTP branches, 80 targeted SSL tests passed and one existing JMX variant was skipped; Checkstyle passed. This includes rejecting reload without certificate chains, accepting a mixed PKCS12 store and verifying its certificate on a new TLS connection, and renewal after removal of the earliest-expiring chain. These are local runs, not a full TeamCity RunAll.

RunAll 9448194 checked the earlier 8a99c99: the checker found no blockers and attributed none of its 22 failing tests to the PR. It does not validate the current head. A new RunAll is on hold at the owner's request.

@anton-vinogradov

Copy link
Copy Markdown
Contributor Author

/runall

@ignite-pr-checker

ignite-pr-checker commented Oct 26, 2026 •

Copy link
Copy Markdown

@anton-vinogradov Ignite PR Checker verdict · RunAll build 9444364 · tested ae42041 · 149 suites ran, 0 reused

⚠️ This run doesn't cover the PR fully:

  • mixed run — Queries 5 re-ran on bda9a61, the rest of the RunAll ran on ae42041; only a RunAll of the PR's head can prove it clean

Everything below is what it did manage to say.

🔎 No blockers found — but the run above can't prove the PR is clean. 19 failing tests are not blamed on this PR. Re-run after you handle what is listed above.

What each label means and what to do: verdict glossary.

The PR is now at bda9a61.

♻️ Settled after 1 auto re-run wave: #1 — 1 broken suite.

🔁 Superseded by the newer RunAll 9444873 — its verdict is on the checker's page.

@anton-vinogradov

Copy link
Copy Markdown
Contributor Author

/runall

@ignite-pr-checker

ignite-pr-checker commented Oct 27, 2026 •

Copy link
Copy Markdown

@anton-vinogradov Ignite PR Checker verdict · RunAll build 9444873 · tested 65c8098 · 149 suites ran, 0 reused

⚠️ This run doesn't cover the PR fully:

  • mixed run — Cache (Failover) 5 re-ran on fbf5c51, the rest of the RunAll ran on 65c8098; only a RunAll of the PR's head can prove it clean

Everything below is what it did manage to say.

🔎 No blockers found — but the run above can't prove the PR is clean. 19 failing tests are not blamed on this PR. Re-run after you handle what is listed above.

What each label means and what to do: verdict glossary.

The PR is now at cbef7aa.

♻️ Settled after 1 auto re-run wave: #1 — 2 suites that failed mid-run.

🔁 Superseded by the newer RunAll 9445513 — its verdict is on the checker's page.

@anton-vinogradov

Copy link
Copy Markdown
Contributor Author

/runall

@ignite-pr-checker

ignite-pr-checker commented Oct 27, 2026 •

Copy link
Copy Markdown

@anton-vinogradov Ignite PR Checker verdict · RunAll build 9445513 · tested cbef7aa · 149 suites ran, 0 reused

✅ No blockers — nothing in this run looks caused by this PR. 21 failing tests are not blamed on this PR.

What each label means and what to do: verdict glossary.

♻️ Settled after 1 auto re-run wave: #1 — 3 suites that failed mid-run.

🔁 Superseded by the newer RunAll 9447228 — its verdict is on the checker's page.

@petrov-mg

petrov-mg commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

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()                               │
│ SSLContext     create()                                        │
└────────────────────────────────────────────────────────────────┘

SslBundle

┌────────────────────────────────────────────────────────────────┐
│ SslBundle                                                      │
├────────────────────────────────────────────────────────────────┤
│ SSLContext ctx                                                 │
│ Map<String, List<X509Certificate>> chains                      │
├────────────────────────────────────────────────────────────────┤
│ SSLContext context()                                           │
│ Map<String, List<X509Certificate>> certificateChains()         │
└────────────────────────────────────────────────────────────────┘

@anton-vinogradov anton-vinogradov changed the title IGNITE-29209 Add fresh SSL contexts with certificate metadata IGNITE-29209 Return fresh SSL bundles with certificate metadata Oct 27, 2026
@anton-vinogradov

Copy link
Copy Markdown
Contributor Author

/runall

@ignite-pr-checker

ignite-pr-checker commented Oct 27, 2026 •

Copy link
Copy Markdown

@anton-vinogradov Ignite PR Checker verdict · RunAll build 9447228 · tested a360d09 · 149 suites ran, 0 reused

⚠️ This run doesn't cover the PR fully:

  • mixed run — Cache (Full API) and Consistency re-ran on fecbd9e, PDS 1 on b672056, the rest of the RunAll ran on a360d09; only a RunAll of the PR's head can prove it clean

Everything below is what it did manage to say.

🔎 No blockers found — but the run above can't prove the PR is clean. 25 failing tests are not blamed on this PR. Re-run after you handle what is listed above.

What each label means and what to do: verdict glossary.

The PR is now at fecbd9e.

♻️ Settled after 2 auto re-run waves: #1 — 2 suites that failed mid-run; #2 — 1 broken suite.

🔁 Superseded by the newer RunAll 9448194 — its verdict is on the checker's page.

@anton-vinogradov

Copy link
Copy Markdown
Contributor Author

/runall

@ignite-pr-checker

ignite-pr-checker commented Oct 28, 2026 •

Copy link
Copy Markdown

@anton-vinogradov Ignite PR Checker verdict · RunAll build 9448194 · tested 8a99c99 · 149 suites ran, 0 reused

✅ No blockers — nothing in this run looks caused by this PR. 22 failing tests are not blamed on this PR.

What each label means and what to do: verdict glossary.

♻️ Settled after 1 auto re-run wave: #1 — 2 suites that failed mid-run.

🔁 Superseded by the newer RunAll 9449478 — its verdict is on the checker's page.

@anton-vinogradov

Copy link
Copy Markdown
Contributor Author

/runall

@ignite-pr-checker

ignite-pr-checker commented Oct 28, 2026 •

Copy link
Copy Markdown

@anton-vinogradov Ignite PR Checker verdict · RunAll build 9449478 · tested 5f0c03f · 149 suites ran, 0 reused

✅ No blockers — nothing in this run looks caused by this PR. 26 failing tests are not blamed on this PR.

What each label means and what to do: verdict glossary.

♻️ Settled after 1 auto re-run wave: #1 — 2 suites that failed mid-run.

🔁 Superseded by the newer RunAll 9450236 — its verdict is on the checker's page.

@ignite-pr-checker

Copy link
Copy Markdown

@anton-vinogradov Ignite PR Checker verdict · RunAll build 9450236 · tested f7e62f6 · 149 suites ran, 0 reused

❌ 2 blockers in 2 suites:

  • Platform .NET (Core Linux) · SslConfigurationTest.TestConfigurationExceptions — failed 3 of 3 runs of this code · TC
  • Platform .NET (Windows) · SslConfigurationTest.TestConfigurationExceptions — failed 3 of 3 runs of this code · TC

What each label means and what to do: verdict glossary.

♻️ Settled after 2 auto re-run waves: #1 — 3 suites that failed mid-run; #2 — 2 suites with blockers.

➡️ Next: fix the blockers, push, and run RunAll again — a /run-all comment here does it when PR commands are on in the checker's ⚙.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants