Thanks Bertrand
After analyzing and debugging Form Authentication Handler, I have still one
doubt. Inspecting the value of cookie sling.formauth, the value changes
constantly with every request. I do not think that was the intended behavior.
There appears to be problem with the method getCookieAuthData [1] which always
returns null. I would consider that a bug.
Initially I was confused because the TokenStore does not store the cookie
tokens. It only has a buffer of 5 security keys, which it cycles through as the
cookie values are generated against those. The cookie values themselves are
what I consider tokens, these not stored.
I think your tests are very helpful. However, do you think tests could
encompass synchronization and thread safety? Specifically concerning the bug
sonar cloud reported about volatile array. For example with this test coverage
is it enough coverage to make the following change? (See below for reasoning
[3])
from
private volatile SecretKey[] currentTokens;
to
private SecretKey[] currentTokens;
Also getActiveToken is synchronized; so when simultaneous requests come thru
with cookies generated from different keys, it would be interesting to know
whether the method is blocking and the proper key is used. Do you think it’s
useful to test with multiple threads, or not necessarily?
private synchronized int getActiveToken()
Regards
Cris
[1]
https://github.com/apache/sling-org-apache-sling-auth-form/blob/e7cfa7827c9ce39d5f686556bb2555c83c335c3f/src/main/java/org/apache/sling/auth/form/impl/FormAuthenticationHandler.java#L514
[2]
https://github.com/apache/sling-org-apache-sling-auth-form/blob/e7cfa7827c9ce39d5f686556bb2555c83c335c3f/src/main/java/org/apache/sling/auth/form/impl/TokenStore.java#L69
[3]
Marking an array volatile means that the array itself will always be read fresh
and never thread cached, but the items in the array will not be. Similarly,
marking a mutable object field volatile means the object reference is volatile
but the object itself is not, and other threads may not see updates to the
object state.
This can be salvaged with arrays by using the relevant AtomicArray class, such
as AtomicIntegerArray, instead. For mutable objects, the volatile should be
removed, and some other method should be used to ensure thread-safety, such as
synchronization, or ThreadLocal storage.
Noncompliant Code Example
private volatile int [] vInts; // Noncompliant
private volatile MyObj myObj; // Noncompliant
Compliant Solution
private AtomicIntegerArray vInts;
private MyObj myObj;
See
CERT, CON50-J. - Do not assume that declaring a reference volatile guarantees
safe publication of the members of the referenced object
> On Mar 26, 2021, at 11:28 AM, Bertrand Delacretaz <[email protected]>
> wrote:
>
> curious
