MSC4388: Secure out-of-band channel for sign in with QR #4388
Conversation
hughns
changed the title
turt2live
added
proposal
| Regardless of which device generates the QR code, either device can be the existing (already signed in) device. The | ||
| other device is then the new device (one seeking to be signed in). | ||
|
|
||
| Symmetric encryption uses a separate encryption key for each sender, both derived from a shared secret using HKDF. |
There was a problem hiding this comment.
| Symmetric encryption uses a separate encryption key for each sender, both derived from a shared secret using HKDF. | |
| Bidirectional encryption uses a separate encryption key for each sender, both derived from a shared secret using HKDF. |
Perhaps?
There was a problem hiding this comment.
This was meant to talk about symmetric encryption as opposed to public key encryption, not specifically about the bidirectional part of the symmetric encryption.
Not sure how to clarify this more. Perhaps we just leave the jargon out and describe this just as "encryption" or "message encryption"?
There was a problem hiding this comment.
Right. I think leaving the jargon out would be clearer.
How about this?
| Symmetric encryption uses a separate encryption key for each sender, both derived from a shared secret using HKDF. | |
| Each device uses a distinct encryption key for sending messages. These encryption keys are derived from a shared secret using HKDF. |
There was a problem hiding this comment.
Clearer for whom? Certainly not clearer for cryptographic engineers who will be familiar with the jargon and who will hopefully be implementing this. Leaving it out can sometimes make clear things less clear.
That said, I don't believe the original formulation was adding much and your suggestion is fine-ish.
However, what I'm missing is a connection to the HPKE RFC. If we're telling people "this is HPKE" and linking to the RFC, surely we should be connecting it to the RFC terminology as much as possible? We also say above that this is "HPKE [...] instantiated with [...] ChaCha20-Poly1305 for the authenticated encryption" but we never mention the term "authenticated encryption" again. So for a reader who is not familiar with the jargon (i.e. is not a cryptographic engineer) and who therefore cannot connect all the dots themselves, it's entirely unclear which encryption key we're describing here, so I'm not sure the clarification actually helps.
Relatedly, why are we even retelling this here when it should be obvious from the RFC? Rather than talking about the key schedule directly, we should be tying the language of the MSC with the Context<ROLE> language of the RFC.
proposals/4388-secure-qr-channel.md
Outdated
|
|
||
| Symmetric encryption uses a separate encryption key for each sender, both derived from a shared secret using HKDF. | ||
| Separate nonces are used for each direction of the communication channel. Device S will create a base nonce from the | ||
| shared secret, while Device G will create a new random base nonce. Each base nonce is mixed with a |
There was a problem hiding this comment.
| shared secret, while Device G will create a new random base nonce. Each base nonce is mixed with a | |
| shared secret, while Device G will create a new random base nonce. Each base nonce is then mixed with a |
Also, "mixed" sounds odd to me here. Maybe "combined" is better?
There was a problem hiding this comment.
I think that's a term that's somewhat commonly used when different sources of randomness are combined.
But I think "combined" works better here. Agree.
|
|
||
| ``` | ||
| TaggedCiphertext := Context_S_S.Seal("MATRIX_QR_CODE_LOGIN_INITIATE", "") | ||
| LoginInitiateMessage := UnpaddedBase64(TaggedCiphertext) || "|" || UnpaddedBase64(Sp) |
There was a problem hiding this comment.
For consistency does it make sense to get rid of the | string separator and rely on the known length of the keys? (e.g. 32 bytes in this case)
| LoginInitiateMessage := UnpaddedBase64(TaggedCiphertext) || "|" || UnpaddedBase64(Sp) | |
| LoginInitiateMessage := UnpaddedBase64(TaggedCiphertext || Sp) |
There was a problem hiding this comment.
Sure, I don't mind them being gone. Not sure if @dkasak has some attachment to those separators.
There was a problem hiding this comment.
So we would chop off a 32 byte suffix in order to reconstruct the ciphertext? It might make more sense then to swap the order of Sp and TaggedCiphertext.
There was a problem hiding this comment.
Yeah, I would expect us to change the order if the | is gone.
There was a problem hiding this comment.
| LoginInitiateMessage := UnpaddedBase64(TaggedCiphertext) || "|" || UnpaddedBase64(Sp) | |
| LoginInitiateMessage := UnpaddedBase64(Sp || TaggedCiphertext) |
| |-|-|-| | ||
| |`id`|required `string`|Opaque identifier for the rendezvous session| | ||
| |`sequence_token`|required `string`|The token opaque token to identify if the payload has changed| | ||
| |`expires_ts`|required `integer`|The timestamp (in milliseconds since the epoch) at which the rendezvous session will expire| |
There was a problem hiding this comment.
concern: using absolute timestamps is hazardous because clients and servers can be desynchronised, leading to severe disagreements in when the rendezvous session should expire, including the client expiring the session immediately (see a concrete & real-world-triggered issue with the JS SDK's implementation of MSC4108 (2024 version) here: matrix-org/matrix-js-sdk#5141)
Should prefer expires_in or at least provide (and require use of) some way of converting this timestamp into a relative time.
|
|
||
| ``` | ||
| TaggedCiphertext := ResponseContext_G.Seal("MATRIX_QR_CODE_LOGIN_OK", "") | ||
| LoginOkMessage := UnpaddedBase64Encode(TaggedCiphertext || ResponseBaseNonce) |
There was a problem hiding this comment.
| LoginOkMessage := UnpaddedBase64Encode(TaggedCiphertext || ResponseBaseNonce) | |
| LoginOkMessage := UnpaddedBase64Encode(ResponseBaseNonce || TaggedCiphertext) |
Rendered
This was originally part of MSC4108 but has been split out to make review easier.