use SecureRandom for digest auth cnonce#2220
Conversation
| public static class Builder { | ||
|
|
||
| // cnonce must be unpredictable (RFC 7616 section 3.3), like the NTLM and SCRAM nonces | ||
| private static final SecureRandom CNONCE_RANDOM = new SecureRandom(); |
There was a problem hiding this comment.
Don't share one static SecureRandom; use a ThreadLocal. newCnonce runs on Netty event-loop threads (digest header build + 401/407 interceptors), and SecureRandom#nextBytes locks internally, so a single shared instance makes all event loops queue on one lock. ThreadLocalRandom had no such lock. ScramEngine already handles this the right way:
private static final ThreadLocal<SecureRandom> CNONCE_RANDOM = ThreadLocal.withInitial(SecureRandom::new);and in newCnonce:
CNONCE_RANDOM.get().nextBytes(b);| byte[] b = new byte[8]; | ||
| ThreadLocalRandom.current().nextBytes(b); | ||
| CNONCE_RANDOM.nextBytes(b); | ||
| byte[] full = md.digest(b); |
There was a problem hiding this comment.
the md.digest(b) step is now pointless. The 8 bytes are already cryptographically random, so hashing them and trimming back to 8 bytes adds nothing. The method can just be:
private void newCnonce() {
byte[] b = new byte[8];
CNONCE_RANDOM.get().nextBytes(b);
cnonce = toHexString(b);
}Same 16-hex-char cnonce. This is safe because pooledMessageDigest already resets the digest before build() gets it, so nothing downstream depends on the digest(b) call.
The digest cnonce was drawn from a non-cryptographic PRNG:
Switched the cnonce source to SecureRandom to match.