[bilelmoussaoui]

As that can lead to failures when the user has been using a short secret in gnome-keyring or other keyring formats. Enforcing a stronger password doesn't help anywhere at all.

Fixes #542

[github-actions]

📊 Code Coverage Report

Metric	Value
Current PR Coverage	83.54%
Main Branch Coverage	83.46%
Coverage Change	📈 +0.08%

Coverage report generated by cargo-tarpaulin

[AdamWill]

Looking at this, doesn't it potentially inadvertently enforce more checks than before when importing from the legacy format?

Previously we used keyring.derive_key_unchecked there, which looked like this:

    pub(crate) fn derive_key_unchecked(&self, secret: &Secret) -> Result<Key, crypto::Error> {
        crypto::derive_key(
            &**secret,
            Ok(()),
            &self.salt,
            self.iteration_count.try_into().unwrap(),
        )
    }

In this patch we get rid of that function entirely. In legacy key import where we previously used it, we now use derive_key, which now looks like this:

    pub fn derive_key(&self, secret: &Secret) -> Result<Key, crypto::Error> {
        crypto::derive_key(
            &**secret,
            self.key_strength(),
            &self.salt,
            self.iteration_count.try_into().unwrap(),
        )
    }

which means we're now applying the iteration count and salt size checks in key_strength to it where we weren't before, right? Is that a good idea? (edit: or are those checks of the keyring to which the credentials are being imported and thus probably OK?)

[mcatanzaro]

As that can lead to failures when the user has been using a short secret in gnome-keyring or other keyring formats. Enforcing a stronger password doesn't help anywhere at all.

It also doesn't make sense, because secrets are not necessarily passwords. E.g. one of my secrets is a low-security PIN that is only 4 characters. Still needs to work. I don't myself have any secrets shorter than that, but clearly some users do.

[bilelmoussaoui]

We do have tests to cover this at https://github.com/linux-credentials/oo7/blob/main/server%2Fsrc%2Fservice%2Ftests.rs#L1213-L1312

I would have to double check but writing a test to confirm there is no regression should be pretty straightforward

[bilelmoussaoui]

yes, we do actually have tests for all of this + c6c35e8 that ensures short keyring secrets are actually migrated just fine

[AdamWill]

A test covers the specific scenario tested, though. That doesn't necessarily prove there is no scenario in which applying the additional checks where they were not applied before is a problem. That seems more like something for a person to think through carefully than something you can prove with a test suite.

[bilelmoussaoui]

I am not sure what are you trying to say here.

There are two things:

• the encryption key validation, only done for v1 and not affecting v0 to v1 migration at all. It mostly checks for iteration count,...
• the secret validation, it used to do length checks because that oo7::Secret type used to be in internal implementation detail when we only had the client side of things. Since the start of the server side, we ended up making it more generic and forgot to drop the secret length check. The unchecked function you mentioned in your comment was mostly meant for skipping secret length as the v0 parser does not do any key validation. Not even v1 does it at read time, only at write.

Hope this convinces you more than a test case.

[AdamWill]

I don't know the details of the code flow at all, I don't know what v0 and v1 are or what migration between them are or when we're reading and when we're writing. I just noticed this PR applies checks on an operation that previously did not have them applied and wanted to verify that won't cause unexpected failures.

[bilelmoussaoui]

It doesn't.

[bilelmoussaoui]

To clarify,

v0 is the file format used by gnome keyring for the host keyring. V1 is a format that was meant to replace v0 in gnome keyring but never happened and is only used by libsecret if the app is sandboxed.

Oo7 uses v1 by default and offers auto migration from v0.