Implement a provider side PKEY MAC bridge #12637

mattcaswell · 2020-08-12T14:37:33Z

This PR replaces the legacy EVP_PKEY to MAC bridge with a provider side version. After this PR the only remaining EVP_PKEY_METHOD still in place will be SM2.

crypto/evp/p_lib.c

providers/implementations/keymgmt/mac_legacy_kmgmt.c

providers/implementations/signature/mac_legacy.c

paulidale

Looks good after a first pass. A few questions and CIs are relevant.

mattcaswell · 2020-08-17T16:27:45Z

All feedback addressed. Also fixed some CI issues, and added some missing documentation.

slontis · 2020-08-18T00:34:01Z

crypto/evp/p_lib.c

+
+    if (cipher != NULL)
+        cipher_name = EVP_CIPHER_name(cipher);
+


Should we error here if there is no cipher_name for both the cases (cipher != NULL or cipher == NULL)?

Yes, since it's the cipher name that gets passed a few lines down

slontis · 2020-08-18T00:44:30Z

doc/man7/EVP_PKEY-HMAC.pod

+=head1 DESCRIPTION
+
+The B<HMAC>, B<SIPHASH>, B<POLY1305> and B<CMAC> keytypes are implemented in
+OpenSSL's default and FIPS providers. Performing MAC operations via an EVP_PKEY


HMAC and CMAC are the only ones in the fips provider.

slontis · 2020-08-18T00:45:30Z

doc/man7/EVP_PKEY-HMAC.pod

+
+=head2 CMAC parameters
+
+As well as all the parameters described above, the B<CMAC> keytype additionally


maybe remove all

slontis · 2020-08-18T00:48:19Z

doc/man7/EVP_SIGNATURE-HMAC.pod

+The algorithms described here have legacy support for creating MACs using
+L<EVP_DigestSignInit(3)> and related functions. This is not the preferred way of
+creating MACs. Instead you should use the newer L<EVP_MAC_init(3)> functions.
+However this mechanism is provided for backwards compatibility with older


Remove However?

slontis · 2020-08-18T00:52:59Z

providers/implementations/include/prov/implementations.h

@@ -277,6 +277,8 @@ extern const OSSL_DISPATCH ed25519_keymgmt_functions[];
 extern const OSSL_DISPATCH ed448_keymgmt_functions[];
 extern const OSSL_DISPATCH ec_keymgmt_functions[];
 extern const OSSL_DISPATCH kdf_keymgmt_functions[];
+extern const OSSL_DISPATCH mac_keymgmt_functions[];


I think these names should indicate they are legacy.. by adding legacy as part of the name.

slontis · 2020-08-18T00:54:38Z

providers/implementations/keymgmt/mac_legacy_kmgmt.c

+#include "prov/macsignature.h"
+
+# ifdef OPENSSL_SYS_WINDOWS
+#  define strcasecmp _stricmp


At some point these will need to be cleaned up..

slontis · 2020-08-18T01:01:39Z

providers/implementations/keymgmt/mac_legacy_kmgmt.c

+    return gettable_params;
+}
+
+


providers/implementations/keymgmt/mac_legacy_kmgmt.c

providers/implementations/signature/mac_legacy.c

doc/man3/EVP_PKEY_new.pod

doc/man7/EVP_PKEY-HMAC.pod

providers/common/provider_util.c

providers/implementations/keymgmt/mac_legacy_kmgmt.c

mattcaswell · 2020-08-19T16:36:45Z

All feedback addressed or responded to. Please take another look.

mattcaswell · 2020-08-20T14:18:59Z

I rebased this to resolve a trivial conflict with master. I also pushed a new fixup commit to address a minor issue identified by travis.

@paulidale - please can you reconfirm?

openssl-machine · 2020-08-20T23:00:13Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

paulidale

The merge conflict looks easy but probably qualifies as non-trivial and thus requiring review too.

test/evp_extra_test.c

Some MAC implementations were available before the current EVP_MAC API. They were used via EVP_DigestSign*. There exists a bridge between the old API and the EVP_MAC API however this bridge itself uses a legacy EVP_PKEY_METHOD. This commit implements the key management for provider side bridge without having to useany legacy code.

…idge Some MAC implementations were available before the current EVP_MAC API. They were used via EVP_DigestSign*. There exists a bridge between the oldAPI and the EVP_MAC API however this bridge itself uses a legacy EVP_PKEY_METHOD. This commit implements the signature functions for the provider side bridge without having to use any legacy code.

evp_extra_test had a test that checks whether an EVP_PKEY_CTX can still be created for HMAC even though there are no providers loaded because it is a legacy algorithm. However after the earlier commits this is no longer the case. We swap the check to a different legacy algorithm (SM2). Hopefully before too long there will be no legacy algorithms left and the test can be deleted.

Some MAC implementations were available before the current EVP_MAC API. They were used via EVP_DigestSign*. There exists a bridge between the old API and the EVP_MAC API however this bridge itself uses a legacy EVP_PKEY_METHOD. This commit implements the key management for provider side bridge without having to useany legacy code. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

…idge Some MAC implementations were available before the current EVP_MAC API. They were used via EVP_DigestSign*. There exists a bridge between the oldAPI and the EVP_MAC API however this bridge itself uses a legacy EVP_PKEY_METHOD. This commit implements the signature functions for the provider side bridge without having to use any legacy code. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

evp_extra_test had a test that checks whether an EVP_PKEY_CTX can still be created for HMAC even though there are no providers loaded because it is a legacy algorithm. However after the earlier commits this is no longer the case. We swap the check to a different legacy algorithm (SM2). Hopefully before too long there will be no legacy algorithms left and the test can be deleted. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

Previously it was a macro. We now make it into a function that is params aware. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

Fixes some issues with EVP_MD_CTX_* functions when doing EVP_DigestSign* and EVP_DigestVerify* functions. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

Convert various mac key creation function calls to use the _with_libctx variants. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

The previous commits added support for HMAC into the provider MAC bridge. We now extend that for SIPHASH too. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

Some signature algorithms don't need a default digest, so don't fail if we don't have one. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

The previous commits added support for HMAC and SIPHASH into the provider MAC bridge. We now extend that for Poly1305 too. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

The previous commits added support for HMAC, SIPHASH and Poly1305 into the provider MAC bridge. We now extend that for CMAC too. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

Now that the all the legacy PKEY MAC bridge code has been moved to the providers we no longer need the old bridge and it can be removed. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

The test only setup the generation of a key. It did not complete that generation. We extend the test to complete the generation and to use the generated key. We also compare the result with a key generated in a different way. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

We reuse concepts such as PROV_CIPHER, and make use of some common code in provider_util.c Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

mattcaswell added the branch: master Merge to master branch label Aug 12, 2020

paulidale reviewed Aug 12, 2020

View reviewed changes

crypto/evp/p_lib.c Show resolved Hide resolved

paulidale reviewed Aug 12, 2020

View reviewed changes

providers/implementations/keymgmt/mac_legacy_kmgmt.c Outdated Show resolved Hide resolved

paulidale reviewed Aug 12, 2020

View reviewed changes

providers/implementations/signature/mac_legacy.c Outdated Show resolved Hide resolved

paulidale reviewed Aug 12, 2020

View reviewed changes

slontis reviewed Aug 18, 2020

View reviewed changes

providers/implementations/keymgmt/mac_legacy_kmgmt.c Outdated

return gettable_params;

}

Copy link

Member

slontis Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra line

slontis reviewed Aug 18, 2020

View reviewed changes

providers/implementations/keymgmt/mac_legacy_kmgmt.c Show resolved Hide resolved

slontis reviewed Aug 18, 2020

View reviewed changes

providers/implementations/signature/mac_legacy.c Show resolved Hide resolved

paulidale reviewed Aug 18, 2020

View reviewed changes

paulidale approved these changes Aug 19, 2020

View reviewed changes

paulidale added the approval: done This pull request has the required number of approvals label Aug 19, 2020

paulidale assigned mattcaswell Aug 19, 2020

mattcaswell force-pushed the provider-pkey-mac-bridge branch from 9a99802 to 1a47629 Compare August 20, 2020 14:18

paulidale reviewed Aug 25, 2020

View reviewed changes

test/evp_extra_test.c Show resolved Hide resolved

mattcaswell added 4 commits August 27, 2020 12:08

Make the provider side EVP PKEY MAC bridge available in default and fips

758e134

swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020

Make the provider side EVP PKEY MAC bridge available in default and fips

fefb738

Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020

Document the EVP_PKEY_new_CMAC_key_with_libctx() function

926d2e9

Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020

Add some documentation about the EVP_PKEY MAC interface

476a428

Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020

Include "legacy" in the name of the various MAC bridge functions

9e1b6fb

Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#12637)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a provider side PKEY MAC bridge #12637

Implement a provider side PKEY MAC bridge #12637

mattcaswell commented Aug 12, 2020

paulidale left a comment •

edited

Loading

mattcaswell commented Aug 17, 2020

slontis Aug 18, 2020

levitte Aug 28, 2020

slontis Aug 18, 2020

slontis Aug 18, 2020

slontis Aug 18, 2020

slontis Aug 18, 2020

slontis Aug 18, 2020

slontis Aug 18, 2020

mattcaswell commented Aug 19, 2020

mattcaswell commented Aug 20, 2020

openssl-machine commented Aug 20, 2020

paulidale left a comment


		=head2 CMAC parameters

		As well as all the parameters described above, the B<CMAC> keytype additionally

Implement a provider side PKEY MAC bridge #12637

Implement a provider side PKEY MAC bridge #12637

Conversation

mattcaswell commented Aug 12, 2020

paulidale left a comment • edited Loading

Choose a reason for hiding this comment

mattcaswell commented Aug 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattcaswell commented Aug 19, 2020

mattcaswell commented Aug 20, 2020

openssl-machine commented Aug 20, 2020

paulidale left a comment

Choose a reason for hiding this comment

paulidale left a comment •

edited

Loading