nevzheng commented on PR #12389:
URL: https://github.com/apache/gravitino/pull/12389#issuecomment-5227686898
## suggestion (non-blocking): Follow-up coverage for implemented HTTPS/mTLS
edges outside M1
M1 handshake coverage (S1–S7) looks complete for merge — this is **not** a
merge blocker. Digging the shipped `JettyServer` / `JettyServerConfig` path
against the new tests + `docs/security/how-to-use-https.md` / server config
docs, these behaviors are **implemented but still untested** (and several are
also undocumented). Highest-value follow-ups for M2 hardening:
### Highest value (please consider next)
1. **S8 — missing keystore fail-fast**
`enableHttps=true` without `keyStorePath` / `keyStorePassword` /
`managerPassword` already fails in `JettyServerConfig.fromConfig`
(`ConfigEntry` not-blank, no defaults) and again in `JettyServer.initialize`
(`Preconditions`). Symmetric to the covered truststore case
(`testClientAuthRejectsMissingTrustStore`), but the keystore trio has no test.
2. **S9 — HTTPS XOR HTTP**
Code path is a hard `if (enableHttps) { https connector } else { http
connector }` — only one listens. Docs already state this
(`how-to-use-https.md`, `gravitino-server-config.md`), but there is no
regression that `http://host:httpPort` is dead once HTTPS is on. Easy,
high-signal pin of a product invariant operators depend on.
3. **S11 — explicit `keyStoreType` / `trustStoreType=PKCS12`**
Fixtures are PKCS12 (`.p12`) while config defaults remain `JKS`, and
today’s JDK (17 here) loads the stores under either type. That tolerance hides
a M2 footgun: helpers/`TestTlsServerUtils` never set store type. Prefer an
explicit PKCS12 happy path (and ideally a typed mismatch negative) so CI does
not depend on JKS↔PKCS12 leniency.
### Stronger undocumented + untested finds
4. **S10 — unusable keystore at start**
Non-blank but wrong path / wrong password / unreadable store fails later
at Jetty SSL init / `start()`, not at `fromConfig`. Failure mode is
implemented, not documented, not tested.
5. **`enableClientAuth` without `enableHttps`**
Client-auth + truststore gates only run when **both** flags are true.
`enableClientAuth=true` alone is a silent no-op (no truststore required, no
`NeedClientAuth`). Undocumented operator footgun; untested.
6. **Invalid `enableCipherAlgorithms` / `tlsProtocol` soft-fail**
Unknown ciphers are intersected away (`getSupportedAlgorithms`); invalid
`tlsProtocol` yields a null `SSLContext` and an empty suite set. If the
effective set is empty, `SslContextFactory` never gets
`setIncludeCipherSuites`, so Jetty keeps defaults — restriction silently does
not apply. Filter math is partially unit-tested in `TestJettyServerConfig`; the
runtime “empty ⇒ no restriction” consequence is neither documented nor
handshake-tested.
### Lower urgency (same code path / later milestones)
- **S12** — hostname/SAN negative (fixtures use `DNS:localhost`; no mismatch
case).
- **S13** — Iceberg REST HTTPS via the same `JettyServer` + different conf
prefix (docs claim parity; no dedicated handshake test).
- **S14/S15** — intentional protocol/cipher policy — deferred by epic
(M5-ish).
**Ask:** non-blocking follow-up issue or patch for **S8 + S9 + S11** first;
S10 and the silent `enableClientAuth`/cipher soft-fails are the best
*undocumented* additions if you want extra belt-and-suspenders before M2. Happy
to leave M1 as-is.
Nevin
Sent from my 🤖 (Cursor)
--
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]
