Visitar URL original
chat: failing test for a sender-key miss stored as a permanent unbox error by chrisnojima · Pull Request #29842 · keybase/client · GitHub
Skip to content

chat: failing test for a sender-key miss stored as a permanent unbox error - #29842

Draft
chrisnojima wants to merge 1 commit into
masterfrom
nojima/HOTPOT-chat-sender-key-stale-error
Draft

chrisnojima wants to merge 1 commit into
masterfrom
nojima/HOTPOT-chat-sender-key-stale-error

Conversation

@chrisnojima

Copy link
Copy Markdown
Contributor

This PR contains only a failing test. It pins a chat service bug so the fix can land on its own. Expect CI's go/chat run to go red here until that fix lands.

Summary

If the reader's cached view of the sender doesn't yet have the sender's device key, the message is stored as a permanent error. It stays that way after the reader learns the key.

UnboxMessage
  ValidSenderKey
    CheckKIDForUID -> found=false
    NewPermanentUnboxingError(NoKeyError{"sender key not found"})   # permanent
  makeErrorMessage                                                   # error MessageUnboxed
Storage.Merge                                                        # stored as-is
blockEngine fetch
  re-fetch only errors whose ParseableVersion() is true             # this one isn't
  -> "Unable to decrypt chat message: sender key not found" forever

TestChatMessageUnboxSenderKeyMissIsNotPermanent (go/chat/boxer_sender_key_test.go) unboxes a canned V1 message. Its fake UPAK finder reports the sender's key as unknown until the test "learns" it:

unbox while the key is unknown   -> expect an error that is NOT permanent (the UPAK was asked once)
learnKey()
unbox the same message again     -> expect a valid message

It needs no kbweb: there's no signup, and the real ValidSenderKey runs.

Evidence

  • Before (master):
    Error:    Should be false
    Messages: a sender-key miss must be transient so the message is retried, not stored as an error: Unable to decrypt chat message: sender key not found
    FAIL github.com/keybase/client/go/chat
    
  • Mutation check: changing NewPermanentUnboxingError to NewTransientUnboxingError at that line makes the test pass. The TestV1Message* tests still pass with that change (it was not committed).
  • gofmt clean; golangci-lint --new-from-rev reports 0 issues.

Run: cd go && go test ./chat/ -run TestChatMessageUnboxSenderKeyMissIsNotPermanent -count=1

Fix options (for the follow-up, not in this PR)

  1. Classify a CheckKIDForUID miss as transient. This is the smallest change, but Boxer.UnboxMessages aborts the whole batch on a transient error. If the key really is missing, the thread pull would stay blocked.
  2. Force-reload the sender's UPAK once on a miss before giving up. CheckKIDForUID skips the forced reload when the first load touched the leaf or the user. Only a miss that survives the reload becomes an error.
  3. Add a "don't persist" error flavour, or have storage re-fetch stored sender-key errors. One way is to purge stored errors for that sender on UserChanged, so the next fetch unboxes them again.

Options 2 and 3 combine: reload first, and never cache the miss as final.

A storage-level test isn't possible without kbweb. Storage.Merge/Fetch need GetSecretBoxKey, which needs a signed-up user. This test pins the classification step that starts the chain.

Merge Danger

Door: two-way

Test-only. Merging it alone turns go/chat red until a fix lands, so it should land together with the fix or after it.

Blast Radius: go-chat-tests

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