HDDS-17030. DefaultApprover.inspectCSR never completes its future when the CSR passes inspection

[smengcl]

Generated-by: Claude Code (Opus 5.5)

What changes were proposed in this pull request?

CertificateApprover.inspectCSR returns a future that should hold Void for a valid CSR and an exception otherwise. DefaultApprover.inspectCSR only ever completed it exceptionally, so for a CSR that passed inspection the future stayed incomplete, and DefaultCAServer.requestCertificate read "incomplete" as approved. A failed check also did not stop the inspection: the remaining checks still ran on that CSR.

This PR:

• completes the future with null once every check passes
• returns right after the first failed check
• makes DefaultCAServer.requestCertificate reject a CSR whose inspection has not completed, instead of signing it

DefaultApprover is the only CertificateApprover and its inspection is synchronous, so certificate issuance behaves as before for valid and invalid CSRs. The first failure is reported exactly as before; only the error logs of the checks that used to run after it are gone.

Follow up from the review of #11486.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-17030

How was this patch tested?

• New TestDefaultProfile.testInspectCsrCompletesWhenApproved, TestDefaultProfile.testInspectCsrStopsAtFirstFailure and TestDefaultCAServer.testRequestCertificateRejectsUnfinishedInspection. All three fail without the change in src/main.
• TestDefaultCAServer, TestDefaultProfile and TestRootCARotationManager pass, and checkstyle and PMD are clean on hdds-server-framework.

🤖 Generated with Claude Code

[Copilot]

🟢 Approval recommended

No unresolved issues were identified that would block approval.

0 open findings

What changed in this PR

Fixes CSR approval futures so valid inspections complete successfully, failed checks stop immediately, and unfinished inspections cannot result in certificate issuance.

Changes:

• Completes successful DefaultApprover inspections.
• Stops validation after the first failure.
• Rejects incomplete inspections in DefaultCAServer.

File	Description
hadoop-hdds/​framework/​src/​test/​java/​org/​apache/​hadoop/​hdds/​security/​x509/​certificate/​authority/​TestDefaultProfile.java	Updated as part of this pull request.
hadoop-hdds/​framework/​src/​test/​java/​org/​apache/​hadoop/​hdds/​security/​x509/​certificate/​authority/​TestDefaultCAServer.java	Updated as part of this pull request.
hadoop-hdds/​framework/​src/​main/​java/​org/​apache/​hadoop/​hdds/​security/​x509/​certificate/​authority/​DefaultCAServer.java	Updated as part of this pull request.
hadoop-hdds/​framework/​src/​main/​java/​org/​apache/​hadoop/​hdds/​security/​x509/​certificate/​authority/​DefaultApprover.java	Updated as part of this pull request.

🧠 Review effort: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jojochuang]

Reviewer notes (plain language)

Simple examples

Good CSR (valid key, profile OK)

• Before: every check passed, but the inspection future never completed with success — it stayed incomplete. The CA server only looked for failure, so “not failed” → sign anyway.
• After: future completes with null (= approved) → sign. Same outcome for operators, correct API behavior.

Bad CSR (e.g. signature does not match key)

• Before: first check fails → future completes exceptionally → no cert. Later checks still ran (extra logs).
• After: fail fast on first error → no cert. Same denial, less noise.

Broken / unfinished inspection (e.g. future never completes)

• Before: incomplete future → treated like approval path → could sign without a finished inspection (latent bug; mocked in testRequestCertificateRejectsUnfinishedInspection).
• After: !isDone() → reject with “CSR inspection did not complete.” Fail closed.

Analogy: bouncer must say “OK” or “no”; the clerk used to stamp if they didn’t hear “no.” Now they require an explicit “OK” (completed success) or reject incomplete/failed inspection.

Bottom line

This is not a security risk on today’s production path with the synchronous DefaultApprover: invalid CSRs already failed exceptionally before signing; valid CSRs were already being signed. The patch fixes contract correctness, fail-fast on inspection, and defense in depth so incomplete inspection is never treated as approval (relevant for follow-up from #11486 and any future async approver). No config or expected behavior change for normal cert enrollment after upgrade.

[jojochuang]

LGTM. Fixes inspectCSR to complete successfully when all checks pass, fail fast on first error, and reject signing when inspection has not finished. Good unit coverage; behavior for sync DefaultApprover unchanged for normal valid/invalid CSRs — correctness and defense in depth, not a production security hotfix (see PR comment).

[jojochuang]

Merged. Thanks @smengcl

[smengcl]

Thanks @jojochuang for the review.