-
Notifications
You must be signed in to change notification settings - Fork 65
Add support for HMAC #567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for HMAC #567
Conversation
simo5
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like good work, and I have mostly nits, however I got to ask why do you care to execute the HMAC function on the token, given you are not using really a token generated key, but rather importing a key that openssl has access to already.
|
Hey Simo, thanks for the quick review!
This is just to allow delegating the operation to a token that is a hardware crypto accelerator. But, I have some WIP changes to do TLS related operations entirely on the token, so this would be a prerequisite for those to work. Do you think such changes could be accepted, in future PRs? |
We are interested in that kind of functionality, yes, although it may require new functionality in OpenSSL as well. But we can collaborate in getting what's needed there, as we are also expanding on the EVP_SKEY stuff (which MAC code should really be able use once we add EVP_SKEY to MAC operations in OpenSSL) |
simo5
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more nits, but overall this looks good
|
Hey, I believe I went through all the comments. I see there are a couple of CI failures, maybe these are sporadic failures but I'm taking a look. Any other things which would I need to address before merging? |
I think the failures are related to to openssl or something. Please, rebase your changes on top of current |
simo5
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very close, just some minor nits.
tests/thmac
Outdated
| # Need to configure early loading, otherwise MACs might not be available | ||
| sed "s/#pkcs11-module-load-behavior/pkcs11-module-load-behavior = early/" \ | ||
| "${OPENSSL_CONF}" > "${OPENSSL_CONF}.early_load" | ||
| OPENSSL_CONF=${OPENSSL_CONF}.early_load |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Early loading seem odd to need, can you instead force all operations on the token? Otherwise there is no guarantee thes operations are done on the token during tests.
See other tests for the properties to set to force all ops on a token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forcing all ops on the token leads to a hang because of a seemingly non-recursive mutex used by SoftHSM to lock the token. Here's a backtrace:
* frame #0: 0x00007ffff7298f70 libc.so.6`__GI___lll_lock_wait [inlined] futex_wait(private=0, expected=2, futex_word=0x00005555556e89e0) at futex-internal.h:146:13
frame #1: 0x00007ffff7298f5a libc.so.6`__GI___lll_lock_wait(futex=0x00005555556e89e0, private=0) at lowlevellock.c:49:7
frame #2: 0x00007ffff72a0101 libc.so.6`___pthread_mutex_lock [inlined] lll_mutex_lock_optimized(mutex=0x00005555556e89e0) at pthread_mutex_lock.c:48:5
frame #3: 0x00007ffff72a00f8 libc.so.6`___pthread_mutex_lock(mutex=0x00005555556e89e0) at pthread_mutex_lock.c:93:7
frame #4: 0x00007ffff7571d59 libsofthsm2.so`OSLockMutex(mutex=0x00005555556e89e0) at osmutex.cpp:109:30
frame #5: 0x00007ffff75719dd libsofthsm2.so`MutexFactory::LockMutex(this=0x00005555556b6970, mutex=0x00005555556e89e0) at MutexFactory.cpp:183:26
frame #6: 0x00007ffff75715d1 libsofthsm2.so`Mutex::lock(this=0x00005555556e8710) at MutexFactory.cpp:61:50
frame #7: 0x00007ffff7571669 libsofthsm2.so`MutexLocker::MutexLocker(this=0x00007fffffffc620, inMutex=0x00005555556e8710) at MutexFactory.cpp:82:32
frame #8: 0x00007ffff75bf5b9 libsofthsm2.so`Token::isSOLoggedIn(this=0x00005555556e5300) at Token.cpp:95:29
frame #9: 0x00007ffff75b9c28 libsofthsm2.so`SessionManager::openSession(this=0x00005555556e8320, slot=0x00005555556e4d50, flags=4, pApplication=0x00005555556e7df0, notify=(pkcs11.so`token_session_callback at session.c:44:1), phSession=0x00005555556e7e08) at SessionManager.cpp:83:58
frame #10: 0x00007ffff7535af0 libsofthsm2.so`SoftHSM::C_OpenSession(this=0x00005555556d0c10, slotID=1146617904, flags=4, pApplication=0x00005555556e7df0, notify=(pkcs11.so`token_session_callback at session.c:44:1), phSession=0x00005555556e7e08) at SoftHSM.cpp:1414:40
frame #11: 0x00007ffff750e07a libsofthsm2.so`C_OpenSession(slotID=1146617904, flags=4, pApplication=0x00005555556e7df0, notify=(pkcs11.so`token_session_callback at session.c:44:1), phSession=0x00005555556e7e08) at main.cpp:317:37
frame #12: 0x00007ffff7dc2d69 pkcs11.so`p11prov_OpenSession(ctx=0x00005555556baa10, slotID=1146617904, flags=4, pApplication=0x00005555556e7df0, Notify=(pkcs11.so`token_session_callback at session.c:44:1), phSession=0x00005555556e7e08) at interface.gen.c:304:11
frame #13: 0x00007ffff7de78e3 pkcs11.so`token_session_open(session=0x00005555556e7df0, flags=4) at session.c:76:19
frame #14: 0x00007ffff7dea631 pkcs11.so`p11prov_get_session(provctx=0x00005555556baa10, slotid=0x00007fffffffc958, next_slotid=0x0000000000000000, uri=0x0000000000000000, mechtype=592, pw_cb=0x0000000000000000, pw_cbarg=0x0000000000000000, reqlogin=false, rw=false, _session=0x0000555555732470) at session.c:1023:19
frame #15: 0x00007ffff7db41ca pkcs11.so`p11prov_digest_init(ctx=0x0000555555732460, params=0x0000000000000000) at digests.c:315:9
frame #16: 0x00007ffff7846c9b libcrypto.so.3`evp_md_init_internal(ctx=0x0000555555728df0, type=0x0000555555731f00, params=0x0000000000000000, impl=0x0000000000000000) at digest.c:302:12
frame #17: 0x00007ffff7846f8f libcrypto.so.3`EVP_DigestInit_ex(ctx=0x0000555555728df0, type=0x00007ffff7c8f8a0, impl=0x0000000000000000) at digest.c:382:12
frame #18: 0x00007ffff75894ac libsofthsm2.so`OSSLEVPHashAlgorithm::hashInit(this=0x00005555556e8f10) at OSSLEVPHashAlgorithm.cpp:61:24
frame #19: 0x00007ffff7596e48 libsofthsm2.so`RFC4880::PBEDeriveKey(password=0x00007fffffffcda0, salt=0x00007fffffffcb70, ppKey=0x00007fffffffcb58) at RFC4880.cpp:74:21
frame #20: 0x00007ffff7597f7a libsofthsm2.so`SecureDataManager::login(this=0x00005555556e50e0, passphrase=0x00007fffffffcda0, encryptedKey=0x00005555556e50e8) at SecureDataManager.cpp:264:28
frame #21: 0x00007ffff75983f5 libsofthsm2.so`SecureDataManager::loginUser(this=0x00005555556e50e0, userPIN=0x00007fffffffcda0) at SecureDataManager.cpp:317:30
frame #22: 0x00007ffff75bfa28 libsofthsm2.so`Token::loginUser(this=0x00005555556e5300, pin=0x00007fffffffcda0) at Token.cpp:176:21
frame #23: 0x00007ffff7535f53 libsofthsm2.so`SoftHSM::C_Login(this=0x00005555556d0c10, hSession=1, userType=1, pPin="12345678", ulPinLen=8) at SoftHSM.cpp:1538:25
frame #24: 0x00007ffff750e328 libsofthsm2.so`C_Login(hSession=1, userType=1, pPin="12345678", ulPinLen=8) at main.cpp:407:31
frame #25: 0x00007ffff7dc39ce pkcs11.so`p11prov_Login(ctx=0x00005555556baa10, hSession=1, userType=1, pPin="12345678", ulPinLen=8) at interface.gen.c:451:11
frame #26: 0x00007ffff7de930a pkcs11.so`token_login(session=0x00005555556e7bb0, uri=0x0000000000000000, pw_cb=0x0000000000000000, pw_cbarg=0x0000000000000000, slot=0x0000555555712ec0, user_type=1) at session.c:565:11
frame #27: 0x00007ffff7de9f3b pkcs11.so`slot_login(slot=0x0000555555712ec0, uri=0x0000000000000000, pw_cb=0x0000000000000000, pw_cbarg=0x0000000000000000, reqlogin=true, _session=0x0000000000000000) at session.c:834:15
frame #28: 0x00007ffff7dea481 pkcs11.so`p11prov_get_session(provctx=0x00005555556baa10, slotid=0x00007fffffffd128, next_slotid=0x0000000000000000, uri=0x0000000000000000, mechtype=545, pw_cb=0x0000000000000000, pw_cbarg=0x0000000000000000, reqlogin=true, rw=false, _session=0x00005555556e6c90) at session.c:968:23
frame #29: 0x00007ffff7dc07cf pkcs11.so`import_mac_key(macctx=0x00005555556e6c80, key="00112233445566778899aabbccddeeff", keylen=32, keyobj=0x00005555556e6c88) at mac.c:122:15
frame #30: 0x00007ffff7dc0c3f pkcs11.so`p11prov_hmac_set_ctx_params(ctx=0x00005555556e6c80, params=0x00005555556e7d40) at mac.c:204:14
frame #31: 0x00007ffff787c0ff libcrypto.so.3`EVP_MAC_CTX_set_params(ctx=0x00005555556ce2a0, params=0x00005555556e7d40) at mac_lib.c:220:16
frame #32: 0x00005555555d7457 openssl`mac_main(argc=12, argv=0x00007fffffffd528) at mac.c:162:14
frame #33: 0x00005555555dbe4a openssl`do_cmd(prog=0x00005555556b8860, argc=12, argv=0x00007fffffffd4d0) at openssl.c:428:16
frame #34: 0x00005555555db9d2 openssl`main(argc=12, argv=0x00007fffffffd4d0) at openssl.c:309:19
frame #35: 0x00007ffff722a1ca libc.so.6`__libc_start_call_main(main=(openssl`main at openssl.c:238:1), argc=13, argv=0x00007fffffffd4c8) at libc_start_call_main.h:58:16
frame #36: 0x00007ffff722a28b libc.so.6`__libc_start_main_impl(main=(openssl`main at openssl.c:238:1), argc=13, argv=0x00007fffffffd4c8, init=<unavailable>, fini=<unavailable>, rtld_fini=<unavailable>, stack_end=0x00007fffffffd4b8) at libc-start.c:360:3
frame #37: 0x00005555555a21a5 openssl`_start + 37
The mutex is locked during Token::loginUser() (frame #22) and is still locked when Token::isSOLoggedIn() is called (frame #8).
I got the test working without early loading or forcing all operations on the token, it was just a matter of initializing pkcs11 a bit earlier and adding HMAC to static_operations_init().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
softhsm is hopelessly broken like that in many places, feel free to exclude the test on softhsm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that it is likely you will never be able to use softhsm with this feature, if that is your target token you may want to consider switching to use kryoptic as token instead (there is a utility to export existing softhsm databases to its format too).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main target is NXP's SMW library (https://github.com/nxp-imx/imx-smw) which exposes a PKCS11 interface to a secure enclave that can do TLS derivation without exporting the key material, thus the need for the HMAC operation as in this PR.
If I do all operations on the token in the test as you mentioned, wouldn't both openssl mac commands do exactly the same thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you say "both" what do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test script, thmac, runs openssl mac 1) with and 2) without -propquuery ?provider=pkcs11, then compares the outputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the results should always be the same, certainly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and I saw in other tests that running all operations on the token implies setting
default_properties = ?provider=pkcs11
in openssl.cnf.
In this case, both openssl mac command would invoke the pkcs11 provider implementation. Is this right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Early loading seem odd to need, can you instead force all operations on the token?
I may have misunderstood what you meant by this, could you take a look at the last commit(s) in this series of fixups? I'm not sure I went the right way with my changes. Many thanks!
|
Can you rebase on top of the current master? Rebuild should fix the centos10 issues. Then we are left with the softhsm issues. |
Signed-off-by: Ilie Halip <[email protected]>
Removed MD* & RIPEMD* digests support. Signed-off-by: Ilie Halip <[email protected]>
Removed p11prov_mac_ctx.hash_mech. Renamed p11prov_mac_ctx.mech to .hmac. Added hash_mech() func. Signed-off-by: Ilie Halip <[email protected]>
Add a new p11prov_create_mac_key() func instead of using the existing p11prov_create_secret_key(). Signed-off-by: Ilie Halip <[email protected]>
clang-tidy fixes. Signed-off-by: Ilie Halip <[email protected]>
Fixed shellcheck warning (SC2206). Removed BLAKE2S* digests, there's no equivalent in PKCS11. Signed-off-by: Ilie Halip <[email protected]>
inner_mac_key -> import_mac_key. Replace hmac_mechanisms array with digest_to_hmac(). Initialize hmac = CK_UNAVAILABLE_INFORMATION. Signed-off-by: Ilie Halip <[email protected]>
Seems clang-format doesn't always give the same result, use the format suggested in CI. While here, remove an unused macro. Signed-off-by: Ilie Halip <[email protected]>
Don't hardcode the template size. Since the template array is not modified, just declare it with no explicit size, then use sizeof(). Signed-off-by: Ilie Halip <[email protected]>
Add a comma at the end for a little saner output and smaller diffs when adding new mechansisms. Signed-off-by: Ilie Halip <[email protected]>
Added HMAC mechanisms to digests mapping array. Reworked digest_to_hmac and digest_from_hmac functions. Signed-off-by: Ilie Halip <[email protected]>
Remove CKA_PRIVATE and add CKA_VERIFY. Signed-off-by: Ilie Halip <[email protected]>
Use a login session to import the mac key. Signed-off-by: Ilie Halip <[email protected]>
Make HMAC work without early loading: - add it to static_operations_init() - initialize pkcs11 (via p11prov_ctx_status()) earlier, because by the time set_ctx_params() is called, the pkcs11 library should be already initialized Remove the early loading part from the test script. Signed-off-by: Ilie Halip <[email protected]>
Removed SoftHSM from supported suites list. During test, force all ops on token. Signed-off-by: Ilie Halip <[email protected]>
8206f9a to
03a3f0e
Compare
|
I'm going to close this PR as it's become stale. I will open a new one after rebasing/squashing and a couple more updates. |
Description
In the context of TLS 1.3, HMAC is used to calculate the
verify_datawith the Finished key. Right now, OpenSSL falls back to it's own internal HMAC.Add an HMAC implementation that calls into the PKCS11 library to do the HMAC calculation. TLS1.3 does not truncate the HMAC length, so use
CKM_XXX_HMACmechanisms without an explicit length.Add tests that compare the output from PKCS11 to the output of OpenSSL. Since PKCS11 libs may not have implementations for all the mechanisms in the specification, define a subset of "mandatory" HMACs that should be supported by all the test suites.
In the process, I had to add 2 new attributes for the generic secret key:
CKA_SIGN: needed to be able to callC_SignInitwith the keyCKA_PRIVATE:softhsmseems to require this attribute for everything to work fine - I guess this could be removed, but the tests would have to be disabled for softhsmChecklist
Reviewer's checklist: