Fix random 403 responses

[OndroMih]

Fixes #25141.

The securityContext is stateful, cannot be shared among requests. RealmAdapter stores the principal from request to it. We need to create a copy of the default context for each request instead of using the shared default context.

This is a quick fix but I can imagine a better and nicer fix. For example, keep returning defaultSecurityContext and instead let RealmAdapter to create a new security context if a user is signed in, and then store the principal to it. This would keep the same defaultSecurityContext for anonymous user.

Another option is to save the sessionPrincipal into a ThreadLocal. Although the SecurityContext is already stored in a thread local, the default context is shared in multiple thread locals. Storing sessionPrincipal in another thread local would keep it only for the current thread even if the security context is shared. However, I don't like adding another thread local.

Maybe @arjantijms , @dmatej , @avpinchuk , @pzygielo, @hs536 you have some idea to improve it? Or it's OK to go with my simple solution?

About tests, I'm not sure if adding tests makes sense. The issue is random. Sometimes doesn't happen even after 1000 requests, especially on a slower machine. So the test would not be reliable and might not fail each time even if the behavior is broken.

[dmatej]

12:25:15       [exec] Caused by: java.lang.NullPointerException
12:25:15       [exec] 	at com.sun.s1peqe.security.ssl.converter.ejb.ConverterBean.myCallerPrincipal(ConverterBean.java:38)

[hs536]

Hmm... that's odd behavior... when I read this, it feels more like a JVM bug than a GlassFish one.
it appears the default context should be immutable across requests, so regenerating it should be approached cautiously.

[OndroMih]

Hi @hs536,

Hmm... that's odd behavior... when I read this, it feels more like a JVM bug than a GlassFish one.
it appears the default context should be immutable across requests, so regenerating it should be approached cautiously.

It's not a bug in JVM. The problem happens because one request sets the sessionPrincipal and then deletes it after the request processing. The security context is the default one. Although it's stored in a thread local, it's always the same object and thus it's shared by all requests. So if multiple requests run at the same time, they access their thread local variable, which gives them the shared default security context and manipulate the sessionPrincipal in it. If one of the requests is too short and clears the context, another request may see empty sessionPrincipal when it's evaluating the permissions and returns 403.

My fix avoids this by creating a new copy of the default context for each thread. This seems to break some expectations.

Another option is to save the sessionPrincipal into another ThreadLocal inside the security context. Then each thread would manipulate its own sessionPrincipal variable, even if they shared the same security context. It feels a bit hacky, I don't like storing values in thread locals, but the current solution seems to be even more hacky, because on one hand it expects the security context is immutable (reusing the default context for many threads), on another hand, its mutable because threads modify the sessionPrincipal.

[hs536]

My apologies, I misunderstood. If the problem is that a specific thread is deleting (or modifying) the shared defaultSecurityContext's sessionPrincipal (and this modification is allowed), wouldn't removing static from defaultSecurityContext resolve the issue...?

glassfish/nucleus/security/core/src/main/java/com/sun/enterprise/security/SecurityContext.java

Line 71 in 6f90816

private static SecurityContext defaultSecurityContext = generateDefaultSecurityContext();

[OndroMih]

wouldn't removing static from defaultSecurityContext resolve the issue...?

I effectively did this with the current solution in this PR - a new security context is created in a thread local if it doesn't exist, instead of using the one in the static variable. It's not possible to just remove the static keyword, because the variable is access from static methods.

However, I think the null pointer now is related to this code:

glassfish/nucleus/security/core/src/main/java/com/sun/enterprise/security/SecurityContext.java

Line 359 in 6f90816

return this == defaultSecurityContext ? getDefaultCallerPrincipal() : initiator;

It checks whether the context is the same as the defaultSecurityContext context, which it isn't anymore, it's just a copy of it. And then it returns initiator, which is most probably null in this case.

I'll rework the solution to store sessionPrincipal into a thread-local so that it's different for each thread, while the context itself is the same for all threads, if it's the default one. That should keep the logic and still fix the issue with the race condition and random 403 responses.

[arjantijms]

Maybe the field serverGeneratedSecurityContext should be in thread-local storage too?

As mentioned in the description; having all these thread locals is kinda confusing when reading the code. The security context itself is in a thread local, but it's individual fields are too.

Probably we need to take a good look at this entire setup later again.

[OndroMih]

Yes, I would review it later. Now, it’s clear what’s happening and my fix is simple and reliable.

With my solution, we have a thread-local in another thread local, maily because we need to store a mutable value in a structure which was designed to be immutable and sharable between multiple threads. Although securityContext is in thread local, the default security context is stored in multiple thread locals and this shared by mutliple threads. It’s OK if the structure is immutable. But after we added mutable state, it resulted in race conditions. If we make each security context unique and change rhe way it’s compared to the default context, then we can remove the nested thread local for sessionPrincipal.

[avpinchuk]

The sessionPrincipal may be final