-
Notifications
You must be signed in to change notification settings - Fork 382
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
MSC1756: cross-signing devices using a master identity key #1756
Conversation
Some observations from a security perspective:
|
I wonder if the {
"failures": {},
"device_keys": {
"@alice:example.com": {
"JLAFKJWSCS": { ... },
"unsigned": {
"device_display_name": "Alice's mobile phone",
"attestations": {
"base64+encoded+public+key": "base64+encoded+signature"
}
}
}
}
},
"verified_master_keys": {
"@user:example.com": {
"key": "base64+encoded+public+key",
"signature": "base64+encoded+signature",
}
}
} Where:
In particular, I'm leaning towards simply signing the actual keys, rather than signed JSON of a bunch of stuff. This is because a) its the keys we care about, and b) signed JSON is a bit of a PITA and I'd prefer it if we avoid it where possible. |
At a high level this is looking good to me - thanks for writing it up. My main concerns are:
|
Agree with @ara4n, and thanks again for writing it up! 👍
Yup. Though we'll probably still want a separate backup that is instead simply encrypted by the master key when stored on the server, as:
|
I think this is how the incremental keybackup stuff works already? (as per #1703 and its predecessors) |
@ara4n, @erikjohnston: I've updated the proposal with the alternative API. I'll do some more thinking about how to integrate with the key backup stuff. |
Looking good! FTR, I much prefer proposal 2 where we differentiate between devices and the master key, as really the only thing devices and master keys have in common are they both share a key ID namespace |
@richvdh is there any chance you could take a quick sanity check over this (so we can avoid another situation where we get valuable but last minute feedback after the impl has already happened O:-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks broadly good to me. My main concern is around how we get hold of the private master key so that we can sign other users' keys, without the user having to type in his recovery password every 30 seconds.
As I understand it, the user must choose, (a) "store master key on device" vs. (b) "store wrapped master key on server." (b) is a non-starter because most users won't participate in the feature. The overall feature only benefits a users' friends, not themselves, so they will not go to any effort to learn about it, nor ever achieve full understanding of the problem you are solving even while they are annoyed by the problem. In ~all cases of a new login, the proposed feature should be used, so it should happen as part of the regular login flow similar to Google's "did you sign in from device X?" notifications on Android. At most using the feature can require a quick "[confirmed]" on an old device to add a new one. It can't require a tier 2 emergency password to do something that will happen exactly as often as the tier 1 ordinary password gets used; if optional it won't happen, and if mandatory the tiers are meaningless. IMHO, (b) should not be implemented, optionally or otherwise. The alternate use-cases will be too confusing on our side, and we will tend to be overly generous in evaluating ourselves if the escape hatch exists, then be surprised when things don't typically go well in the wild. (a) doesn't handle revocations well. 1 Alice has 3 devices in the normal state (master key on all of them, in accordance with (a)). For a protocol to have "revocation" in a meaningful way, (3) must mitigate (4). Yes, there are other scenarios to worry about, for example where Alice doesn't do revocation at all, or where (4) and (3) are inverted in time. But the perfect should not be allowed to be the enemy of the good. Baseline "meaningful" revocation is valuable because the gap between 2 and 4 is likely large, ex. a recycled hard drive. In this proposal, (3) seems to have become basically meaningless because the master key can't be revoked at all, or can't be revoked without nuking Alice's trust graph anchored to the non-lost devices, which in practice will often mean worse security than ignoring the lost device because in many cases the lost device will never be recovered by an actual hacker (it's probably something that got wiped), and nuking accounts frequently has a cost of teaching people to accept unverified keys, which become the more realistic actual attack than trawling for lost devices. In my opinion, a good revocation step would have this basic property:
and these advanced properties:
I don't know the best way to hit these revocation goals, especially the stuff around quorum. One degenerate implementation that falls short, but is an improvement:
The workflow with the paper key is: 1 Alice has 3 devices in the normal state (master key on all of them, in accordance with (a)). One thing to watch for is whether timestamps can be forged. It's probably better to express the watermark as a position in the crypto ratchet, not a timestamp, regardless of how the UI surfaces the feature, which is why I mention using a previously-sent message as a marker. This means device cross-signing needs to be put on the same ratchet somehow so any signatures made by the hacker can be revoked without the hacker evading that by pre-dating the signature, which might not be possible. : / Timestamp attacks vs ratchets may also apply to the quorum timers. : ( I think this degenerate scheme won't work well for users compared to the "quorum" rules because either users won't keep the paper key, or they will lose the paper key (which can't be itself rotated), but at least the degenerate scheme degrades gracefully enough to be strictly better than the existing proposal. I'm afraid I would make a mistake if I tried to implement the quorum rules. |
Thanks @caev for the thoughts, especially around how this would actually be used in the real world. Its taken me a while to digest it, but a few quick notes:
Technically there's also option c) store master key in a safe and only take it out on special occasions like adding a new device or recovering you account, etc. Though this is more for power user folks, so doesn't really change anything re your following points.
This is a very interesting way of looking at it. Certainly I've been assuming that clients would suitably prompt users to Do The Right Thing, e.g. prompting users to save their master key offline and/or upload an encrypted version to the server, with suitable UX to push people into not just skipping those steps. However this raises two questions: 1) is this enough to get people to use it and 2) will client implementors get it right (assuming Riot gets it right so can be used for reference)? Certainly its a bit unfortunate that there hasn't been a greater discussion around likely real world UX in this MSC, I know its being considered elsewhere.
Yup, I believe the idea is to do exactly this 👍. Again its unfortunate that the UX/UI proposals aren't linked to this MSC.
Hopefully it will get used in the wild, as above.
I think the ideas in this comment and elsewhere help with a lot of those concerns, though isn't as fully featured as your excellent suggestions. Basically, if you can pass user interactive auth (UIA) you can always revoke the master key and either a) blow away your master key and start again or b) if you have the old master key you can rotate to a new one. I'm not so sure about the quorum proposal, as it sounds easy to game by just adding enough new devices. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Written in a nice, understandable format, and I don't see any red flags as a server developer, so lgtm.
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period, with a disposition to merge, as per the review above, is now complete. |
Feature flag for this is |
should the feature flag be in the MSC? |
probably, yes. |
In which version will it be release? |
it is now on develop branches of Riot/Web, Riot/iOS & RiotX/Android (but we're still debugging it). |
Merged! 🎉 |
Rendered