Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify secret storage format #1695

Merged
merged 2 commits into from
Dec 11, 2023
Merged

Clarify secret storage format #1695

merged 2 commits into from
Dec 11, 2023

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Dec 11, 2023

I found the description of how the m.secret_storage.key.[key ID] account data object is calculated extremely confusing.

Preview: https://pr1695--matrix-spec-previews.netlify.app

@richvdh richvdh requested a review from a team as a code owner December 11, 2023 12:22
For the purposes of allowing clients to check whether a user has
correctly entered the key, clients should:
5. Encode the IV from step 2, the ciphertext from step 3, and MAC from step 4,
using base64, and store them as the `iv`, `ciphertext`, and `mac`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question? Padded or unpadded base64? If padded, which variant?

Existing text just says "base64", which implies padded (of some variant). Web uses (I think) padded RFC4648 base64. But the spec also says

it’s expected that most encryption schemes would have ciphertext and mac properties, where the ciphertext property is the unpadded base64-encoded ciphertext

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I made a mistake not specifying this originally. We want it to be unpadded, to be consistent with everything else in Matrix, but since it was left unspecified, clients should be able to handle either one, for example by base64-decoding the MAC and comparing the binary value instead of comparing the base64 strings.

Comment on lines 111 to 112
| iv | string | The 16-byte initialization vector, encoded with base64. |
| mac | string | The MAC of the result of encrypting 32 bytes of 0, encoded with base64. |
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: are these required or not? If not, what are clients expected to do if they are absent? Reject the key, or carry on regardless?

cf element-hq/element-web#26721

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MSC said that clients "should" calculate and store the IV and MAC, not "must", so I guess they're optional. I don't remember why we made it optional.

If they're not present, we have to assume the key is correct, and carry on.

Comment on lines 111 to 112
| iv | string | The 16-byte initialization vector, encoded with base64. |
| mac | string | The MAC of the result of encrypting 32 bytes of 0, encoded with base64. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MSC said that clients "should" calculate and store the IV and MAC, not "must", so I guess they're optional. I don't remember why we made it optional.

If they're not present, we have to assume the key is correct, and carry on.

For the purposes of allowing clients to check whether a user has
correctly entered the key, clients should:
5. Encode the IV from step 2, the ciphertext from step 3, and MAC from step 4,
using base64, and store them as the `iv`, `ciphertext`, and `mac`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I made a mistake not specifying this originally. We want it to be unpadded, to be consistent with everything else in Matrix, but since it was left unspecified, clients should be able to handle either one, for example by base64-decoding the MAC and comparing the binary value instead of comparing the base64 strings.

@richvdh
Copy link
Member Author

richvdh commented Dec 11, 2023

@uhoreg: thanks for the clarifications. Have attempted to clarify the spec text.

@richvdh richvdh merged commit 37ab151 into main Dec 11, 2023
12 checks passed
@richvdh richvdh deleted the rav/clarify_secret_storage branch December 11, 2023 16:29
zecakeh added a commit to zecakeh/matrix-spec that referenced this pull request Feb 27, 2024
Introduced in matrix-org#1695.

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
tulir pushed a commit that referenced this pull request Feb 27, 2024
* Fix typo in secrets module

Introduced in #1695.

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

* Add changelog

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

---------

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
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.

None yet

2 participants