dialog: type digest auth failures apart from the response - #337
Open
Mliviu79 wants to merge 2 commits into
Open
Conversation
WaitAnswer guarded the digest retry on the absence of an Authorization header, so after the first attempt a second 401 was never treated as a challenge. That guard was also doing loop-termination duty, which is why it could not answer a re-challenge. A stale=true re-challenge (RFC 2617 3.2.1, adopted for SIP by RFC 3261 22.4) asks the UAC to retry with the new nonce and the same credentials, so a registrar rotating nonces failed the INVITE on a correct password. Attempts are counted instead, capped at two and shared by the 401 and 407 arms so a 407 followed by a 401 consumes both. Replacing credentials is safe because digestAuthApply and digestProxyAuthApply remove the existing authorization header before appending. A 401/407 is now treated as a challenge only when it carries the matching authenticate header, so a plain rejection reaches the caller as its own status instead of a digest parse error. Exhausting the cap returns the final response as the ErrDialogResponse it already returned, so no new API. Refs emiago#329
A challenge that could not be answered was folded into the password and attempt checks on the challenge condition itself, so it fell through to the final response and reached the caller as a plain 401/407 ErrDialogResponse. That is indistinguishable from a peer refusing the route, though the two want opposite handling: a blank password is our own misconfiguration, and a peer still challenging past the attempt cap will not accept the credentials at all. Return ErrAuthMissingCreds and ErrAuthMaxRetry on those paths instead, keeping the authenticate header as the only test for whether a response is a challenge so a plain rejection still arrives with its real status. Pre-existing gofmt findings in dialog_client.go and sip/request.go, and the racy TestTransportLayerClientConnectionReuse and ...NoReuse in sip, are unrelated to this change and unchanged by it. Refs emiago#336
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #336
Stacked on #330, which introduces the retry cap. Please take that first — the diff here is only the sentinels on top.
When a digest challenge cannot be answered,
WaitAnswerfalls through to the final response and returnsErrDialogResponsecarrying the 401 or 407. That is byte-identical to what a caller gets when the peer simply rejects the route, so a blank local password and an exhausted retry cap are indistinguishable from a remote refusal.Two sentinels separate them:
ErrAuthMissingCreds— a real challenge arrived and no password is configuredErrAuthMaxRetry— the peer kept challenging past the capA genuine peer rejection still returns
ErrDialogResponse, unchanged.Four subtests added alongside the existing ones, covering both sentinels on the 401 and 407 arms.