Move HKDF to OpenSSL 3 interfaces #8909

jeredfloyd · 2022-06-14T04:49:23Z

This patch does a few related things to get building successfully with OpenSSL 3.

Adds a new conditional compilation identifier OPENSSL_IS_OPENSSL3 (parallel to OPENSSL_IS_BORINGSSL). I considered using OPENSSL_VERSION_NUMBER instead, but there seems to be precedent for this model -- happy to change.
Modifies tscore/HKDF to accept a char* digest name instead of an EVP_MD* object for compatibility with the new EVP_KDF interface, and updates the other HKDF implementations and HKDF clients.
Provides an EVP_KDF implementation of the HKDF class. EVP_PKEY isn't deprecated yet, but it's slower and currently has a blocking bug.
Moves experimental/access_control/utils to the new EVP_MAC interface; the old HMAC interface is deprecated, and removes test cases using deprecated hashes (MD4, RIPEMD160).

This makes some progress on #7341 .

jeredfloyd · 2022-06-14T05:13:27Z

Oops, looks like I missed some references in QUICTLS.cc. (Not sure why this didn't cause my local builds to fail.) I'll fix these tomorrow.

maskit · 2022-06-14T06:10:09Z

Thank you for taking care of the test failure in test_HKDF and also making this progress.

Adds a new conditional compilation identifier OPENSSL_IS_OPENSSL3 (parallel to OPENSSL_IS_BORINGSSL). I considered using OPENSSL_VERSION_NUMBER instead, but there seems to be precedent for this model -- happy to change.

I'm fine with either way. It seems like OPENSSL_VERSION_NUMBER was un-deprecated at some point.

Modifies tscore/HKDF to accept a char* digest name instead of an EVP_MD* object for compatibility with the new EVP_KDF interface, and updates the other HKDF implementations and HKDF clients.

+1. I'd add HKDF_openssl3.cc since almost all the code in HKDF_openssl.cc is ifdef-ed and we already have HKDF_boringssl.cc, but that's just my preference and not a blocker at all.

Provides an EVP_KDF implementation of the HKDF class. EVP_PKEY isn't deprecated yet, but it's slower and currently has a blocking bug.

Sounds reasonable.

Moves experimental/access_control/utils to the new EVP_MAC interface; the old HMAC interface is deprecated, and removes test cases using deprecated hashes (MD4, RIPEMD160).w

Did you see any performance difference between EVP_MAC interface and HMAC interface? I made a similar change on #7447 and decided to use the old interface as long as it's available (even if it's deprecated) because the new interface was slow. According to #8615 the performance issue may be resolved on the latest version, but it'd be great if you could confirm there's no (big) difference between the new and old interfaces. Calling EVP_MAC_CTX_new/free is concerning in terms of memory allocation.

bneradt · 2022-06-14T15:58:14Z

In case it's helpful, here's the Fedora build results:
https://ci.trafficserver.apache.org/job/Github_Builds/job/fedora/1053/console

QUICTLS.cc: In member function 'void QUICTLS::update_key_materials_for_read(QUICEncryptionLevel, const uint8_t*, size_t)':
QUICTLS.cc:264:46: error: no matching function for call to 'QUICHKDF::QUICHKDF(const EVP_MD*)'
  264 |   QUICHKDF hkdf(this->_get_handshake_digest());
      |                                              ^
In file included from QUICKeyGenerator.h:27,
                 from QUICHandshakeProtocol.h:26,
                 from QUICTLS.h:37,
                 from QUICTLS.cc:24:
QUICHKDF.h:31:3: note: candidate: 'QUICHKDF::QUICHKDF(const char*)'
   31 |   QUICHKDF(const char *digest) : HKDF(digest) {}
      |   ^~~~~~~~
QUICHKDF.h:31:24: note:   no known conversion for argument 1 from 'const EVP_MD*' {aka 'const evp_md_st*'} to 'const char*'
   31 |   QUICHKDF(const char *digest) : HKDF(digest) {}
      |            ~~~~~~~~~~~~^~~~~~
QUICHKDF.h:28:7: note: candidate: 'constexpr QUICHKDF::QUICHKDF(const QUICHKDF&)'
   28 | class QUICHKDF : public HKDF
      |       ^~~~~~~~
QUICHKDF.h:28:7: note:   no known conversion for argument 1 from 'const EVP_MD*' {aka 'const evp_md_st*'} to 'const QUICHKDF&'
QUICHKDF.h:28:7: note: candidate: 'constexpr QUICHKDF::QUICHKDF(QUICHKDF&&)'
QUICHKDF.h:28:7: note:   no known conversion for argument 1 from 'const EVP_MD*' {aka 'const evp_md_st*'} to 'QUICHKDF&&'
QUICTLS.cc: In member function 'void QUICTLS::update_key_materials_for_write(QUICEncryptionLevel, const uint8_t*, size_t)':
QUICTLS.cc:322:46: error: no matching function for call to 'QUICHKDF::QUICHKDF(const EVP_MD*)'
  322 |   QUICHKDF hkdf(this->_get_handshake_digest());
      |                                              ^
QUICHKDF.h:31:3: note: candidate: 'QUICHKDF::QUICHKDF(const char*)'
   31 |   QUICHKDF(const char *digest) : HKDF(digest) {}
      |   ^~~~~~~~
QUICHKDF.h:31:24: note:   no known conversion for argument 1 from 'const EVP_MD*' {aka 'const evp_md_st*'} to 'const char*'
   31 |   QUICHKDF(const char *digest) : HKDF(digest) {}
      |            ~~~~~~~~~~~~^~~~~~
QUICHKDF.h:28:7: note: candidate: 'constexpr QUICHKDF::QUICHKDF(const QUICHKDF&)'
   28 | class QUICHKDF : public HKDF
      |       ^~~~~~~~
QUICHKDF.h:28:7: note:   no known conversion for argument 1 from 'const EVP_MD*' {aka 'const evp_md_st*'} to 'const QUICHKDF&'
QUICHKDF.h:28:7: note: candidate: 'constexpr QUICHKDF::QUICHKDF(QUICHKDF&&)'
QUICHKDF.h:28:7: note:   no known conversion for argument 1 from 'const EVP_MD*' {aka 'const evp_md_st*'} to 'QUICHKDF&&'
make[3]: *** [Makefile:1903: QUICTLS.o] Error 1
make[3]: *** Waiting for unfinished jobs....
QUICKeyGenerator.cc: In member function 'void QUICKeyGenerator::generate(QUICVersion, uint8_t*, uint8_t*, uint8_t*, size_t*, QUICConnectionId)':
QUICKeyGenerator.cc:60:90: error: cannot convert 'const char*' to 'const EVP_MD*' {aka 'const evp_md_st*'}
   60 |                                    LABEL_FOR_CLIENT_INITIAL_SECRET.length(), EVP_MD_size(md));
      |                                                                                          ^~
      |                                                                                          |
      |                                                                                          const char*
In file included from QUICKeyGenerator.h:25,
                 from QUICKeyGenerator.cc:24:
/opt/openssl-quic/include/openssl/evp.h:447:31: note:   initializing argument 1 of 'int EVP_MD_size(const EVP_MD*)'
  447 | int EVP_MD_size(const EVP_MD *md);
      |                 ~~~~~~~~~~~~~~^~
QUICKeyGenerator.cc:70:90: error: cannot convert 'const char*' to 'const EVP_MD*' {aka 'const evp_md_st*'}
   70 |                                    LABEL_FOR_SERVER_INITIAL_SECRET.length(), EVP_MD_size(md));
      |                                                                                          ^~
      |                                                                                          |
      |                                                                                          const char*
/opt/openssl-quic/include/openssl/evp.h:447:31: note:   initializing argument 1 of 'int EVP_MD_size(const EVP_MD*)'
  447 | int EVP_MD_size(const EVP_MD *md);
      |                 ~~~~~~~~~~~~~~^~
make[3]: *** [Makefile:1903: QUICKeyGenerator.o] Error 1
make[3]: Leaving directory '/home/jenkins/workspace/Github_Builds/fedora/src/iocore/net/quic'
make[2]: *** [Makefile:1350: all-recursive] Error 1
make[2]: Leaving directory '/home/jenkins/workspace/Github_Builds/fedora/src/iocore/net'
make[1]: *** [Makefile:607: all-recursive] Error 1
make[1]: Leaving directory '/home/jenkins/workspace/Github_Builds/fedora/src/iocore'
make: *** [Makefile:883: all-recursive] Error 1

The Rocky (our CentOS replacement for CI) seems to be failing for the same reason:
https://ci.trafficserver.apache.org/job/Github_Builds/job/rocky/439/console

jeredfloyd · 2022-06-14T16:13:36Z

Yeah, I've already fixed it; working on the other suggested changes now.

… missing QUIC changes

jeredfloyd · 2022-06-14T16:40:11Z

I've reverted the HMAC changes since I have no reason to believe the EVP interface is faster. We can shelf this until the deprecated interfaces go away.

I moved the HKDF openssl3 implementation into a separate file as suggested.

Switched to using the OPENSSL_VERSION_NUMBER symbol.

Watching for build results now...

…MPAT symbol

jeredfloyd · 2022-06-14T20:22:04Z

Sorry about the flapping there; for some reason my test environment is not running all the QUIC tests. Everything is passing now.

maskit · 2022-06-14T23:33:02Z

include/tscore/HKDF.h

 protected:
-  const EVP_MD *_digest = nullptr;
-  EVP_PKEY_CTX *_pctx   = nullptr;
+  const char *_digest = nullptr;


It seems like only the constructor needs a digest name. Maybe we can remove this member variable.

maskit · 2022-06-14T23:34:32Z

Overall looks good. Just a minor comment on the unnecessary member variable.

jeredfloyd · 2022-06-15T14:04:04Z

Good catch; removed the unnecessary member variable.

traeak · 2022-07-08T14:10:58Z

cherry pick to 9.2.x needed for fedora 36

zwoop · 2022-07-08T17:42:24Z

Cherry-picked to v9.2.x

* Move HKDF and HMACs to openssl 3 interfaces * clang-format changed files * Revert HMAC change, move EVP_KDF to separate implementation file, add missing QUIC changes * switch back to OPENSSL_IS_OPENSSL3 symbol due to conflict with API_COMPAT symbol * Get digest size from name * Remove extraneous HKDF member variable '_digest' (cherry picked from commit 8ca74ee)

bryancall · 2022-10-14T15:33:28Z

Cherry picked to 9.1.x

* Move HKDF and HMACs to openssl 3 interfaces * clang-format changed files * Revert HMAC change, move EVP_KDF to separate implementation file, add missing QUIC changes * switch back to OPENSSL_IS_OPENSSL3 symbol due to conflict with API_COMPAT symbol * Get digest size from name * Remove extraneous HKDF member variable '_digest' (cherry picked from commit 8ca74ee)

jeredfloyd added 2 commits June 14, 2022 04:37

Move HKDF and HMACs to openssl 3 interfaces

5e4aaf9

clang-format changed files

fc7430b

Revert HMAC change, move EVP_KDF to separate implementation file, add…

d2fb562

… missing QUIC changes

jeredfloyd added 2 commits June 14, 2022 16:54

switch back to OPENSSL_IS_OPENSSL3 symbol due to conflict with API_CO…

3577065

…MPAT symbol

Get digest size from name

8f74c42

jeredfloyd changed the title ~~Move HKDF and HMACs to OpenSSL 3 interfaces~~ Move HKDF to OpenSSL 3 interfaces Jun 14, 2022

maskit reviewed Jun 14, 2022

View reviewed changes

Remove extraneous HKDF member variable '_digest'

aa66176

maskit approved these changes Jun 15, 2022

View reviewed changes

maskit merged commit 8ca74ee into apache:master Jun 15, 2022

maskit assigned jeredfloyd Jun 15, 2022

maskit added the QUIC label Jun 15, 2022

maskit added this to the 10.0.0 milestone Jun 15, 2022

maskit added the Core label Jun 15, 2022

jeredfloyd deleted the openssl3-hkdf branch June 16, 2022 03:48

zwoop modified the milestones: 10.0.0, 9.2.0 Jul 8, 2022

bryancall removed this from the 9.2.0 milestone Oct 14, 2022

bryancall added this to the 9.1.4 milestone Oct 14, 2022

Move HKDF to OpenSSL 3 interfaces #8909

Move HKDF to OpenSSL 3 interfaces #8909

Uh oh!

Conversation

jeredfloyd commented Jun 14, 2022

Uh oh!

jeredfloyd commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maskit commented Jun 14, 2022

Uh oh!

bneradt commented Jun 14, 2022

Uh oh!

jeredfloyd commented Jun 14, 2022

Uh oh!

jeredfloyd commented Jun 14, 2022

Uh oh!

jeredfloyd commented Jun 14, 2022

Uh oh!

maskit Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

maskit commented Jun 14, 2022

Uh oh!

jeredfloyd commented Jun 15, 2022

Uh oh!

traeak commented Jul 8, 2022

Uh oh!

zwoop commented Jul 8, 2022

Uh oh!

bryancall commented Oct 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jeredfloyd commented Jun 14, 2022 •

edited

Loading