ZOOKEEPER-4922: Audit JAAS login, logout and Kerberos ticket renewal

[ikxeno]

Problem

JAAS login, logout and Kerberos ticket renewal are missing from the audit log.

Fix

Add login, logout and ticketRenewal events through the existing audit provider, enabled by zookeeper.audit.enable. These record local JAAS operations; incoming client and learner SASL authentication failures are outside this patch's scope.

• Events contain user, operation and result. Identity comes from the subject's Kerberos principal or DIGEST username, with configured JAAS identity and OS user as fallbacks. Failed renewals retain the last authenticated identity.
• Each failed renewal attempt is audited, including retries. Skipped renewals emit nothing. Re-login also records its JAAS logout; stopping the refresh thread alone does not.
• RuntimeException and LinkageError from the audit logger leave the JAAS result unchanged. Initialization failures are retained so later calls can report the original cause.
• Servers validate audit initialization before starting autopurge or server services, including embedded servers. If auditing a failed start also fails, the server keeps its intended exit code.
• The logger is fixed per JVM. Disabled client logins leave it uninitialized. An embedded server accepts the selected logger class and rejects a conflicting one.

The audit docs describe this behavior and mark the feature new in 3.10.0.

Tests

Tests cover identity selection, event ordering, renewal retries and skips, disabled auditing, logger failures, and configuration during initialization. Startup tests check exit codes and rejection before autopurge starts.

Both RuntimeException and LinkageError are tested for successful-login and failed-start audit writes. The other logger write failure tests use RuntimeException.

Locally, 48 tests pass on Java 17 and 25. SpotBugs and Checkstyle also pass.

[anmolnar]

Just a quick question, looks good to me regardless. I wonder if I can think of a better way of lazy initialization, though I'm okay with that nonetheless.

[anmolnar]

Claude's comments on the patch:

1. Scope may not match what auditors expect.
These events record the local process logging in with its own keytab or JAAS section. They don’t cover inbound client or learner SASL authentication failures on the server. Someone reading “authentication failures have no audit event” will probably assume they now get failed-client-auth auditing, and they won’t. The PR description and docs should say so explicitly.

2. The user field is misleading for non-Kerberos logins.
With DigestLoginModule (quorum or client DIGEST-MD5), the subject has no KerberosPrincipal, so the event logs the OS user (Login.SYSTEM_USER), not the JAAS username. LoginAuditInitializationTest asserts exactly this. An audit line that names someone other than the identity that authenticated is worse than no name. I’d either use the digest username or log something like explicitly.

3. Failures in the audit logger now surface later and less clearly.
Before, a broken audit.impl.class failed the server at startup with “Couldn’t instantiate X”. Now, if a client Login in the same JVM triggers initialization first, the ExceptionInInitializerError is swallowed and logged. Every later isAuditEnabled() call then throws NoClassDefFoundError, including the calls in AuditHelper on the request path. This only affects embedded-server setups, but the resulting error is hard to diagnose. Consider remembering the initialization failure and rethrowing a clear exception from isAuditEnabled() or at server start.

4. Renewal events are counted per attempt, not per renewal.
The kinit loop audits every failed attempt, retries included. A single renewal that fails three times before succeeding produces three failure lines and one success line. It’s documented, but SIEM-style alerting may read that as four separate renewals. Worth agreeing on deliberately.

5. Relogin produces a pair of events.
A successful relogin emits logout success followed by ticketRenewal success. The docs explain this, but anyone counting logouts will be thrown off. An alternative is to drop the inner logout event during relogin.

6. Backports aren’t trivial.
The pattern-matching instanceof requires Java 16+, which is fine on master (release 17) but won’t compile on branch-3.9. The new public API (configureAuditLogger, public log(user, op, result), public AUDIT_ENABLE) is also a 3.10-only surface to keep in mind.

7. The tests are heavy. The tests use reflection on the private reLogin(), static mutable flags in the test login module and logger, and a 10-second latch in the concurrency test. They’re thorough, but some could probably be simplified with a package-private hook.

[anmolnar]

Backports are not necessarily a problem, but I think other points are worth to think about. Especially with package-private fields you can easily make the tests more lightweight. It's a common practice across the code.

[ikxeno]

@anmolnar Thanks for checking this with Claude. I've made the fixes.

1. Agreed, the wording was too broad. The docs now explicitly limit this to local JAAS events, excluding incoming SASL failures.

2. Fixed DIGEST: the code now also reads the subject's username.

3. Fixed the holder issue: initialization failures are retained, and later calls and server startup report the original cause.

4. I kept per-attempt failures. Failed attempts are still useful to see when a retry succeeds.

5. I kept logout too, since re-login actually logs out of JAAS. The docs already distinguish this from a client disconnect.

6. Removed reflection, isolated JAAS test state, and trimmed 40 cases to 34. The latch was already a timeout

[anmolnar]

Thanks for the quick response. Here're a few new comments, but all of them are nitpicks, we can merge w/o addressing them once CI is green.

New issue

• The failure-path audit call can throw. QuorumPeerMain.main and ZooKeeperServerMain.main catch the startup exception and then call ZKAuditProvider.addServerStartFailureAuditLog(). That method calls isAuditEnabled(), which now throws again inside the catch block. As a result, requestSystemExit(ExitCode...) is skipped and main ends with an uncaught exception instead of the intended exit code. It’s only cosmetic: the JVM still exits non-zero and the cause gets logged twice. Master had the same problem in a different form (ExceptionInInitializerError escaping main), so this isn’t a regression. Still, wrapping addServerStartFailureAuditLog() in a try/catch is a one-line fix.

Test nits

• The LinkageError parameterization was dropped from the failure-path tests (failed login, logout and renewal). Only the successful-login test still checks it. The code path is the same, so the risk is low, but it is less coverage than the PR description claims.
• testEmbeddedServerAcceptsSelectedAuditLogger lost its serverStart assertion. It now only checks that build() doesn’t throw.

[ikxeno]

Thanks, Andor, for your time and the careful review.

I've updated the description to match the reduced tests. I kept the tests as they are: both exception types exercise the same handler, and the embedded-server test checks logger configuration, so build() is enough for that case.

The startup error-path fix can go separately, since it also affects master.

[ikxeno]

I ended up including the exit-code fix here after all. While checking it, I found that autopurge could delete old snapshots before startup failed on a broken audit logger. That ordering predates this PR. Both CLI and embedded startup now check auditing before starting purge.

[anmolnar]

lgtm.

[ikxeno]

Could someone rerun the failed CI job? The previous run passed. SaslAuthFailFipsModeTest hit the same race seen in #2469, similar to ZOOKEEPER-1799. I’ll file a ticket for this test.