sailfishos / sailfish-secrets

BSD 3-Clause "New" or "Revised" License
24 stars 15 forks source link

Where to store and how to pass a user's password for Tokens? #23

Open Denis-Semakin opened 6 years ago

Denis-Semakin commented 6 years ago

Hello. In order to perform a common cryptographic operation using CryptoToki API (for Tokens) one need to call C_Login() function which requires a user's password. By default this password should be generated and set up on Token initialization process by security administrator. However user can change its own password. So the question is how to pass this password to plugin's method since the parent abstract class CryptoPlugin has now such opportunity and parameters? Any thoughts are welcome.

chriadam commented 6 years ago

Hi! I suspect we may need to add a "QVariantMap customParameters" argument to each method in the CryptoPlugin interface (similar to what was done for the KDF method). This seems somewhat ugly, but probably necessary. Do you agree, or do you think some other solution would be better?

Denis-Semakin commented 6 years ago

@chriadam Hi. I think we don't need to pass "customParameters" in every single cryptoki method. User just need to be authorized to run them. Let me show a steps: The following steps must be performed before any actual method will be called. So I suppose it should be invoked inside constructor of CryptoPlugin, if it's possible and then we need to pass (or to get it) a user's password there.

Denis-Semakin commented 6 years ago

@chriadam A CryptoPlugin should provide a number of methods, e.g. sign(), verify(), decrypt(), etc... To perform them with tokens a session with token have to be opened and user's password is used for this. I see two ways to solve this:

Denis-Semakin commented 6 years ago

@chriadam BTW I vote to your idea about initialize(parameters) method that you told on '#'OMP channel. Then we need to design it correctly.

Denis-Semakin commented 6 years ago

@chriadam Hi. I wouldn't like to create a new issue, so let me write a little bit off-topic question here. Diving into tokens theme I found a new question. now it's about verify() method that has a prototype below:

verify(const QByteArray &data,
          const Sailfish::Crypto::Key &key,
          Sailfish::Crypto::CryptoManager::SignaturePadding padding,
          Sailfish::Crypto::CryptoManager::DigestFunction digest,
          bool *verified)

I think that here signature is missed. data - is the data that's needed to be veryfied, padding and digest -- are not a signatures, verified -- out coming parameter that indicates abous success or not. And where is signature itself? The prototype of Cryptoki verify() function looks like:

C_Verify(hSession, data, length, signature, nSignatureLength));

Where hSession - a session handler, data -- a data that we need to verify (verify with what?), signature - a pointer to array of bytes that hold a signature, nSignatureLength -- a length of signature. There are a lot of examples of source code that can be found of it, and everywhere signature is used. But here it's at least is not clear for me how to get it. Thanks!

chriadam commented 6 years ago

Hi, yes I should add signature QByteArray parameter. Originally I had thought that the signature would be included (e.g. prepended or appended to) the data, then the padding+digest information could be used to extract the signature from the data and perform the verification, but that is probably far less user-friendly than just requiring the client to provide separate data + signature params. I will add.

chriadam commented 6 years ago

I've added the signature parameter to the verify method, and also added the CalculateDigestRequest, in https://github.com/sailfishos/sailfish-secrets/pull/29

chriadam commented 6 years ago

The masterlock stuff was added in https://github.com/sailfishos/sailfish-secrets/pull/30 which allows asynchronously providing an unlock code to a plugin (and to set the new unlock code by passing old+new code parameters).

We may need to extend the existing functionality by modifying LockCodeRequest in the following way: instead of secretName+collectionName parameters, have a enum TargetType { BookkeepingDatabase, StandaloneSecret, Collection, ExtensionPlugin } and QString targetName properties. That way, we would allow providing separate / explicit lock codes for different plugins (i.e. separate from the masterlock code). Let me know if you think that would be useful / needed.

chriadam commented 6 years ago

The API to allow plugin-targeted lock code requests can be found at: https://github.com/sailfishos/sailfish-secrets/pull/38.

Denis-Semakin commented 6 years ago

I think this issue can be closed.