OpenSC / pkcs11-helper

Library that simplifies the interaction with PKCS#11 providers for end-user applications using a simple API and optional OpenSSL engine
Other
66 stars 43 forks source link

Use EVP_PKEY methods to support PSS - part2 #34

Closed selvanair closed 3 years ago

selvanair commented 3 years ago

Part 2 of PR #31 replacement 3 commits

Lightly tested on Linux: Signing: EC, RSA-PKCS1, RSA-PSS Decrypt: RSA-PKCS1, RSA-OAEP

Notes:

Missing: update msvc makefile (how?)

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 09a8d3971e617d5dd83047d3e55c38ad9279daf5 into 295c35db39287a94c2ef47fc3c0277b02620d2b4 - view on LGTM.com

new alerts:

selvanair commented 3 years ago

Pushed fixup commits addressing the suggested changes. Leaving them as separate commits for ease of review -- will squash later.

alonbl commented 3 years ago

Hi,

For nmake, you can assume we use latest openssl.

For common file, of course if there is a common code we can have a separate file, I suggest for now to work on a separate file until we finalize the most effective method to support 1.1.1 and then look into commonality.

Can you please look into RSA_padding_add_PKCS1_type_1 family and see if it helps to simplify some of the implementation of manually constructing the padding?

Regards,

selvanair commented 3 years ago

In this PR we do not manually construct any padding -- instead we let the token handle the padding. The only place we are doing some preprocessing is for PKCS#1 v1.5 signature where encoded DigestInfo should be added to the hash before passing to PKCS#11 -- the one constructed in __pkcs11h_encode_pkcs1(). I haven't seen an OpenSSL API for that.

Other additional functions introduced are for filling in the mechanism parameters for PSS and OAEP.

For legacy tokens that cannot handle PSS or OAEP padding, I plan to use the RSA_paddingcheck and RSA_paddingadd family as pre- and post- processing routines (former for OAEP, latter for PSS). This is not included in this PR.

selvanair commented 3 years ago

@alonbl In retrospect, you were totally right about waiting for OpenSSL 3.0 before writing all this code... In OpenVPN we are moving to a provider for interfacing with external keys and my patch you merged here (CK_MECHANISM parameter support) would not only meet our needs, but is ideal -- we can now support PSS signature with virtually no extra code -- well the provider is a lot of glue code but that we have/had to do anyway.

Installing callbacks in the key has become deprecated with OpenSSL 3.0. For 1.1.1 and older we can live with the reduced functionality (no PSS, mainly).

TLDR: Can we expect a new release anytime soon?

Cheers Selva

alonbl commented 3 years ago

Hello @selvanair, Do we need anything of this patch? Regards,

selvanair commented 3 years ago

Not really --- for OpenSSL 3.0 pretty well nothing in the patch is usable. For 1.1.1, it would allow for easier use of PSS, but not worth the trouble with so much more code to maintain? Even with 1.1.1, one could get PSS support without this by directly calling ..._signAny_ex() instead of signAny() and bypassing openssl_session suport.

What is lost would be the convenience of openssl_session for setting up the callbacks. We can revisit that in the context of OpenSSL 3.0 providers to provide a glue so that application authors don't have to write a provider.

I'll withdraw this PR at some point.

alonbl commented 3 years ago

Thanks.

As currently we provide an engine, I guess we should provide a provider... right?

selvanair commented 3 years ago

Something like engine but the semantics of provider being different, will need a new approach. Essentially we give an API for the user to call, say, ossl_private_key_load(pkcs11-id) and give them an EVP_PKEY back. We could also return the certificate as an x509 in the same call -- I haven't thought of the design in detail. What we return will be a key attached to our internal provider, keydata to OpenSSL, but acts like an EVP_PKEY in all respects. The user can set the key on their SSL_CTX context with right propq, and all keymgmt and related (signature) operations will get delegated to our provider.

Let's get back to this once the provider I'm writing for OpenVPN is complete -- we need one in OpenVPN as management-external-key as well as Windows CNG are kind of "opaque" keys like in a token.

alonbl commented 3 years ago

As far as I understand each "technology" should provide a keystore with objects. While the application is completely agnostic to the underline low-level structure. Much like the Windows keystore.

I may be wrong, but it might be that you are implementing the "old ways" instead of adjusting to the new structure of openssl.

selvanair commented 3 years ago

Yes, each technology should give us a provider interface and act like a store. But sometimes a built-in provider in an application is unavoidable. And that's the case with --management-external-key in openvpn. It is special because its an interface within the application to many possible backends all accesed via a socket with no knowledge of what kind of a keystore is behind it -- so we need a shim provider inside.

For Windows CNG, we could (and ideally should) use an external provider but none is available as yet -- going by the state of CAPI engine, it may be years before that happens.

And, yes, I do not want to call signAny() if there is a provider interface in here. Then I can just load the cert and key through it a be rest assured. But until that happens, we have no option.

alonbl commented 3 years ago

I think that your provider for external key should be a keystore, we should build a keystore for the pkcs11-helper as well, each in its own provider, in future there will be CNG one... you should follow the architecture.

BTW: you can look at the archives, I believe openvpn must nuke the entire PK cryptography implementation from the core and always use the management interface. There should be a helper process to access the keys. This way the privileged openvpn user (or unprivileged) has any privilege to directly access the keys. This reduces much of the complexity of the daemon, less attack surface.

selvanair commented 3 years ago

@alonbl In retrospect, you were totally right about waiting for OpenSSL 3.0 before writing all this code... In OpenVPN we are moving to a provider for interfacing with external keys and my patch you merged here (CK_MECHANISM parameter support) would not only meet our needs, but is ideal -- we can now support PSS signature with virtually no extra code -- well the provider is a lot of glue code but that we have/had to do anyway.

For this reason I'm closing this.