ZOOKEEPER-5038: Migrate to Jetty 12.1.12 (EE10) - #2435
Open
IvanKhanas wants to merge 4 commits into
Open
Conversation
The Jetty BOMs replace the per-artifact versions and jetty-servlet becomes jetty-ee10-servlet. The BOMs don't manage jakarta.servlet-api, so it is pinned to 6.0.0 separately. The sources still import javax.servlet, so this commit does not compile on its own.
Import blocks shift because checkstyle sorts jakarta before java. The value of X509AuthenticationProvider.X509_CERTIFICATE_ATTRIBUTE_NAME changes with the namespace. zookeeper-contrib-rest keeps javax.servlet, since it runs on Grizzly rather than Jetty.
UnifiedConnectionFactory extends DetectorConnectionFactory, which does the byte sniffing ReadAheadEndpoint used to do, so that class is deleted. The SSL factory stays out of the connector's factory list, because that list is where the plaintext fallback looks for the next protocol. CommandListener moves to Handler.Abstract and the TRACE constraint to SecurityHandler.PathMethodMapped. The admin context path becomes "/", since Jetty 12 rejects "/*". Both servers turn off the SNI host check, which defaults to true from Jetty 10 on and would reject requests to a bare IP address.
…y 12 The LICENSE files now match the jars the binary tarball ships. jetty-ee10-servlet and jetty-session replace jetty-servlet, and jakarta.servlet-api needs its own EPL-2.0 and GPL-2.0-with-CPE text rather than Jetty's. jetty-client and jetty-util-ajax lose their files, since neither one ever shipped in lib/. The CVE-2024-6763 suppression rested on Jetty 9.x being EOL and never getting the fix marked as such. CVE-2021-28164 and CVE-2021-34429 are specific to Jetty 9.4.x. All three are gone. Jenkinsfile-owasp moves to JDK 17, which Jetty 12 needs to resolve at all.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
which therefore needs its own pin. Does not compile on its own.
lines; import blocks move because checkstyle sorts jakarta before java.
UnifiedConnectionFactory, CommandListener, PrometheusMetricsProvider, and
ReadAheadEndpoint, which is deleted.
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.