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

rust backup support ground work #3548

Closed
wants to merge 14 commits into from

Conversation

BillCarsonFr
Copy link
Member

@BillCarsonFr BillCarsonFr commented Jul 6, 2023

Ground work for support of key backup in rust:

Fixes https://github.com/vector-im/crypto-internal/issues/104
Fixes https://github.com/vector-im/crypto-internal/issues/105

Changes:

  • megolm-backup.spec.ts to run with both crypto implementations. Replace use of TestClient (using fetch mock). Rust test is now skipped until all implemented
  • Moved {get,store}SessionBackupPrivateKey to CryptoApi. Depends on WebR Expose matrix-sdk-crypto-js bindings for KeyBackup matrix-rust-sdk#2196 for rust implementation
  • Created an interface for backupMachine, with some initial methods and mocks for rust.
  • Added getBackupManager() in crypto API and deprecated direct access to crypto!.backupManager, rust implem stubbed for nw
  • Moved signObject to CryptoApi, so that megolm-backup.spec.ts stops using deprecated crypto
  • The client was not stopped correctly when there is an active backup, added a stop() api in backupManager to clear any pending backup upload.

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)

This PR currently has none of the required changelog labels.

Add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is plus X-Breaking-Change if it's a breaking change.

@BillCarsonFr BillCarsonFr requested a review from a team as a code owner July 6, 2023 12:56
@t3chguy
Copy link
Member

t3chguy commented Jul 6, 2023

Looks like this needs a rust crypto update to function, will leave it to vdh as the matter expert

@t3chguy t3chguy removed their request for review July 6, 2023 12:59
@BillCarsonFr
Copy link
Member Author

Looks like this needs a rust crypto update to function, will leave it to vdh as the matter expert

I commented out for now, and just put a blank stub to see what CI is saying

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.

going to a meeting, some initial comments

beforeAll(function () {
return Olm.init();
/**
* Integration tests for cross-signing functionality.
Copy link
Member

Choose a reason for hiding this comment

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

this looks like a C&P error

return session;
});
}
// jest.useFakeTimers();
Copy link
Member

Choose a reason for hiding this comment

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

dead code

spec/integ/crypto/megolm-backup.spec.ts Outdated Show resolved Hide resolved
spec/integ/crypto/megolm-backup.spec.ts Outdated Show resolved Hide resolved
Comment on lines 47 to 84
export interface KeyBackupStatus {
version: string;
enabled: boolean;
}

export type SigInfo = {
deviceId: string;
valid?: boolean | null; // true: valid, false: invalid, null: cannot attempt validation
device?: DeviceInfo | null;
crossSigningId?: boolean;
deviceTrust?: DeviceTrustLevel;
};

export type TrustInfo = {
usable: boolean; // is the backup trusted, true iff there is a sig that is valid & from a trusted device
sigs: SigInfo[];
// eslint-disable-next-line camelcase
trusted_locally?: boolean;
};

export interface IKeyBackupCheck {
backupInfo?: IKeyBackupInfo;
trustInfo: TrustInfo;
}

export interface SecureKeyBackup {
getKeyBackupStatus(): Promise<KeyBackupStatus | null>;

stop(): void;

/**
* Check the server for an active key backup and
* if one is present and has a valid signature from
* one of the user's verified devices, start backing up
* to it.
*/
checkAndStart(): Promise<IKeyBackupCheck | null>;
}
Copy link
Member

Choose a reason for hiding this comment

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

these things all need good doc-comments explaining the concepts they represent and what each of the properties are for.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, will try revert engineer it ;)

Comment on lines +301 to +306
/**
* sign the given object with our ed25519 key
*
* @param obj - Object to which we will add a 'signatures' property
*/
signObject<T extends ISignableObject & object>(obj: T): Promise<void>;
Copy link
Member

Choose a reason for hiding this comment

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

do you think we actually need this? I note that the only place it is called (MatrixClient.createKeyBackupVersion) has this comment:

This can probably go away very soon in
favour of just signing with the cross-singing master key.

is "very soon" less than 3.5 years?

Copy link
Member Author

Choose a reason for hiding this comment

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

mm, not sure about the comment. What we will stop doing is trusting a backup if it's signed by a device we trust. But I think signing with our own device is a way to mark as trusted.

@BillCarsonFr BillCarsonFr requested a review from a team as a code owner July 6, 2023 21:15
@BillCarsonFr BillCarsonFr changed the base branch from develop to valere/refactor_backup_test July 6, 2023 21:15
@richvdh
Copy link
Member

richvdh commented Jul 7, 2023

I think this is on hold pending #3555 ? marking as draft for now

@richvdh richvdh marked this pull request as draft July 7, 2023 10:57
/**
* Implementation of {@link CryptoApi#signObject}
*/
public async signObject<T extends ISignableObject & object>(obj: T): Promise<void> {
Copy link
Member Author

Choose a reason for hiding this comment

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

I need to update that it's not correct at the moment, but having this rust PR available would make it easier to implement

@t3chguy t3chguy removed request for a team and kerryarchibald July 20, 2023 11:11
Base automatically changed from valere/refactor_backup_test to develop July 25, 2023 20:58
@richvdh
Copy link
Member

richvdh commented Jul 26, 2023

This has been superceded by #3622 and #3623 (and some of the bits that were in here that should have been in #3555 have moved there).

@richvdh richvdh closed this Jul 26, 2023
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.

3 participants