Repository navigation
fix(auth): Digest authentication replay protection (stateless signed nonces) - #504
Conversation
60d7b1e to
3df0a90
Compare
6480fb9 to
f054c44
Compare
willmmiles
left a comment
willmmiles
left a comment
There was a problem hiding this comment.
This PR comes at a substantial RAM cost for all clients, whether or not they use the digest feature. I think we should avoid penalizing the normal case (no authentication) to support a less-common feature.
The per-instance digest secret, nonce TTL and opaque String were always allocated, costing RAM for every user even without Digest auth. Move them into a DigestState struct behind a single pointer, initialized on first use from _digestState() (no lock/atomic: first use happens from setup() or the async_tcp task). - setDigestSecret() derives the opaque before applying the secret: on allocation failure the previous secret/opaque pair stays intact, and the first-use path avoids the double secret/opaque initialization. - On init failure the state is discarded so the next use retries. - digestOpaque() returns an empty String if Digest auth was never used. Closes review: #504 (discussion r4483301272) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
510b3a1 to
115e054
Compare
115e054 to
d32bab1
Compare
d32bab1 to
9afb820
Compare
8bcfb60 to
f1daf86
Compare
|
@willmmiles : I have updated the PR as discussed in Discord and rebased it. I have also simplified a little the code and also limited the exposed public API to facilitate further rework. |
16798ed to
9e78e4a
Compare
willmmiles
left a comment
willmmiles
left a comment
There was a problem hiding this comment.
Some nitpicks - nothing blocking.
| // Extracts one parameter value from a Digest header (e.g. getDigestParam(header, "nonce", out)). | ||
| // Quote-aware: values in quoted form are returned without their quotes. | ||
| // Returns false when the parameter is absent; an empty value yields an empty out string. | ||
| bool getDigestParam(const char *header, const char *name, String &out); |
There was a problem hiding this comment.
Wish we could return std::optional<std::string_view>. I still advise avoiding output parameters - it's particularly bad in this case since we always end up zeroing the String twice (once in the default constructor, once immediately after the function starts).
There was a problem hiding this comment.
Wish we could return
std::optional<std::string_view>. I still advise avoiding output parameters - it's particularly bad in this case since we always end up zeroing theStringtwice (once in the default constructor, once immediately after the function starts).
there are several states outputted. One way would be to use optional also but I don't know if we can use -std=gnu++17 for all platforms.
I will leave it as-is for now.
There was a problem hiding this comment.
We cannot - we're stuck with C++11 as floor support for the foreseeble future, I think. Hence the wish. ;) I'm very tempted to consider including polyfills for useful newer C++ features in that bad_alloc-safe containers library.
…nonces) Digest challenges now include server-side anti-replay protections, with no server-side per-nonce storage (server stays stateless): - the challenge nonce embeds its issuing time and is signed with a per-instance secret (HMAC-MD5): forged, foreign or expired nonces are rejected; expiry issues a challenge with stale=true (RFC 7616 sec. 3.3) so compliant clients re-authenticate silently; - the opaque is derived from the instance secret and identifies the server; - the header uri field is bound to the effective request URI (RFC 7230 sec. 5.5 raw request-target, path + query), taken before decoding and rewrites: a captured header cannot be replayed on another endpoint nor with a different query string. AsyncWebServerRequest::authenticateDigest() keeps its signature: NULL nonce/opaque/uri arguments mean automatic (library) validation, values take precedence. authenticate() defaults to the full stateless check. New request API: rawUri() (effective request URI), authStale(). New server API: setDigestNonceTimeout(), setDigestSecret(). New build flag: ASYNCWEBSERVER_DIGEST_NONCE_TTL (default 60s). Consolidated hex digit tables in literals.h (T_hex_chars, lowercase). Example: examples/arduino/Auth gains endpoints for digest-with-hash and test_digest.sh which verifies replay protection live. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
0cffa89 to
3b30b31
Compare
Digest challenges now include server-side anti-replay protections, with no server-side per-nonce storage (server stays stateless):
AsyncWebServerRequest::authenticateDigest() keeps its signature: NULL nonce/opaque/uri arguments mean automatic (library) validation, values take precedence. authenticate() defaults to the full stateless check.
New request API: rawUri() (effective request URI), authStale().
New server API: setDigestNonceTimeout(), setDigestSecret().
New build flag: ASYNCWEBSERVER_DIGEST_NONCE_TTL (default 60s).
Consolidated hex digit tables in literals.h (T_hex_chars, lowercase).
Example: examples/arduino/Auth gains endpoints for digest-with-hash and test_digest.sh which verifies replay protection live.
test_digest.shcan be used to verify the fix and launch tests