Accept key backups as usable if they're signed with the master cross-signing key #1492
Conversation
| @@ -1154,6 +1154,8 @@ - (MXKeyBackupVersionTrust *)trustForKeyBackupVersionFromCryptoQueue:(MXKeyBacku | |||
| } | |||
| else | |||
| { | |||
| keyBackupVersionTrust.usable = YES; | |||
There was a problem hiding this comment.
The existing parent else branch does not ensure that the deviceId is indeed a master key (it only mentions it in a comment), as opposed to other type of key. Could you please change the else // Try interpreting it as the MSK public key into something like else if ([deviceId isEqualToString:masterKey])? The master key can be obtained for instance as crypto.crossSigning.myUserCrossSigningKeys.masterKeys.keys. See also Android implementation for reference.
If the comparison check fails, we can log that as another issue.
Additionally it may be worth renaming the deviceId local variable into deviceIdOrCrossSigningKey, otherwise the name can be misleading
There was a problem hiding this comment.
Only a month and a half later I've finally addressed this feedback, thanks @Anderas !
There was a problem hiding this comment.
Thanks for making the change. I see that we will now be adding a signature object even if the master key verification did not succeed, but it will be set as invalid (plus this copies js / android behaviour) so it sounds good to me
bradtgmurray
force-pushed
the
accept-key-backups-signed-with-the-cross-signing-key
branch
from
a0b3a87
to
dda6477
Compare
MatrixSDKTests/MXCryptoBackupTests.m
Outdated
| // - Check the returned MXKeyBackupVersion is trusted | ||
| // -> It must be trusted by 2 entities | ||
| // -> Trusted by her device | ||
| // -> It must be trusted by 1 entities | ||
| // -> Trusted by her MSK | ||
| - (void)testCrossSigningMSKTrustForKeyBackupVersion |
There was a problem hiding this comment.
Repurposed an existing test. Based on the name it seems to have been wanting to test MSKTrust, but because the valid device signature was marking the device usable it was hiding the bug that I'm fixing with this PR. Now the only signature on the backup is the MSK.
Codecov Report
@@ Coverage Diff @@
## develop #1492 +/- ##
===========================================
- Coverage 12.05% 12.04% -0.01%
===========================================
Files 512 512
Lines 83831 83839 +8
Branches 35790 35792 +2
===========================================
- Hits 10103 10097 -6
- Misses 73360 73374 +14
Partials 368 368
Continue to review full report at Codecov.
|
bradtgmurray
force-pushed
the
accept-key-backups-signed-with-the-cross-signing-key
branch
from
5081a33
to
1e23875
Compare
|
Strange, these tests seem to pass locally, any ideas? |
| XCTAssertNotNil(keyBackupVersionTrust); | ||
| XCTAssertTrue(keyBackupVersionTrust.usable); | ||
| XCTAssertEqual(keyBackupVersionTrust.signatures.count, 2); |
There was a problem hiding this comment.
The new test looks good, however it might be better to create it as copy and leave testCrossSigningMSKTrustForKeyBackupVersion as is, otherwise we loose the test case for getting a device signature (XCTAssertEqual(keyBackupVersionTrust.signatures.count, 2))
| @@ -1154,6 +1154,8 @@ - (MXKeyBackupVersionTrust *)trustForKeyBackupVersionFromCryptoQueue:(MXKeyBacku | |||
| } | |||
| else | |||
| { | |||
| keyBackupVersionTrust.usable = YES; | |||
There was a problem hiding this comment.
Thanks for making the change. I see that we will now be adding a signature object even if the master key verification did not succeed, but it will be set as invalid (plus this copies js / android behaviour) so it sounds good to me
Unfortunatelly crypto + integration tests are very flaky, we are in the process of enabling + fixing them. I'd say make sure that they pass on your local branch, and optionally you can rerun the GitHub tests a few times. We cannot just turn them off because locally they are not as flaky and thus hard to spot the misbehaving ones. |
bradtgmurray
force-pushed
the
accept-key-backups-signed-with-the-cross-signing-key
branch
from
1f1324a
to
43c107e
Compare
Accept key backups if they're signed by the master cross-signing key.
Used matrix-js-sdk as a reference which accepts it as usable if either the signing device is verified or it's the cross-signing key. https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/crypto/backup.ts#L426-L433
Signed-off-by: Brad Murray brad@beeper.com
Pull Request Checklist