[#12299] feat(server-common): CI-enforced tests for server-side HTTPS and mTLS

[Octavi00]

[Subtask] M1: CI-enforced tests for server-side HTTPS and mTLS
#12299

What changes were proposed in this pull request?

• Added TestHttpsServerAuthentication test file to verify server side HTTPS across all 7 requested tests.
• These changes introduce 6 new certificate fixtures for https server TLS/mTLS testing.
• Includes new README.md file for instructions on certificate regeneration + regen script file.
• Added TestTlsServerUtils to provide a reusable Jetty TLS server setup for future test modules
• Updated server-common/build.gradle.kts to publish test classes and fixtures via test artifacts configuration

Fix: #12299

Does this PR introduce any user-facing change?

No user facing changes

How was this patch tested?

Ran:
./gradlew rat
Passed
./gradlew :server-common:test --tests "org.apache.gravitino.server.web.TestHttpsServerAuthentication"
Passed

[nevzheng]

Bugbot (Cursor) on behalf of Nevin

Automated Bugbot review of this PR (branch changes vs main).

Result: no bugs found.

Scope checked: TLS/mTLS handshake tests, fixture layout, testArtifacts publishing, and helper reuse for downstream client subtasks.

Local verification also green:

./gradlew :server-common:test --tests "org.apache.gravitino.server.web.TestHttpsServerAuthentication"
BUILD SUCCESSFUL

gravitino-server-common-*-tests.jar contains TestTlsServerUtils, TestHttpsServerAuthentication, and all tls/*.p12 fixtures.

---

Nevin
Sent from my 🤖 (Cursor)

[nevzheng]

Overall

LGTM — thanks for a clean M1.

praise: Spec (#12299) followed closely, good style, and the seven handshake tests pin the right P0 CUJs. Keep shipping milestones like this and the TLS epic lands soon.

note (non-blocking): I did a deeper dive for other CUJs we might be missing against the shipped server HTTPS/mTLS path and didn’t find anything I’d classify as immediately actionable for this PR. We can iterate later if a gap shows up in M2+.

@roryqi PTAL

[roryqi]

Could u avoid uploading the binary files? You should generate them in the tests instead of uploading them into the repo. Repo should manage the text files instead of binary files. Although they are not large, it still isn't a good practice.

[github-actions]

Code Coverage Report

Overall Project	68.43%	🟢
Files changed	73.08%	🟢

Module	Coverage
aliyun	1.72%	🔴
api	49.37%	🟢
authorization-common	85.96%	🟢
aws	42.04%	🟢
azure	2.47%	🔴
catalog-common	9.92%	🔴
catalog-fileset	79.74%	🟢
catalog-glue	68.95%	🟢
catalog-hive	79.4%	🟢
catalog-jdbc-common	45.7%	🟢
catalog-jdbc-doris	81.8%	🟢
catalog-jdbc-mysql	79.33%	🟢
catalog-jdbc-postgresql	83.39%	🟢
catalog-jdbc-starrocks	79.16%	🟢
catalog-kafka	77.01%	🟢
catalog-lakehouse-generic	59.18%	🟢
catalog-lakehouse-hudi	79.1%	🟢
catalog-lakehouse-iceberg	85.86%	🟢
catalog-lakehouse-paimon	84.23%	🟢
catalog-model	77.72%	🟢
cli	44.48%	🟢
client-java	78.27%	🟢
common	52.57%	🟢
core	83.29%	🟢
filesystem-hadoop3	77.28%	🟢
flink	0.0%	🔴
flink-common	48.68%	🟢
flink-runtime	0.0%	🔴
gcp	14.12%	🔴
hadoop-auth	68.0%	🟢
hadoop-common	12.7%	🔴
hive-metastore-common	53.4%	🟢
iceberg-common	64.75%	🟢
iceberg-rest-server	75.03%	🟢
idp-basic	86.02%	🟢
integration-test-common	0.0%	🔴
jobs	62.92%	🟢
lance-common	31.75%	🔴
lance-rest-server	63.47%	🟢
lineage	53.02%	🟢
optimizer	83.24%	🟢
optimizer-api	21.95%	🔴
server	85.76%	🟢
server-common	79.19% -0.06%	🟢
spark	28.57%	🔴
spark-common	45.89%	🟢
tencent	69.84%	🟢
trino-connector	40.29%	🟢

Files

Module	File	Coverage
server-common	MetadataIdConverter.java	73.08%	🟢

[nevzheng]

Could u avoid uploading the binary files? You should generate them in the tests instead of uploading them into the repo. Repo should manage the text files instead of binary files. Although they are not large, it still isn't a good practice.

@roryqi I recommend just submitting the binary files as is. We do not need a more complex scheme to manage key files at this time. We have other concerns to address.

The principle I’d like to highlight here is to keep things as simple as they need to be. If we ask for a change to generate key files every time that isn’t required we just delay the project without a product requirement that we need to satisfy.

In my experience, storing test, artifacts alongside tests like this is perfectly normal.

can you elaborate on your concerns?

[roryqi]

Thanks for the explanation.

My concern is mainly about long-term maintainability. Binary fixtures are hard to review, diff, and version over time. If a certificate needs to be updated later, reviewers cannot easily tell what changed or whether the generated content still matches the documented intent.

I agree that committing the binary files is simpler for this PR, but I do not think it is a good practice for this repo. Since the PR already includes a regeneration script and README, I would prefer that we keep the source/configuration for generating the certificates in the repo and generate the binary artifacts during the test/setup process instead of committing them directly.

[Octavi00]

Thanks for the explanation.

My concern is mainly about long-term maintainability. Binary fixtures are hard to review, diff, and version over time. If a certificate needs to be updated later, reviewers cannot easily tell what changed or whether the generated content still matches the documented intent.

I agree that committing the binary files is simpler for this PR, but I do not think it is a good practice for this repo. Since the PR already includes a regeneration script and README, I would prefer that we keep the source/configuration for generating the certificates in the repo and generate the binary artifacts during the test/setup process instead of committing them directly.

Hi @roryqi, I see what you mean, as in you can't easily read a binary file as a reviewer. I'm assuming it's probably from a security standpoint of the repo that you don't want to accept an unreadable file from a PR. I think the simplest fix, if you are willing to keep a binary implementation, would be to accept my PR without the trustores and submit your own pr after inspecting my regen script and running it on your end, then uploading the result.

Before I had decided to use a stable fixture, I had attempted to implement generating a runtime trust store, but I had inquired if it was necessary after realizing it would take me some more time to research and learn what a good method would be. As of right now, I wouldn't recommend generating the test trust stores at runtime by trying to implement my regen script, as you would need to import X.509 api's, which would introduce a large dependency. I would also avoid using a bouncyhouse implementation too, simply because I think it would be possible to avoid adding any dependency at all if we use the JDK's keytool, which I'm already using to create the licenses for tests.

Using keytool, I could implement generating a runtime trust store within my Util file pretty cleanly since it's already getting published and available across different test classes. The only downside is the fact that it would theoretically slow down tests, as they would need to be generated at least once per test class.

I'm currently going to start working on M2 in the meantime, but if you'd like me to try the keytool approach, I could make the change.

[roryqi]

Thanks for the explanation. I do not think accepting the binary fixtures is the right direction here.

This repo is maintained collaboratively by many people, so test assets should be reviewable and maintainable by future contributors, not only by the original author. For certificates/truststores, the important part is not just whether the current files work, but whether future reviewers can understand and verify how they were produced and what changed when they are regenerated.

I am fine with using the JDK keytool approach to avoid adding dependencies. The test setup can generate the required keystores/truststores from text/config inputs, ideally once per test run or reused through a helper/temp directory to avoid unnecessary overhead.

So my preference is:

1. Do not commit the binary keystore/truststore fixtures.
2. Keep the generation source/script/config in the repo.
3. Generate the binary artifacts during test setup using JDK tooling.

Could you please try the keytool approach?

[nevzheng]

@roryqi @Octavi00 Logging my recommendation so the decision is clear.

I would approve this PR as-is, merge it, and move on.

On the binary fixtures: I do not think generating them at test setup adds enough value to justify reworking this PR. The maintainability concerns (provenance, regeneration, inspectability) are already addressed by regenerate.sh and the README. Reviewers should review that generation path and docs, not the .p12 diffs.

If we still want automated generation, please do it as a follow-up PR. That keeps this M1 diff focused and avoids delaying work that has already been blocked for a long time.

Part of our responsibility as reviewers is to keep work moving. Prefer comments that make the change materially better — favor approving once a CL clearly improves the system, even if it is not perfect, and balance the importance of the suggestion against the need to make forward progress. For me, runtime fixture generation does not clear that bar relative to the delay cost on this milestone.

[roryqi]

OK, thanks for the recommendation.

Since you are shepherding this issue, I will respect your judgment as much as possible. In general, I prefer to make a PR as complete as reasonably possible, especially because community contributors may not always be able to follow up later.

That said, I still think my concern about committing binary test fixtures is valid from a long-term maintainability and reviewability perspective. However, to avoid blocking this work and to let you move it forward in the way you think is most practical, I am willing to compromise on this point for this PR.

Let's merge this PR as-is, but we should optimize this in a follow-up PR by generating the keystore/truststore artifacts from reviewable source/configuration instead of committing the generated binaries.

[nevzheng]

@roryqi agreed thanks. @Octavi00 can you write a followup issue. We can prioritize from there. thanks.