EncryptorDecryptor Trait for Logins Component

[jo]

The encryption and decryption of credentials is outsourced to a Foreign Trait so that Android, desktop and iOS can each bring their own implementations of encryption, including key management. This makes the Logins API way simpler and cleaner. It is the first step in making AS-Logins desktop-ready.

The new EncryptorDecryptor trait replaces the current EncryptorDecryptor struct. Instead of using the decrypt_struct method, which involves serializing and deserializing the string to be encrypted, this trait uses only byte-based operations and serialization is up to the consumer.

We do not Uniffi-expose the crypto primitives encrypt, decrypt, encrypt_struct and decrypt_struct anymore. Also EncryptedLogin will not be exposed anymore.

A ManagedEncryptorDecryptor will provide an EncryptorDecryptor implementation which uses the currently used crypto methods, given a KeyManager implementation to ease adaption for mobile.

BREAKING CHANGE

This commit introduces breaking changes to the Logins component:

During initialization, it receives an additional argument, a
EncryptorDecryptorTrait implementation. In addition, several LoginsStore
API methods have been changed to not require an encryption key argument
anymore, and return Logins objects instead of EncryptedLogins.

Additionally, a new API method has been added to the LoginsStore,
has_logins_by_base_domain(&self, base_domain: &str), which can be used
to check for the existence of a login for a given base domain.

EncryptorDecryptor

With the introduction of the EncryptorDecryptor trait, encryption
becomes transparent. That means, the LoginStore API receives some
breaking changes as outlined above. A ManagedEncryptorDecryptor will
provide an EncryptorDecryptor implementation which uses the currently
used crypto methods, given a KeyManager implementation. This eases
adaption for mobile. Furthermore, we provide a StaticKeyManager
implementation, which can be used in tests and in cases where the key is

• you name it - static. Constructors Now an implementation of the above
  property must be passed to the constructors. To do this, the signatures
  are extended as follows:

pub fn new(path: impl AsRef<Path>, encdec: Arc<dyn EncryptorDecryptor>) -> ApiResult<Self>
pub fn new_from_db(db: LoginDb, encdec: Arc<dyn EncryptorDecryptor>) -> Self
pub fn new_in_memory(encdec: Arc<dyn EncryptorDecryptor>) -> ApiResult<Self>

LoginStore API Methods

This allows the LoginStore API to be simplified as follows, making
encryption transparent by eliminating the need to pass the key and
allowing the methods to return decrypted login objects.

pub fn list(&self) -> ApiResult<Vec<Login>>
pub fn get(&self, id: &str) -> ApiResult<Option<Login>>
pub fn get_by_base_domain(&self, base_domain: &str) -> ApiResult<Vec<Login>>
pub fn find_login_to_update(&self, entry: LoginEntry) -> ApiResult<Option<Login>>
pub fn update(&self, id: &str, entry: LoginEntry) -> ApiResult<Login>
pub fn add(&self, entry: LoginEntry) -> ApiResult<Login>
pub fn add_or_update(&self, entry: LoginEntry) -> ApiResult<Login>

We will stop Uniffi-exposing the crypto primitives encrypt, decrypt,
encrypt_struct and decrypt_struct. Also EncryptedLogin will not be
exposed anymore. Checking for the Existence of Logins for a given Base
Domain In order to check for the existence of stored logins for a given
base domain, we provide an additional store method,
has_logins_by_base_domain(&self, base_domain: &str), which does not
utilize the EncryptorDecryptor.

[mhammond]

This is fantastic and a massive improvement, thanks! Dealing with Android and iOS will be a challenge though.

[bendk]

Looks really great to me, I like the overall approach. I commented on some of the details -- treat them as suggestions only. I don't have strong feelings either way.

Hopefully the consumer app updates aren't too bad.

The pull request has been modified, dismissing previous reviews.

[bendk]

This looks great to me. A lot of code is changing, but in general it seems like the same system as before only better. I mostly just had suggestions around the error handling.

The pull request has been modified, dismissing previous reviews.

[jo]

This looks great to me. A lot of code is changing, but in general it seems like the same system as before only better. I mostly just had suggestions around the error handling.

Thanks for the good points!

[bendk]

The changes look great to me, I'm all for merging this. I think we should get a second review if possible though.

The pull request has been modified, dismissing previous reviews.

[mhammond]

This is fantastic!

[mhammond]

I'm really happy to see this die. Back in the day, we were torn about whether the logins store should be able to function while in a "locked" state - ie, we were picturing a world where the app knows a login exists for a site, but the app must force the user to unlock via biometrics etc before the actual details were available, which then allows the app to "re-lock" the store at its convenience.

It looks like you've decided this is a non-goal, and your team is the correct team to make that decision, and the extra simplicity that comes with that is a nice bonus!

You might be able to take this further though - it seems that without EncryptedLogin you might also be able to flatten out the Login record so avoid the nested records (ie, can RecordFields, and SecureLoginFields now die?) While that might be able to be done as a followup, the cost of re-breaking the mobile platforms might mean the opportunity to do so might not arise. IMO, the current splitting of these records makes the use of this component in the mobile code quite complicated to understand so would likely benefit from a simplification here.

(alternatively, a different split might make sense - eg, split out "editable" fields from "computed" fields, but even that might not have enough value)

[jo]

Yes, I think the advantages of a simplified way of working for LoginsStore outweigh the disadvantages. The idea of a locked state further complicates things and has a knock-on effect at lower levels. We can still perform certain operations without the store unlocked – for example, I added (thanks @bendk!) the method has_logins_by_base_domain(&self, base_domain: &str) so that the application can still check whether logins exist for a specific domain.

I like that you suggest further simplifying the login structure. I hadn't even dared to tackle that :) I'll discuss it with the mobile teams. On Android, the code is already abstracted, so the changes would be very simple here. On iOS, I'm not sure. At the latest now, a decoupling on the iOS side seems sensible to me in order to be able to make adjustments more agilely.

Furthermore, we would actually like to encrypt more fields, especially the domain. This information could already be interesting for certain attackers and some users could therefore support such changes. Although this would complicate or even prevent the lookup by domain in the locked state.

[jo]

@mhammond something like this: jo#2?

[mhammond]

Yeah! I'm a little torn by RecordFields though - roughly speaking, they are fields which the consumer should never provide - eg, when creating a login you never specify the id or the number of times it has been used. This is a tension we often face - use different structs for "get" vs "set" operations, which makes consumers a little more painful in some ways, but makes things clearer in others.

But certainly the flattening of LoginFields looks like a big improvement.

[jo]

Yes, I know, this is an old problem. And then, of course, there is something appealing about grouping all these meta fields, which only play a role when reading, into a separate record struct. And on the other hand, there is this simplicity and clarity ¯_(ツ)_/¯

[mhammond]

exactly :) It sounds like we agree there's no objectively correct answer here, so I can get behind whatever you decide!

[mhammond]

The canary story can probably be better too - the intent there is to better work with keys which are our of our control. IIUC there's no equivalent thing on desktop. Maybe that concept could be optionally added to the traits? This might be good fodder for a followup.

[jo]

Yes, very good idea!

The pull request has been modified, dismissing previous reviews.