SOLR-18270 CertAuthPlugin : update the lookup attribute (without parametrization) by heitzjm · Pull Request #4474 · apache/solr

heitzjm · 2026-05-29T12:50:27Z

https://issues.apache.org/jira/browse/SOLR-18270

Description

This PR only update the request attribute used by CertAuthPlugin to match the one filled by Jetty 12 - for SOLR 10. SOLR 9 works fine in the Official Docker Image, as it uses a lower major Jetty version.

Solution

This PR only updates the request attribute used by CertAuthPlugin to match the one filled by Jetty 12 - for SOLR 10, and provides, after revision, as advised by @laminelam, a fallback on the old attribute.

Tests

I checked locally by compiling SOLR from source, as well as building Docker Image from source and tested it in a K8S environment.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
I have developed this patch against the main branch.
I have run ./gradlew check.
5582 tests completed, 1 failed, 184 skipped
ERROR: The following test(s) have failed:
- org.apache.solr.handler.component.AsyncTrackerSemaphoreLeakTest.testSemaphoreLeakOnLBRetry (:solr:core)
  => however, in my opinion, this test does not seem to be relevant for this PR
I have added tests for my changes.
I have added documentation for the Reference Guide
I have added a changelog entry for my change
unreleased/SOLR-18270-easy-fix-without-parametrization.yml

laminelam · 2026-06-01T16:07:36Z

I'd rather have jakarta as default and javax as fallback for code back-portability and compatibility.

…he mailing list

heitzjm · 2026-06-01T20:31:24Z

As asked, I've just updated this PR with the fallback.
I ran the tests again :

:solr:core:test (FAILURE): 5583 test(s), 1 failure(s), 2611 skipped

5583 tests completed, 1 failed, 2611 skipped

> Task :solr:core:test FAILED
ERROR: The following test(s) have failed:
  - org.apache.solr.cloud.LeaderTragicEventTest (:solr:core)
    Test history: https://develocity.apache.org/scans/tests?search.rootProjectNames=solr-root&tests.container=org.apache.solr.cloud.LeaderTragicEventTest
    Test output: /tmp/SOLR/solr/core/build/test-results/test/outputs/OUTPUT-org.apache.solr.cloud.LeaderTragicEventTest.txt
    Reproduce with: ./gradlew :solr:core:test --tests "org.apache.solr.cloud.LeaderTragicEventTest" "-Ptests.jvmargs=-XX:TieredStopAtLevel=1 -XX:+UseParallelGC -XX:ActiveProcessorCount=1 -XX:ReservedCodeCacheSize=120m" -Ptests.seed=8D6096E2411286D8 -Ptests.useSecurityManager=true -Ptests.file.enc

I ran the failed test again, and it was successful.

psalagnac · 2026-07-15T17:05:42Z

-        (X509Certificate[]) request.getAttribute("javax.servlet.request.X509Certificate");
+        (X509Certificate[]) request.getAttribute(CURRENT_REQUEST_ATTRIBUTE_NAME);
+    if (certs == null || certs.length == 0) {
+      certs=(X509Certificate[]) request.getAttribute(OLD_REQUEST_ATTRIBUTE_NAME);


Do we still need to check the old name?
My understanding is this name changed with new Jetty. I don't think we will ever go back to the other name. We are good with checking only the new name in 10+.

I think both should stay - in my understanding, SOLR, at its basis, is a web application that could run in other java web application servers than the Jetty provided in the Docker image - so I think it would be better to keep both names to ease integration. This way, this PR might even be merged to SOLR 9 as well.

Makes sense. Lets keep it this way.

You may want to rename the variables to something meaningful, how about:

JAVAX_REQUEST_ATTRIBUTE_NAME
JAKARTA_REQUEST_ATTRIBUTE_NAME ?

Also, you will need to run spotless check, you're missing some white spaces

@heitzjm these days, Solr only runs in Jetty, and we've been getting more specific to just that. Notice we no longer produce a .war file. I think the only place where you might have solr run in another container is via the EmbeddedSolr approach.. Having said that, I don't think we have made a "This is how it is" explicit decision, and I'm fine with the backcompat for now.

Maybe I'm wrong, but I don't know of anyone that's taking Solr and running it in a different app server or even a different Jetty version than what Solr ships with. Our code is really tightly coupled with Jetty to the point where I doubt that sort of thing is even possible.

Ultimately it's a minor concern that's not worth holding up the PR, but the odds are so small of someone doing this successfully that idk that the fallback logic is worth it 🤷

psalagnac · 2026-07-15T17:43:41Z

If nothing else comes to the PR, I'll merge it early next week.

laminelam · 2026-07-15T18:31:12Z

-        (X509Certificate[]) request.getAttribute("javax.servlet.request.X509Certificate");
+        (X509Certificate[]) request.getAttribute(CURRENT_REQUEST_ATTRIBUTE_NAME);
+    if (certs == null || certs.length == 0) {
+      certs=(X509Certificate[]) request.getAttribute(OLD_REQUEST_ATTRIBUTE_NAME);


You may want to rename the variables to something meaningful, how about:

JAVAX_REQUEST_ATTRIBUTE_NAME
JAKARTA_REQUEST_ATTRIBUTE_NAME ?

Also, you will need to run spotless check, you're missing some white spaces

…itespace formatting

heitzjm · 2026-07-15T20:52:33Z

Thanks @laminelam and @psalagnac . I have just updated the code. I hope it is fine now.

psalagnac · 2026-07-16T08:56:38Z

@heitzjm Formatting is still broken. Please make sure to run ./gradlew precommit before pushing (spotless formatting can be applied with ./gradlew tidy).

…essful

heitzjm · 2026-07-16T09:56:40Z

Thanks @psalagnac for the gradlew commands, I have just run them.

epugh · 2026-07-16T12:29:10Z

I just triggered the builds...

gerlowskija

Left a few comments/questions inline, but this looks close enough that I hope we can merge it soon. I'll circle back next week for a re-review if I see replies here.

One more general comment:

I'm surprised that this PR can change the attribute-name used without needing any corresponding changes in CertAuthPluginTest. Should there be some changes there do you think?

gerlowskija · 2026-07-17T20:35:54Z

  private static final String PARAM_PRINCIPAL_RESOLVER = "principalResolver";
  private static final String PARAM_CLASS = "class";
  private static final String PARAM_PARAMS = "params";
+  private static final String JAVAX_REQUEST_ATTRIBUTE_NAME =


[0] We probably don't want to support reading both properties forever.

Could you put a TODO comment here nudging someone in the future to remove support for the 'javax' variant on/after Solr 11

Sure, I do it. And this time, I am using gradlew with tidy and precommit !

heitzjm · 2026-07-17T21:22:34Z

      throws Exception {
    X509Certificate[] certs =
-        (X509Certificate[]) request.getAttribute("javax.servlet.request.X509Certificate");
+        (X509Certificate[]) request.getAttribute(JAKARTA_REQUEST_ATTRIBUTE_NAME);


I implemented the fallback logic because at the time of writing, I was still thinking of Solr as of a web application running in a web application server (that is, that could be distributed with a web application archive (war) file). However, @epugh #4474 (comment) made me notice Solr is no more distributed that way.
At least having the fallback could allow to merge the commit in both 9.x and 10.x.

I had a chat with @dsmiley today, and he confirmed that Solr is NOT ever going to run in anything but Jetty, so we don't need to support that. Of course, that doesn't apply to the embedded mode ;-).

gerlowskija · 2026-07-17T20:44:21Z

@@ -0,0 +1,8 @@
+title: Update looked up attribute for SSL authentication


As I understand your JIRA description, CertAuthPlugin is broken entirely in Solr 10, since it's looking at the wrong attribute name?

If that's correct, then this is a pretty serious bug you're fixing here! We should tweak the wording to highlight it more strongly. Maybe something like:

Suggested change

title: Update looked up attribute for SSL authentication

title: CertAuthPlugin now uses correct Jetty attribute for certificate lookup

Yes, CertAuthPlugin is broken in Solr 10 because of the attribute name mismatch - and it was hard for me to find out why, while trying to migrate from Solr 9 to 10. However, I think the wording also depends on the target (9 and/or 10).

heitzjm · 2026-07-17T21:22:34Z

      throws Exception {
    X509Certificate[] certs =
-        (X509Certificate[]) request.getAttribute("javax.servlet.request.X509Certificate");
+        (X509Certificate[]) request.getAttribute(JAKARTA_REQUEST_ATTRIBUTE_NAME);


I implemented the fallback logic because at the time of writing, I was still thinking of Solr as of a web application running in a web application server (that is, that could be distributed with a web application archive (war) file). However, @epugh #4474 (comment) made me notice Solr is no more distributed that way.
At least having the fallback could allow to merge the commit in both 9.x and 10.x.

gerlowskija · 2026-07-17T21:34:10Z

At least having the fallback could allow to merge the commit in both 9.x and 10.x.

OK, I guess. Though CertAuthPlugin already works on 9.x, doesn't it. So I'm not sure why the PR would go back to that branch at all?

But as I said above it's so minor a thing that it's not worth holding up the PR over 🤷

gerlowskija · 2026-07-17T21:34:59Z

Btw - for some reason when I initially reviewed the PR GH didn't load any of the previous comment/review history. Only saw folks' prior comments on a reload. Sorry for the noise and repetition!

heitzjm · 2026-07-17T21:39:12Z

Btw - for some reason why I initially reviewed the PR GH didn't load any of the previous comment/review history. Only saw folks' prior comments on a reload. Sorry for the noise and repetition!

No problem. I am grateful for the support everyone gives me on this PR !

heitzjm · 2026-07-17T22:04:31Z

Left a few comments/questions inline, but this looks close enough that I hope we can merge it soon. I'll circle back next week for a re-review if I see replies here.

One more general comment:

I'm surprised that this PR can change the attribute-name used without needing any corresponding changes in CertAuthPluginTest. Should there be some changes there do you think?

CertAuthPluginTest.java uses a mock that does not take care of the looked up attribute, and provides unit tests. As the problem is an integration concern, I think it is fair that the unit tests did not detect the problem. However I am not convinced that writing an integration test would be worth the time...

heitzjm added 2 commits May 29, 2026 01:00

CertAuthPlugin : update the lookup attribute

4c748a8

SOLR-18270 Add changelog

93a2330

github-actions Bot added the cat:security label May 29, 2026

SOLR-18270 : fix typo in changelog

a04a242

laminelam mentioned this pull request Jun 1, 2026

SOLR-18270: cert auth plugin : add request attribute parametrization #4479

Closed

8 tasks

SOLR-18270 : implementing fallback attribute lookup as suggested on t…

47ca1cf

…he mailing list

psalagnac reviewed Jul 15, 2026

View reviewed changes

SOLR-18270 : change JIRA issue URL

9df535c

psalagnac approved these changes Jul 15, 2026

View reviewed changes

psalagnac added this to the 10.x milestone Jul 15, 2026

laminelam suggested changes Jul 15, 2026

View reviewed changes

SOLR-18270 : changing constants to more meaningful names ; improve wh…

835a224

…itespace formatting

SOLR-18270 : reformatting using gradlew tidy ; gradlew precommit succ…

366f94c

…essful

gerlowskija reviewed Jul 17, 2026

View reviewed changes

heitzjm commented Jul 17, 2026

View reviewed changes

SOLR-18270 : add TODO for future back-compability removal

748a8f4

		@@ -0,0 +1,8 @@
		title: Update looked up attribute for SSL authentication

	title: Update looked up attribute for SSL authentication
	title: CertAuthPlugin now uses correct Jetty attribute for certificate lookup

Uh oh!

Conversation

heitzjm commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Solution

Tests

Checklist

Uh oh!

laminelam commented Jun 1, 2026

Uh oh!

heitzjm commented Jun 1, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psalagnac commented Jul 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heitzjm commented Jul 15, 2026

Uh oh!

psalagnac commented Jul 16, 2026

Uh oh!

heitzjm commented Jul 16, 2026

Uh oh!

epugh commented Jul 16, 2026

Uh oh!

gerlowskija left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gerlowskija commented Jul 17, 2026

Uh oh!

gerlowskija commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

heitzjm commented Jul 17, 2026

Uh oh!

heitzjm commented Jul 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

heitzjm commented May 29, 2026 •

edited

Loading

gerlowskija commented Jul 17, 2026 •

edited

Loading