Skip to content

use SecureRandom for digest auth cnonce#2220

Open
jmestwa-coder wants to merge 1 commit into
AsyncHttpClient:mainfrom
jmestwa-coder:digest-cnonce-securerandom
Open

use SecureRandom for digest auth cnonce#2220
jmestwa-coder wants to merge 1 commit into
AsyncHttpClient:mainfrom
jmestwa-coder:digest-cnonce-securerandom

Conversation

@jmestwa-coder

Copy link
Copy Markdown
Contributor

The digest cnonce was drawn from a non-cryptographic PRNG:

  • Realm.Builder.newCnonce seeds the cnonce from ThreadLocalRandom, whose output is predictable
  • RFC 7616 section 3.3 needs the cnonce unpredictable so it guards against chosen-plaintext and precomputation attacks on the credentials
  • NtlmEngine and ScramEngine in this repo already use SecureRandom for their nonces

Switched the cnonce source to SecureRandom to match.

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();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. switched to ThreadLocal.withInitial(SecureRandom::new) to match ScramEngine so the event loops don't serialize on one lock.

byte[] b = new byte[8];
ThreadLocalRandom.current().nextBytes(b);
CNONCE_RANDOM.nextBytes(b);
byte[] full = md.digest(b);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense. dropped the digest step, newCnonce now hex-encodes the 8 random bytes directly, and removed the now-unused Arrays import. same 16-hex cnonce and the digest handed to newResponse is still the reset pooled instance.

@jmestwa-coder jmestwa-coder force-pushed the digest-cnonce-securerandom branch from cd2f098 to 693d54a Compare July 3, 2026 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants