makiolaj/agendula
1
0
Fork 0
Code Issues Pull Requests Actions Releases 8 Activity

sync: three defects in the vendored Digest handler

Browse Source 
The Basic arm returned out of the challenge loop as soon as it found a
Basic challenge it had already tried. The handler is a network
interceptor, so Basic is always primed preemptively over HTTPS — the
abort therefore fired on the first 401, and a Digest challenge later in
the same header was never read. A Baikal or Apache front end offering
both and rejecting Basic at the app layer got no Digest answer at all,
and the account was marked as needing sign-in for good. Every challenge
is read before anything is decided now; giving up still happens, just
after the whole header has been looked at.

clientNonce and nonceCount sat on the companion object while SyncEngine
builds one handler per account, so two accounts syncing at once
interleaved their nc values. They are instance state now, and a new
server nonce restarts the count — RFC 7616 3.4.1 counts requests sent
with that nonce, starting at 1, and Apache's AuthDigestNcCheck answers
401 for a carried-over count.

qop values were split on "," and compared untrimmed, so
qop="auth, auth-int" quietly downgraded to auth and qop=" auth" matched
nothing — falling into the RFC 2069 branch, which emits no qop, nc or
cnonce and which an RFC 7616 server rejects outright.

Documented as changes 10-12 in PROVENANCE. The four static assignments
in upstream's digest tests now address the handler; nothing else in that
file changes.
This commit is contained in:
makiolaj
2026-09-09 11:22:34 +02:00
parent 210e394163
commit 80133d5cc8
4 changed files with 212 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files
dav/PROVENANCE.md
+43
View File
@@ -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.
dav/src/main/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandler.kt
+54 -18
View File
@@ -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
}
dav/src/test/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandlerTest.kt
+8 -8
View File
@@ -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<String?, String>(
@@ -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<String?, String>(
@@ -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<String?, String>(
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<String?, String>(
Pair("realm", "AuthInt Example"),
dav/src/test/kotlin/at/bitfire/dav4jvm/LocalChangesTest.kt
+107
View File
@@ -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<String?, String>("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<String?, String>("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<String?, String>(
"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<String?, String>("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="))
}
}