Do not use InheritedThreadLocal for SecurityRequestContext
Description
Using InheritedThreadLocal with any kind of executor pool is dangerous because executor pools spin up threads on-demand when a job is submitted; they do not necessarily warm up the pool. Since those executors may be reused, this can cause the SecurityRequestContext (i.e. user ID and credentials) to be transferred to a different request.
This specifically happens in the /run endpoint of system workers; the call eventually gets sent to a downstream ScheduledExecutorService which will inherit the first tasks’s SecurityRequestContext, but following tasks will continue to use the previous SecurityRequestContext since it is not reset.
This is also demonstrated in the below code; specifically, the second task is forced to spawn a new thread because the first task is in an infinite while loop. However, the third task will print test 2 instead of the expected test 3 because it inherits the threadlocal set before the second task:
Release Notes
Fixed an error in security-enabled instances which caused pipeline launch to fail and return `token expired` to the user when evaluating secure macros in provisioner properties.
Using InheritedThreadLocal with any kind of executor pool is dangerous because executor pools spin up threads on-demand when a job is submitted; they do not necessarily warm up the pool. Since those executors may be reused, this can cause the SecurityRequestContext (i.e. user ID and credentials) to be transferred to a different request.
This specifically happens in the /run endpoint of system workers; the call eventually gets sent to a downstream ScheduledExecutorService which will inherit the first tasks’s SecurityRequestContext, but following tasks will continue to use the previous SecurityRequestContext since it is not reset.
This is also demonstrated in the below code; specifically, the second task is forced to spawn a new thread because the first task is in an infinite while loop. However, the third task will print
test 2instead of the expectedtest 3because it inherits the threadlocal set before the second task: