ZOOKEEPER-5038: Migrate to Jetty 12.1.12 (EE10)

[IvanKhanas]

I maintain packages for ALT Linux, and this comes out of a downstream problem.
Our repository carries two branches of the same library, Jetty 12.1.8 and Jetty
9.4.58, and the 9.x package exists mostly because ZooKeeper needs it. Two
branches are expensive to keep, CVE triage above all. Since master builds with
release 17 the usual reason to stay on Jetty 9 no longer applies, and patching
consumers downstream is the worse option, so I would rather fix it upstream.

I found ZOOKEEPER-5038 and read the thread. The Java baseline blocker is gone
now that #2376 landed, which leaves EE8 versus EE10. This is EE10, with numbers.

Four commits, meant to be read in order:

1. Jetty 12 BOMs and jakarta.servlet-api 6.0.0, which the BOMs do not manage and
   which therefore needs its own pin. Does not compile on its own.
2. javax.servlet to jakarta.servlet across 17 files. Mechanical, 24 changed
   lines; import blocks move because checkstyle sorts jakarta before java.
3. The five files that needed real work: JettyAdminServer,
   UnifiedConnectionFactory, CommandListener, PrometheusMetricsProvider, and
   ReadAheadEndpoint, which is deleted.
4. LICENSE files, OWASP suppressions, Jenkinsfile-owasp and admin docs.

EE8 versus EE10

The EE8 option as described on the JIRA still requires updating AbstractHandler,
client.api, EndPoint and SslConnection. EE10 needs the same rewrites, and that
is where the regression risk sits. EE8 only saves the import rename, which
turned out to be the cheap part: one pass, 24 lines.

The rewrites

UnifiedConnectionFactory extends DetectorConnectionFactory, which does the byte
sniffing ReadAheadEndpoint was written for. The subclass only adds
insecure_admin_count on the plaintext path, in nextProtocol(), reached exactly
when detection fails to recognise TLS bytes, including the case where the client
sends nothing. The SSL factory stays out of the connector's factory list on
purpose: the detector holds it, and the connector's list is where the plaintext
fallback looks for the next protocol.

CommandListener moves to Handler.Abstract and completes its callback exactly
once on every path. The TRACE constraint moves to
SecurityHandler.PathMethodMapped with Constraint.FORBIDDEN. The admin context
path becomes "/", since Jetty 12 rejects "/*"; servlet mapping and getPathInfo()
are unaffected. Both servers disable the SNI host check, which defaults to true
from Jetty 10 on and would otherwise reject endpoints reached by IP address or
localhost.

Breaking public API

handleAuthentication(HttpServletRequest, byte[]) on AuthenticationProvider is a
default method and its parameter type changes. A third-party provider compiled
against javax keeps compiling, quietly stops overriding it, and admin auth
answers 401. Fail-closed but silent, so it belongs in the release notes.

X509_CERTIFICATE_ATTRIBUTE_NAME changes value, and as a compile-time constant it
is inlined into consumers, which need a rebuild rather than a restart.

ReadAheadEndpoint is removed. UnifiedConnectionFactory changes superclass, drops
its (String) constructor, and narrows the other to (SslContextFactory.Server,
String).

Testing

JettyAdminServerTest passes 9/9, including traceAdminServer. That class enables
port unification for every test and queryAdminServer hits http:// and https:// on
the same port, so the detector path is exercised directly. Prometheus suites
pass 37/37. Checkstyle 0, spotbugs 0, RAT clean. The tarball builds, and the
LICENSE set comes from the jars it ships rather than from guesswork.

Two gaps: nothing asserts insecure_admin_count, before or after this change, and
CommandListener has no executable coverage because JUnit 4 classes are never
collected here: junit-vintage-engine sits in dependencyManagement and is
declared nowhere, so 11 test classes have not run since the JUnit 5 migration.
Separate issue, untouched here.

[anmolnar]

Nice, lgtm.
Thanks for the contribution!

[IvanKhanas]

Thanks for the review. Heads up that I rebased onto master, so your approval now
sits on the previous head.

Only one thing actually changed. ZOOKEEPER-5076 switched Jenkinsfile-owasp to
jdk_17_latest a day before I did, so my hunk there collapsed to nothing during
the rebase, and I dropped the matching sentence from the fourth commit message.
The Java sources are untouched.

[PDavid]

Thanks for this. 👍

I really like that you grouped the work in separate commits, added detailed PR description and payed attention to a lot of detail, like license files, documentation and even OWASP suppression.

Also thanks for calling out the public API breaking change on AuthenticationProvider. We will have to document that.

[anmolnar]

@IvanKhanas

handleAuthentication(HttpServletRequest, byte[]) on AuthenticationProvider is a
default method and its parameter type changes. A third-party provider compiled
against javax keeps compiling, quietly stops overriding it, and admin auth
answers 401. Fail-closed but silent, so it belongs in the release notes.

I think this should give a compile time error, rather than fail silently if Override annotation is being used. Compiler will fail to override any method and throws an error.

[IvanKhanas]

Rebased onto master.

ZOOKEEPER-5077 bumped Netty on the line right above jetty.version, so the branch went into conflict. I kept the Jetty block and took your new Netty version. The Java sources are untouched, so nothing either of you approved has changed.

[IvanKhanas]

@anmolnar

I think this should give a compile time error, rather than fail silently if Override annotation is being used. Compiler will fail to override any method and throws an error.

You are right when the provider gets recompiled with the annotation. I was less sure about the other two paths, so I built a minimal repro: an interface with a default method whose parameter type changes to an unrelated type, and an implementation compiled against the old version. JDK 25.

Provider jar	Compiler	Runtime
recompiled, @Override present	error: method does not override or implement a method from a supertype	never gets there
recompiled, @Override absent	compiles clean, no warning	falls through to the default method
not recompiled	not involved	RESULT: DEFAULT, exit 0

The third row is what I had in mind for the release notes, and it is the ordinary upgrade path. You drop in the new server jars and leave your own provider jar in lib/ alone. The JVM resolves handleAuthentication(jakarta.servlet.http.HttpServletRequest, byte[]) against the interface, the old class does not declare it, so the default body runs and returns an empty list, which Commands turns into 401. I also removed javax from the classpath entirely, expecting verification to complain, and it behaves the same way: the descriptor of a method nobody calls never gets resolved.

It fails closed, so nobody gets in who should not. What bothers me is that the only trace is Auth Id list is empty, which says nothing about the cause.

If you read it the same way, something like this would cover it:

Custom AuthenticationProvider implementations that override handleAuthentication(HttpServletRequest, byte[]) must be recompiled against jakarta.servlet. A provider compiled against javax.servlet keeps loading, silently stops overriding the method, and admin server authentication starts answering 401.

Want it on the JIRA too, since that is where the release notes come from?

repro

javaxpkg.Req and jakartapkg.Req are empty classes standing in for the two servlet request types.

// api/Provider.java   v1 takes javaxpkg.Req, v2 takes jakartapkg.Req
package api;
public interface Provider {
    default String handle(javaxpkg.Req r) { return "DEFAULT"; }
}

// plugin/MyProvider.java   compiled against v1
package plugin;
public class MyProvider implements api.Provider {
    @Override public String handle(javaxpkg.Req r) { return "MINE"; }
}

// app/Main.java   compiled against v2
api.Provider p = (api.Provider) Class.forName("plugin.MyProvider")
        .getDeclaredConstructor().newInstance();
System.out.println("RESULT: " + p.handle(new jakartapkg.Req()));

Compile the plugin against v1, the app against v2, then run with both on the classpath and without javaxpkg.

[anmolnar]

@IvanKhanas

Thanks for the detailed reproduction. It's very helpful.

Want it on the JIRA too, since that is where the release notes come from?

Yes, please. We'll put this in the release notes of 3.10.0. Much appreciated.

[IvanKhanas]

CI is red on mvn (full-build-java-tests, 25), one failure out of 3262:

SnapshotAndRestoreCommandTest.testSnapshotAndRestoreCommand_streaming:168
  -> validateSnapshotMetrics:398   expected: <true> but was: <false>

That assertion is assertTrue((Double) metrics.get("avg_snapshottime") > 0.0). The metric comes from ZooKeeperServer.takeSnapshot, where elapsed is a long count of milliseconds. The database in that test is nearly empty, so on a fast runner the snapshot finishes inside one millisecond, elapsed is 0, and the average never gets above zero.

It passes locally here, and the admin server tests were green in the same run: JettyAdminServerTest 9/9, CommandsTest 27/27, CommandAuthTest 14/14.

The same job also failed on master at 4ee3b27 and on branch-3.8 the same evening, then passed on the next master commit.

Could someone re-run it? I would rather not push an empty commit and drop the approvals again. I can open a separate issue for that assertion if it is worth tracking.

[IvanKhanas]

@anmolnar

Yes, please. We'll put this in the release notes of 3.10.0. Much appreciated.

Added it to ZOOKEEPER-5038.

[PDavid]

CI is red on mvn (full-build-java-tests, 25), one failure out of 3262:

SnapshotAndRestoreCommandTest.testSnapshotAndRestoreCommand_streaming:168
  -> validateSnapshotMetrics:398   expected: <true> but was: <false>

That assertion is assertTrue((Double) metrics.get("avg_snapshottime") > 0.0). The metric comes from ZooKeeperServer.takeSnapshot, where elapsed is a long count of milliseconds. The database in that test is nearly empty, so on a fast runner the snapshot finishes inside one millisecond, elapsed is 0, and the average never gets above zero.

It passes locally here, and the admin server tests were green in the same run: JettyAdminServerTest 9/9, CommandsTest 27/27, CommandAuthTest 14/14.

The same job also failed on master at 4ee3b27 and on branch-3.8 the same evening, then passed on the next master commit.

Could someone re-run it? I would rather not push an empty commit and drop the approvals again. I can open a separate issue for that assertion if it is worth tracking.

Thanks for looking into this. Yes, unfortunately this SnapshotAndRestoreCommandTest.testSnapshotAndRestoreCommand_streaming seems to be flaky. Probably it worth to file a new Jira ticket about this. I re-run the CI check now.