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

ElementR: Fix missing key check values in 4S key storage #3950

Merged
merged 13 commits into from
Dec 18, 2023

Conversation

BillCarsonFr
Copy link
Member

@BillCarsonFr BillCarsonFr commented Dec 11, 2023

Fixes element-hq/element-web#26721

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

🚨 BREAKING CHANGES

@BillCarsonFr BillCarsonFr requested a review from a team as a code owner December 11, 2023 10:38
@BillCarsonFr BillCarsonFr requested review from t3chguy, florianduros and richvdh and removed request for t3chguy December 11, 2023 10:38
@BillCarsonFr BillCarsonFr changed the title fix missing key check in key storage ElementR: Fix missing key check in key storage Dec 11, 2023
@richvdh richvdh changed the title ElementR: Fix missing key check in key storage ElementR: Fix missing key check values in 4S key storage Dec 11, 2023
@richvdh richvdh removed their request for review December 11, 2023 16:45
@@ -100,10 +100,12 @@ export interface PassphraseInfo {
* Options for {@link ServerSideSecretStorageImpl#addKey}.
*/
export interface AddSecretStorageKeyOpts {
pubkey?: string;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is some legacy field that was used for the deprecated m.secret_storage.v1.curve25519-aes-sha2 algorithm.

@@ -713,7 +713,8 @@ export interface CrossSigningKeyInfo {
* Recovery key created by {@link CryptoApi#createRecoveryKeyFromPassphrase}
*/
export interface GeneratedSecretStorageKey {
keyInfo?: AddSecretStorageKeyOpts;
/** Information to generate the key from a passphrase if any. */
Copy link
Member

Choose a reason for hiding this comment

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

Nit: could be a bit clearer here

Suggested change
/** Information to generate the key from a passphrase if any. */
/** If the key was derived from a passphrase, information (algorithm, salt, etc) on that derivation. */

name?: string;
key?: Uint8Array;
/** The private key. Will be used to generate the key check values in the key info, it will not be stored on the server */
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change
/** The private key. Will be used to generate the key check values in the key info, it will not be stored on the server */
/** The private key. Will be used to generate the key check values in the key info; it will not be stored on the server */

@@ -713,7 +713,8 @@ export interface CrossSigningKeyInfo {
* Recovery key created by {@link CryptoApi#createRecoveryKeyFromPassphrase}
Copy link
Member

Choose a reason for hiding this comment

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

not a new issue, but this documentation is incomplete. This interface is also the return type of CreateSecretStorageOpts.createSecretStorageKey.

@@ -713,7 +713,8 @@ export interface CrossSigningKeyInfo {
* Recovery key created by {@link CryptoApi#createRecoveryKeyFromPassphrase}
*/
export interface GeneratedSecretStorageKey {
keyInfo?: AddSecretStorageKeyOpts;
/** Information to generate the key from a passphrase if any. */
passphrase?: PassphraseInfo;
Copy link
Member

Choose a reason for hiding this comment

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

The problem with this change (replacing the optional keyInfo property with an optional passphrase property) is that it means any existing implementation of CreateSecretStorageOpts.createSecretStorageKey (such as the one in matrix-react-sdk) will be incorrect. Worse, it will fail silently because new new property is optional. I think this is very dangerous.

I see two possible ways forward:

  • Retain the keyInfo property, but with a reduced structure. This is what I suggested before:
    export interface GeneratedSecretStorageKey {
        keyInfo?: {
            passphrase?: PassphraseInfo;
            name?: string;  // maybe we don't need this?
        },
        privateKey: Uint8Array;
        encodedPrivateKey?: string;
    }
  • Define a new interface to replace GeneratedSecretStorageKey (and deprecate the old one). Define a new property in CreateSecretStorageOpts to replace createSecretStorageKey which returns the new interface (and deprecate the old one); allow clients to provide either interface.

The second option ends up being more elegant (once we get rid of the deprecated classes/properties), but seems a lot more work.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm other than a few wording nits

src/crypto-api.ts Outdated Show resolved Hide resolved
src/crypto-api.ts Outdated Show resolved Hide resolved
spec/unit/rust-crypto/rust-crypto.spec.ts Outdated Show resolved Hide resolved
BillCarsonFr and others added 3 commits December 18, 2023 14:59
Co-authored-by: Richard van der Hoff <[email protected]>
Co-authored-by: Richard van der Hoff <[email protected]>
@BillCarsonFr BillCarsonFr added this pull request to the merge queue Dec 18, 2023
Merged via the queue into develop with commit 48d4f1b Dec 18, 2023
21 checks passed
@BillCarsonFr BillCarsonFr deleted the valere/element-r/fix-secret_storage_key_check branch December 18, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ElementR: Key Storage | When adding a new key to 4S the key encryption check is not added to the key info
2 participants