diff --git a/dav/PROVENANCE.md b/dav/PROVENANCE.md index 10419c4..2d877be 100644 --- a/dav/PROVENANCE.md +++ b/dav/PROVENANCE.md @@ -247,3 +247,46 @@ cleartext cases it used to cover are pinned explicitly by `cleartextBasicIsSentWhenExplicitlyAllowed` and `cleartextDigestIsStillAnswered`. This is the one upstream test this port deliberately changes rather than inherits. + +## Change 10 — a Basic challenge no longer hides the Digest one beside it + +The 401 branch scanned `response.challenges()` in a loop and did a non-local +`return null` the moment it saw a `Basic` challenge it had already tried. Because +the handler is installed as a **network interceptor**, `basicAuth` is always +primed preemptively over HTTPS — so that fired on the *first* 401, and a +`Digest` challenge later in the same header was never read at all. + +A Baïkal or Apache front end that advertises both and rejects Basic at the +application layer therefore never got a Digest answer, and `SyncEngine` marked +the account as needing sign-in permanently. + +The loop now reads every challenge before anything is decided; a scheme already +known not to work is simply not re-offered. Giving up is still the outcome when +nothing usable is left — it happens after the whole header has been looked at +rather than in the middle of it. + +## Change 11 — the digest counter is per handler, and per nonce + +`clientNonce` and `nonceCount` lived on the companion object while `SyncEngine` +builds **one handler per account**, so two accounts syncing at once interleaved +their `nc` values against each other's nonces. They are instance state now. + +The count was also never reset. RFC 7616 §3.4.1 defines `nc` as the count of +requests sent *with that nonce*, starting at 1, so a server that enforces it +(Apache `AuthDigestNcCheck On`, several NAS stacks) answers 401 for a rotated +nonce that arrives with a carried-over count. A new server nonce now restarts +it. The client nonce stays put: it is ours, one per handler, and pairing it with +a restarted count is what the RFC describes. + +The tell upstream left behind is that every digest test has to reset the counter +by hand — those four assignments now address the handler rather than the class, +which is the only change to that file. + +## Change 12 — `qop` list values are trimmed + +`paramValue.split(",")` with no `trim()`. HTTP list syntax allows space around +the separator, so `qop="auth, auth-int"` silently downgraded to `auth`, and a +single spaced value (`qop=" auth"`) matched nothing at all — dropping into the +RFC 2069 legacy branch, which emits a response with no `qop`, `nc` or `cnonce`. +An RFC 7616 server rejects that outright: a permanent 401 against a server +behaving perfectly legally. Values are trimmed and compared case-insensitively. diff --git a/dav/src/main/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandler.kt b/dav/src/main/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandler.kt index 44e7d4d..be694f0 100644 --- a/dav/src/main/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandler.kt +++ b/dav/src/main/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandler.kt @@ -46,10 +46,6 @@ class BasicDigestAuthHandler( companion object { private const val HEADER_AUTHORIZATION = "Authorization" - // cached digest parameters - var clientNonce = h(UUID.randomUUID().toString()) - var nonceCount = AtomicInteger(1) - fun quotedString(s: String) = "\"" + s.replace("\"", "\\\"") + "\"" fun h(data: String) = data.toByteArray().toByteString().md5().hex() @@ -66,6 +62,22 @@ class BasicDigestAuthHandler( private var basicAuth: Challenge? = null private var digestAuth: Challenge? = null + /* + * ⚠️ Per handler, not per process. Upstream keeps these on the companion + * object while `SyncEngine` builds one handler per account, so two accounts + * syncing at once interleave their `nc` values against each other's nonces. + * RFC 7616 §3.4.1 defines `nc` as the count of requests sent *with that + * nonce*, starting at 1 — a server that enforces it (Apache + * `AuthDigestNcCheck On`, several NAS stacks) answers 401 and the account is + * marked as needing sign-in. The tell upstream left behind is that every + * digest test has to reset the counter by hand. + */ + var clientNonce = h(UUID.randomUUID().toString()) + val nonceCount = AtomicInteger(1) + + /** The server nonce [nonceCount] is counting against. */ + private var countedNonce: String? = null + fun authenticateRequest(request: Request, response: Response?): Request? { domain?.let { @@ -94,26 +106,33 @@ class BasicDigestAuthHandler( } else { // we're processing a 401 response + // ⚠️ Every challenge is read before anything is decided. Upstream + // returned out of this loop the moment Basic was found to have + // failed — and because the handler is a network interceptor, + // `basicAuth` is *always* primed preemptively, so that fired on the + // very first 401. A server advertising `Basic` then `Digest` in one + // header therefore never had its Digest challenge read at all: a + // Baikal/Apache front end that offers both and rejects Basic at the + // app layer got no Digest answer, and the account was marked as + // needing sign-in permanently. Giving up is still the outcome when + // nothing usable is left — it just happens after the whole header + // has been looked at. var newBasicAuth: Challenge? = null var newDigestAuth: Challenge? = null for (challenge in response.challenges()) when { - "Basic".equals(challenge.scheme, true) -> { - basicAuth?.let { - Dav4jvm.log.warning("Basic credentials didn't work last time -> aborting") - basicAuth = null - return null + "Basic".equals(challenge.scheme, true) -> + if (basicAuth != null) { + Dav4jvm.log.warning("Basic credentials didn't work last time -> not offering them again") + } else { + newBasicAuth = challenge } - newBasicAuth = challenge - } - "Digest".equals(challenge.scheme, true) -> { + "Digest".equals(challenge.scheme, true) -> if (digestAuth != null && !"true".equals(challenge.authParams["stale"], true)) { - Dav4jvm.log.warning("Digest credentials didn't work last time and server nonce has not expired -> aborting") - digestAuth = null - return null + Dav4jvm.log.warning("Digest credentials didn't work last time and server nonce has not expired -> not offering them again") + } else { + newDigestAuth = challenge } - newDigestAuth = challenge - } } // ⚠️ Not cached if we would refuse to answer it. Caching a challenge @@ -201,6 +220,17 @@ class BasicDigestAuthHandler( params.add("uri=${quotedString(digestURI)}") if (qop != null) { + // ⚠️ A new server nonce restarts the count at 1: §3.4.1 counts + // requests sent with *that* nonce, so carrying the count over makes + // the first request against a rotated nonce look like the + // thousandth. The client nonce stays put — it is ours, one per + // handler, and pairing it with a restarted count is what the RFC + // describes. + if (nonce != countedNonce) { + countedNonce = nonce + nonceCount.set(1) + } + params.add("qop=${qop.qop}") params.add("cnonce=${quotedString(clientNonce)}") @@ -290,8 +320,14 @@ class BasicDigestAuthHandler( paramValue?.let { var qopAuth = false var qopAuthInt = false + // ⚠️ Trimmed. HTTP list syntax allows space around the + // separator, so `qop="auth, auth-int"` silently downgraded + // to auth and a single spaced value matched nothing at all — + // which dropped into the RFC 2069 branch and emitted a + // response with no qop, nc or cnonce, for a permanent 401 + // against a server behaving legally. for (qop in paramValue.split(",")) - when (qop) { + when (qop.trim().lowercase()) { "auth" -> qopAuth = true "auth-int" -> qopAuthInt = true } diff --git a/dav/src/test/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandlerTest.kt b/dav/src/test/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandlerTest.kt index 0c7a497..6660762 100644 --- a/dav/src/test/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandlerTest.kt +++ b/dav/src/test/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandlerTest.kt @@ -116,8 +116,8 @@ class BasicDigestAuthHandlerTest { fun testDigestRFCExample() { // use cnonce from example val authenticator = BasicDigestAuthHandler(null, "Mufasa", "Circle Of Life") - BasicDigestAuthHandler.clientNonce = "0a4f113b" - BasicDigestAuthHandler.nonceCount.set(1) + authenticator.clientNonce = "0a4f113b" + authenticator.nonceCount.set(1) // construct WWW-Authenticate val authScheme = Challenge("Digest", mapOf( @@ -147,8 +147,8 @@ class BasicDigestAuthHandlerTest { @Test fun testDigestRealWorldExamples() { var authenticator = BasicDigestAuthHandler(null, "demo", "demo") - BasicDigestAuthHandler.clientNonce = "MDI0ZDgxYTNmZDk4MTA1ODM0NDNjNmJjNDllYjQ1ZTI=" - BasicDigestAuthHandler.nonceCount.set(1) + authenticator.clientNonce = "MDI0ZDgxYTNmZDk4MTA1ODM0NDNjNmJjNDllYjQ1ZTI=" + authenticator.nonceCount.set(1) // example 1 var authScheme = Challenge("Digest", mapOf( @@ -203,8 +203,8 @@ class BasicDigestAuthHandlerTest { @Test fun testDigestMD5Sess() { val authenticator = BasicDigestAuthHandler(null, "admin", "12345") - BasicDigestAuthHandler.clientNonce = "hxk1lu63b6c7vhk" - BasicDigestAuthHandler.nonceCount.set(1) + authenticator.clientNonce = "hxk1lu63b6c7vhk" + authenticator.nonceCount.set(1) val authScheme = Challenge("Digest", mapOf( Pair("realm", "MD5-sess Example"), @@ -242,8 +242,8 @@ class BasicDigestAuthHandlerTest { @Test fun testDigestMD5AuthInt() { val authenticator = BasicDigestAuthHandler(null, "admin", "12435") - BasicDigestAuthHandler.clientNonce = "hxk1lu63b6c7vhk" - BasicDigestAuthHandler.nonceCount.set(1) + authenticator.clientNonce = "hxk1lu63b6c7vhk" + authenticator.nonceCount.set(1) val authScheme = Challenge("Digest", mapOf( Pair("realm", "AuthInt Example"), diff --git a/dav/src/test/kotlin/at/bitfire/dav4jvm/LocalChangesTest.kt b/dav/src/test/kotlin/at/bitfire/dav4jvm/LocalChangesTest.kt index dbf9ec8..11af411 100644 --- a/dav/src/test/kotlin/at/bitfire/dav4jvm/LocalChangesTest.kt +++ b/dav/src/test/kotlin/at/bitfire/dav4jvm/LocalChangesTest.kt @@ -314,4 +314,111 @@ class LocalChangesTest { // No innocent reading of this one, so it stays fatal. assertThrows(at.bitfire.dav4jvm.exception.DavException::class.java) { resource.head { } } } + + // --- change 10: the Digest challenge beside a Basic one is read ---------- + + @Test + fun `a Basic challenge no longer hides the Digest one in the same header`() { + val handler = BasicDigestAuthHandler("example.com", "user", "pw") + val request = okhttp3.Request.Builder().url("https://cloud.example.com/dav/").build() + // The interceptor primes Basic preemptively over HTTPS, so by the time + // the first 401 arrives a Basic challenge is already cached — which is + // what made the abort fire on every server that offers both. + handler.authenticateRequest(request, null) + + val challenged = handler.authenticateRequest( + request, + run { + okhttp3.Response.Builder() + .request(request) + .protocol(okhttp3.Protocol.HTTP_1_1) + .code(401) + .message("Unauthorized") + .addHeader("WWW-Authenticate", "Basic realm=\"dav\"") + .addHeader( + "WWW-Authenticate", + "Digest realm=\"dav\", nonce=\"abc\", qop=\"auth\"", + ) + .build() + }, + ) + + assertTrue(challenged!!.header("Authorization")!!.startsWith("Digest")) + } + + // --- change 11: the digest counter is per handler and per nonce ---------- + + @Test + fun `the nonce count restarts when the server issues a new nonce`() { + val handler = BasicDigestAuthHandler(null, "user", "pw") + val request = okhttp3.Request.Builder().url("https://example.com/dav/").build() + fun answer(nonce: String) = handler.digestRequest( + request, + okhttp3.Challenge( + "Digest", + mapOf("realm" to "dav", "nonce" to nonce, "qop" to "auth"), + ), + )!!.header("Authorization")!! + + assertTrue(answer("one").contains("nc=00000001")) + assertTrue(answer("one").contains("nc=00000002")) + // RFC 7616 3.4.1 counts requests sent with *that* nonce. + assertTrue(answer("two").contains("nc=00000001")) + } + + @Test + fun `two accounts do not interleave their nonce counts`() { + val request = okhttp3.Request.Builder().url("https://example.com/dav/").build() + val challenge = okhttp3.Challenge( + "Digest", + mapOf("realm" to "dav", "nonce" to "n", "qop" to "auth"), + ) + val first = BasicDigestAuthHandler(null, "one", "pw") + val second = BasicDigestAuthHandler(null, "two", "pw") + + first.digestRequest(request, challenge) + val theirs = second.digestRequest(request, challenge)!!.header("Authorization")!! + + assertTrue(theirs.contains("nc=00000001")) + } + + // --- change 12: qop list values are trimmed ----------------------------- + + @Test + fun `a spaced qop list still selects auth-int`() { + val handler = BasicDigestAuthHandler(null, "user", "pw") + val request = okhttp3.Request.Builder().url("https://example.com/dav/").build() + val header = handler.digestRequest( + request, + okhttp3.Challenge( + "Digest", + mapOf( + "realm" to "dav", + "nonce" to "n", + "qop" to "auth, auth-int", + ), + ), + )!!.header("Authorization")!! + + assertTrue(header.contains("qop=auth-int")) + } + + @Test + fun `a single spaced qop value does not fall back to RFC 2069`() { + val handler = BasicDigestAuthHandler(null, "user", "pw") + val request = okhttp3.Request.Builder().url("https://example.com/dav/").build() + val header = handler.digestRequest( + request, + okhttp3.Challenge( + "Digest", + mapOf("realm" to "dav", "nonce" to "n", "qop" to " auth"), + ), + )!!.header("Authorization")!! + + // The legacy branch emits no qop, nc or cnonce, which an RFC 7616 server + // rejects outright — a permanent 401 against a server behaving legally. + assertTrue(header.contains("qop=auth")) + assertTrue(header.contains("nc=")) + assertTrue(header.contains("cnonce=")) + } }