As mentioned, I already suspected the “let’s only make the FIDO devices secure” bit to be controversial.
Here I guess that making the whole feature opt-in already means leaving the threat open for most people, but I see your point. My fear is that by allowing recovery phrases to be “secured” in the way we discussed we make it easy for users to think their account is secure enough, whereas really it carries all the issues mentioned here:
Maybe a UX expert can chime in here?
About the implementation, you say
And while it’s true that in theory everything can be reversed, making changes to the stable memory does come at a cost (that of twiddling with the schema and opening the door to really bad bugs, as well as the cost of preparation to make sure upgrades and rollbacks do still somewhat work). There’s also the problem that once we roll this out, it’s not really “reversible” in the sense that we can’t tell people “your recovery phrase is secure!” and then roll back the feature a week later, that would defeat the purpose.
Some pointers on implementation now; the code suggest that a Storage<T> object is the only thing read from stable memory:
If we follow the code and the types, we see this is actually a Storage of a vector of DeviceDataInternal:
and here’s the definition:
So the stable memory is cut in chunks that are indexed by anchors, and where each chunk contains a vector of devices, where some values seem hardcoded/computed by hand:
One thing to note is that the canister doesn’t really make a distinction between “recovery” and “authentication” devices (it looks like the purpose field is only read by the frontend code). This means that the simplest way to implement the feature is to add some data to DeviceDataInternal (meaning to all devices, including recovery phrases, recovery devices, etc) which specifies whether the user should prove they have the private key before removing the device; this is quite a big change and we’d need to think hard. Also, whatever data we add there, we need to make sure the new storage object fits. Depending on the implementation we’ll also need a migration step (say if we go from storage Vec<DeviceDataInternal> to Vec<DeviceDataInternalWithExtraData>).
All of this to say that it’s not quite backwards compatible, and while in theory it is reversible, in practice it’ll be non-trivial and there’s some risk. Voilà!