Introduce keypair generation as engine ctrl command #474

istepic · 2022-09-08T13:32:53Z

As discussed in #379 and
#378 we need a generic interface
that supports multiple algorithms for key generation. Attempt was made
to create a new keygen method and register it in PKCS11_pkey_meths() in
p11_pkey.c (so that it's possible to generate keys using OpenSSL's
EVP_PKEY_* API) but multiple design issues appeared. How and where do you
pass the key ID, token label and alike was the first question. As
suggested by the maintainer here:
#379 (comment),
app_data from EVP_PKEY_CTX was (mis)used and that worked well. The
reason why this approach was abandoned is because a good (or bad) way
to get a handle of the PKCS11_CTX_private, that is necessary for the
Cryptoki call, was not found.
The way other operations work is that they rely on the key being
loaded first through ENGINE_load_public(private)_key because this
is when the PKCS11_CTX gets initialized and a handle to
PKCS11_OBJECT_private gets set to the ex_data of the underlying key.
Key generation obviously cannot rely on that mechanism since key
doesn't yet exist.

Instead, a generic PKCS11_generate_key interface was made that
takes a structure describing the key generation algorithm. For now
it only contains simple options like curve name for ECC or number
of bits for RSA key generation. This interface can then be used
as any other PKCS11 wrapper interface or using the ENGINE control
commands. Using it with ENGINE control commands is demonstrated in
the new tests/keygen.c file.

Code for ECC keygen was taken from:
#379 and reworked to compile and
work with some new additions to libp11 i.e. templates.

istepic · 2022-10-05T07:34:10Z

Hello @mtrojnar, does this approach to key generation seem plausible at all to you?

ldts · 2022-11-02T13:59:52Z

what is the status of this PR? has it been abandoned as well? It seems there was a previous attempt at implementing EC keygen which didn't prosper either (#379).

is there are reason not to pursue either of them?

dengert · 2022-11-03T01:01:08Z

what is the status of this PR? has it been abandoned as well? It seems there was a previous attempt at implementing EC keygen which didn't prosper either (#379).

is there are reason not to pursue either of them?

IMHO biggest issue with both of these is when will OpenSSL drops engine support and what happens to libp11.

This patch uses experimental change in libp11 to generate EC keypairs instead of RSA:2048: OpenSC/libp11#474 The patches from this PR need to be built with libp11 for lmp-device-register to compile. Signed-off-by: Milosz Wasilewski <[email protected]>

mwasilew · 2022-11-10T21:31:45Z

Is there an alternative way of generating EC key pairs on the token? It is currently possible to generate EC keys with pkcs11-tool, but not libp11 API.

istepic · 2022-11-14T17:46:39Z

Is there an alternative way of generating EC key pairs on the token? It is currently possible to generate EC keys with pkcs11-tool, but not libp11 API.

No alternative way for EC keys.

Patch: OpenSC/libp11#474

Patch: OpenSC/libp11#474 Rework docker setup to allow for adding files to the image. This is not possible when using " - < " and reading from STDIN because there is no Docker Build Context there which is necessary for ADD and COPY commands.

- Patch: OpenSC/libp11#474 - Reworked docker setup to allow adding files to the image. This is not possible when using " - < " and reading from STDIN because there is no Docker Build Context there which is necessary for ADD and COPY commands. - Built libp11 the same way upstream builds it in their CI. This requires libtool package.

mtrojnar · 2024-12-02T20:26:41Z

The engine interface looks good to me.

On the other hand, changing the definition of PKCS11_generate_key() is unacceptable, because it breaks each and every existing function that uses that API function. You need to define a new API function instead (e.g., PKCS11_keygen()) and reimplement the existing PKCS11_generate_key() API function as a wrapper for the new PKCS11_keygen().

rafajunio · 2025-02-09T16:08:42Z

Hello @mtrojnar , can you please check if the PR is good for you now?

I requested Ivan access for change the patch.

I do not known what to do with the hex_to_bin in the p11_front.c, but I do not want to introduce even more changes to the PR.

mtrojnar · 2025-02-10T20:43:09Z

@rafajunio

The API looks better now. I think it should be possible to merge this feature.

@olszomal and myself are in the middle of implementing PKCS#11 provider. More internal changes in PIN UI will be implemented soon. Please wait a few weeks for the code changes to stabilize, and then I'll ask you to rebase your PR on top of the new code. What do you think?

mtrojnar · 2025-03-17T20:22:18Z

@rafajunio
The provider is merged and eng_back.c/util_uri.c are stable enough for merging your keypair generation code. If possible, try to remove any non-essential changes from the next version of your PR to simplify the review.

PR474 was now rebased on top of libp11-0.4.13 OpenSC/libp11#474 | * 98ebc16 (istepic/master, istepic/HEAD) Fix typo | * 79a7937 Fix build for ubuntu | * e837a0e update to the latest version of libp11 |/ * 7addb3c Cleanup * 958c8e8 Fix debug_level for UTIL_CTX_log() * 1a780a0 Make UTIL_CTX opaque * 761ef67 Further util_uri.c separation * 3b80c73 Remove dead code * 3c7b2cc Fix Windows build * 6dc51cf Initial util_uri.c separation * dba1add Reorder CMD_DEBUG_LEVEL processing to match definition order * 4852910 Fix from CKK_GENERIC_SECRET keys * 7300cb8 Add configure option to enable the static PKCS11 engine * 5be6422 Formatting * 22832d3 Simplify implementation of the QUIET control command * 2c747ad Restore the VERBOSE engine control command * 50b1363 Enhance libp11 logging system (#573) * e177b3b Starting 0.4.14_git * 6d66918 (HEAD -> master, tag: libp11-0.4.13, tag: devtool-patched, tag: devtool-base, devtool) Fixed tests in distribution tarball Signed-off-by: Jose Quaresma <[email protected]>

As discussed in OpenSC#379 and OpenSC#378 we need a generic interface that supports multiple algorithms for key generation. Attempt was made to create a new keygen method and register it in PKCS11_pkey_meths() in p11_pkey.c (so that it's possible to generate keys using OpenSSL's EVP_PKEY_* API) but multiple design issues appeared. How and where do you pass the key ID, token label and alike was the first question. As suggested by the maintainer here: OpenSC#379 (comment), app_data from EVP_PKEY_CTX was (mis)used and that worked well. The reason why this approach was abandoned is because a good (or bad) way to get a handle of the PKCS11_CTX_private, that is necessary for the Cryptoki call, was not found. The way other operations work is that they rely on the key being loaded *_first_* through ENGINE_load_public(private)_key because this is when the PKCS11_CTX gets initialized and a handle to PKCS11_OBJECT_private gets set to the ex_data of the underlying key. Key generation obviously cannot rely on that mechanism since key doesn't yet exist. Instead, a generic PKCS11_generate_key interface was made that takes a structure describing the key generation algorithm. For now it only contains simple options like curve name for ECC or number of bits for RSA key generation. It also possible to configure CKA_SENSITIVE and CKA_EXTRACTABLE key attributes. This interface can then be used as any other PKCS11 wrapper interface or using the ENGINE control commands. Using it with ENGINE control commands is demonstrated in the new tests/keygen.c file. Code for ECC keygen was taken from: OpenSC#379 and reworked to compile and work with some new additions to libp11 i.e. templates. Upstream-Status: Denied [OpenSC#474] Signed-off-by: istepic <[email protected]> Signed-off-by: cps-b <[email protected]>

rafajunio · 2025-03-26T23:37:46Z

@mtrojnar
I tried to refactor as much as possible, I also tried to create the provider test already but I could not. Either way I upload the code for you start the review, if you have some hint please let me known

olszomal

Excellent work – the code is clean and well-structured, making it easy to review.

I confirm that key pair generation works correctly for both the libp11 library and the engine.

Unfortunately, the OpenSSL provider currently only supports retrieving objects from the token, meaning it cannot be used for key generation. Please remove the key generation test for the provider.

olszomal · 2025-04-02T11:59:46Z

tests/keygen.softhsm

+	exit 1
+fi
+
+echo "Checking pkcs11-tool result..."


Restore LD_LIBRARY_PATH before calling list_objects.

olszomal · 2025-04-02T12:00:30Z

tests/keygen.c

+cleanup:
+	if (!engine)
+		ENGINE_finish(engine);
+


Remember to call EVP_PKEY_free()

olszomal · 2025-04-02T12:01:06Z

tests/keygen.c

+		display_openssl_errors(__LINE__);
+		goto cleanup;
+	}
+


ENGINE_init() returns a functional reference, so release the structural reference obtained from ENGINE_by_id() and use ENGINE_free(engine).

olszomal · 2025-04-02T12:03:55Z

tests/keygen.c

+	/*
+	 * EC key generation test
+	 */
+	PKCS11_EC_KGEN ec = {


Declare structures and variables at the beginning of the code block, following the C89/C90 standard.

olszomal · 2025-04-02T12:04:40Z

tests/keygen.c

+	printf("ECC Sign-verify success\n");
+
+	/*
+	 * RSA key generation test


Split the test into two separate cases for RSA and EC key generation to maintain consistency with existing tests.

olszomal · 2025-04-02T12:07:13Z

src/eng_back.c

+		}
+	}
+
+	// Try with logging in


This project uses C-style comments instead of C++.

olszomal · 2025-04-02T12:10:52Z

src/eng_back.c

+	if (!found_slot)
+		return 0;
+
+	/* If login is not forced, try to generate key without logging in first.


Remove the attempt to generate a key without logging in.
If token login is required, UTIL_CTX_login() must be called, as a private object cannot be created unless the session is logged in as a normal user (CKR_USER_NOT_LOGGED_IN error).

olszomal · 2025-04-02T12:11:29Z

src/util.h

 void UTIL_CTX_set_vlog_a(UTIL_CTX *ctx, PKCS11_VLOG_A_CB vlog);
 void UTIL_CTX_set_debug_level(UTIL_CTX *ctx, int debug_level);
 void UTIL_CTX_log(UTIL_CTX *ctx, int level, const char *format, ...);
+int UTIL_CTX_hex_to_bin(UTIL_CTX *ctx, const char *in, char *out, size_t *outlen);


Remove the unused function UTIL_CTX_hex_to_bin().

olszomal · 2025-04-02T12:14:45Z

tests/keygen-prov.c

+	char *key_pass = argv[3];
+
+	/* Load pkcs11prov and default providers */
+	if (!providers_load()) {


Unfortunately, the OpenSSL provider currently only supports retrieving objects from the token, meaning it cannot be used for key generation.
Please remove the key generation test for the provider.

olszomal · 2025-04-02T12:19:19Z

src/util.h


 int UTIL_CTX_set_pin(UTIL_CTX *ctx, const char *pin);
 void UTIL_CTX_set_force_login(UTIL_CTX *ctx, int force_login);
+int UTIL_CTX_get_force_login(UTIL_CTX *ctx);


Remove the unused function UTIL_CTX_get_force_login().

olszomal

Generally well done! Some minor style tweaks, but also a significant issue that needs attention.

olszomal · 2025-04-03T12:03:06Z

src/eng_back.c

+
+	/* Try with logging in */
+	ERR_clear_error();
+	if (found_slot->token->loginRequired && UTIL_CTX_login(ctx->util_ctx,


If token login is required, UTIL_CTX_login() must be called. However, try to generate a key pair.

olszomal · 2025-04-03T12:06:24Z

src/p11_key.c

+	curve_obj = OBJ_nid2obj(curve_nid);
+	if (!curve_obj)
+		return -1;
+	// convert to DER format and take just the length


Use C-style comments instead of C++. This note applies to all comments; sorry for the confusion.

olszomal · 2025-04-03T12:11:52Z

tests/Makefile.am

 rsa_oaep_prov_SOURCES = rsa-oaep-prov.c helpers_prov.c
 store_cert_prov_SOURCES = store-cert-prov.c helpers_prov.c
 check_all_prov_SOURCES = check-all-prov.c helpers_prov.c
+keygen_prov_SOURCES = keygen-prov.c helpers_prov.c


olszomal · 2025-04-03T12:14:48Z

tests/ec-keygen.c

+		printf("Could not generate ECC keys\n");
+		goto cleanup;
+	}
+	printf("ECC keys generated\n");
+
+	ecpb = ENGINE_load_public_key(engine, "2233", NULL, NULL);
+	ecpr = ENGINE_load_private_key(engine, "2233", NULL, NULL);
+	if ((ret = sign_verify_test(ecpr, ecpb)) < 0) {
+		printf("ECC Sign-verify failed with err code: %d\n", ret);
+		goto cleanup;
+	}
+	printf("ECC Sign-verify success\n");


Formally, it should be EC keys instead of ECC keys, as EC refers specifically to Elliptic Curve keys, while ECC refers to the broader field of Elliptic Curve Cryptography.

olszomal · 2025-04-03T12:23:41Z

src/libp11-int.h

-	char *label, unsigned char *id, size_t id_len);
+extern int pkcs11_rsa_keygen(PKCS11_SLOT_private *tpriv,
+	unsigned int bits, const char *label, const unsigned char* id,
+	size_t id_len, const PKCS11_params* params);


Please use TYPE *ptr instead of TYPE* ptr to follow the project's convention.

olszomal · 2025-04-04T07:30:06Z

tests/ec-keygen.c

+
+	ret = 0;
+cleanup:
+	if (!engine)


This condition is incorrect.
ENGINE_finish(engine) should be called unconditionally to release the functional reference obtained via ENGINE_init(). Please remove the if statement and call ENGINE_finish(engine); directly. Skipping it when engine == NULL is safe, as the function handles that internally.

olszomal · 2025-04-04T07:30:46Z

tests/rsa-keygen.c

+
+	ret = 0;
+cleanup:
+	if (!engine)


The condition is incorrect.

As discussed in OpenSC#379 and OpenSC#378 we need a generic interface that supports multiple algorithms for key generation. Attempt was made to create a new keygen method and register it in PKCS11_pkey_meths() in p11_pkey.c (so that it's possible to generate keys using OpenSSL's EVP_PKEY_* API) but multiple design issues appeared. How and where do you pass the key ID, token label and alike was the first question. As suggested by the maintainer here: OpenSC#379 (comment), app_data from EVP_PKEY_CTX was (mis)used and that worked well. The reason why this approach was abandoned is because a good (or bad) way to get a handle of the PKCS11_CTX_private, that is necessary for the Cryptoki call, was not found. The way other operations work is that they rely on the key being loaded *_first_* through ENGINE_load_public(private)_key because this is when the PKCS11_CTX gets initialized and a handle to PKCS11_OBJECT_private gets set to the ex_data of the underlying key. Key generation obviously cannot rely on that mechanism since key doesn't yet exist. Instead, a generic PKCS11_generate_key interface was made that takes a structure describing the key generation algorithm. For now it only contains simple options like curve name for ECC or number of bits for RSA key generation. It also possible to configure CKA_SENSITIVE and CKA_EXTRACTABLE key attributes. This interface can then be used as any other PKCS11 wrapper interface or using the ENGINE control commands. Using it with ENGINE control commands is demonstrated in the new tests/keygen.c file. Code for ECC keygen was taken from: OpenSC#379 and reworked to compile and work with some new additions to libp11 i.e. templates.

olszomal

Looks good — thanks for the changes!
I’ll take care of the small style issue separately to keep things moving.

rafajunio · 2025-04-04T10:17:23Z

Please, if possible check all the source code there are a lot of identation using space instead of tab. I recomend use checkpatch from kernel to update the style.

Really thank you =)

As discussed in OpenSC#379 and OpenSC#378 we need a generic interface that supports multiple algorithms for key generation. Attempt was made to create a new keygen method and register it in PKCS11_pkey_meths() in p11_pkey.c (so that it's possible to generate keys using OpenSSL's EVP_PKEY_* API) but multiple design issues appeared. How and where do you pass the key ID, token label and alike was the first question. As suggested by the maintainer here: OpenSC#379 (comment), app_data from EVP_PKEY_CTX was (mis)used and that worked well. The reason why this approach was abandoned is because a good (or bad) way to get a handle of the PKCS11_CTX_private, that is necessary for the Cryptoki call, was not found. The way other operations work is that they rely on the key being loaded *_first_* through ENGINE_load_public(private)_key because this is when the PKCS11_CTX gets initialized and a handle to PKCS11_OBJECT_private gets set to the ex_data of the underlying key. Key generation obviously cannot rely on that mechanism since key doesn't yet exist. Instead, a generic PKCS11_generate_key interface was made that takes a structure describing the key generation algorithm. For now it only contains simple options like curve name for ECC or number of bits for RSA key generation. This interface can then be used as any other PKCS11 wrapper interface or using the ENGINE control commands. Using it with ENGINE control commands is demonstrated in the new tests/keygen.c file. Code for ECC keygen was taken from: OpenSC#379 and reworked to compile and work with some new additions to libp11 i.e. templates. Upstream-Status: Submitted [OpenSC#474] Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>

Upstream-Status: Submitted [OpenSC#474] Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>

These changes depend on EC support being merged [ie, OpenSC#474] Until then, this code is not upstreamable. Upstream-Status: Inappropriate [lmp specific] Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>

proxyrer · 2025-11-18T15:29:46Z

src/p11_key.c

 	};
 	CK_ULONG num_bits = bits;
-	CK_BYTE public_exponent[] = { 1, 0, 1 };
+	CK_BYTE public_exponent[] = { 1, 0, 0, 0, 1 };


Hi, I'm regularly running some tests with SoftHSM 2.6.1 and libp11. I've noticed that ever since this commit the exponent attribute has changed from the usual 65537 (0x10001) to a surprising 4294967297 (0x100000001).

Before this commit, libp11 would pass the byte array [1, 0, 1] which SoftHSM would unwrap to 65537. The new array [1, 0, 0, 0, 1 ] is unwrapped to 4294967297 obviously raising some questions.

Was this public_exponent change intentional? I'm trying to understand what the intended behavior is.

Could it be an attack against implementations that assume a 32-bit public exponent? Otherwise, a non-standard composite (641 * 6700417) public exponent does not seem to be dangerous.

Interesting! So the value is indeed a valid exponent? Sorry, I'm not too well-versed in cryptography. It seems then that the real limitation that got me here is that the software I use that signs the CSR does not support large public key exponents. Is there a way to specify desired public key exponent in libp11 during key generation? Also let me know if you'd rather I open an issue to move this discussion elsewhere.

@istepic Would you kindly share your rationale for changing { 1, 0, 1 } to { 1, 0, 0, 0, 1 }?

istepic force-pushed the master branch 2 times, most recently from 23ac33d to 97ee3ce Compare September 12, 2022 14:22

istepic pushed a commit to istepic/MoCOCrW that referenced this pull request Nov 18, 2022

ci: install patched p11 from source

23ad269

Patch: OpenSC/libp11#474

istepic pushed a commit to istepic/MoCOCrW that referenced this pull request Nov 18, 2022

ci: install patched p11 from source

b111b0f

Patch: OpenSC/libp11#474

istepic pushed a commit to istepic/MoCOCrW that referenced this pull request Nov 18, 2022

ci: install patched p11 from source

2608715

Patch: OpenSC/libp11#474

istepic pushed a commit to istepic/MoCOCrW that referenced this pull request Nov 18, 2022

ci: install patched p11 from source

b0bcb57

Patch: OpenSC/libp11#474

istepic pushed a commit to istepic/MoCOCrW that referenced this pull request Nov 18, 2022

ci: install patched p11 from source

89ea8dc

Patch: OpenSC/libp11#474

istepic pushed a commit to istepic/MoCOCrW that referenced this pull request Nov 18, 2022

ci: install patched p11 from source

24c3c29

Patch: OpenSC/libp11#474

istepic pushed a commit to istepic/MoCOCrW that referenced this pull request Nov 18, 2022

ci: install patched p11 from source

52d7cb9

Patch: OpenSC/libp11#474

istepic mentioned this pull request Nov 18, 2022

ci: install patched p11 from source istepic/MoCOCrW#3

Merged

istepic pushed a commit to istepic/MoCOCrW that referenced this pull request Nov 18, 2022

ci: install patched p11 from source

0f9d506

Patch: OpenSC/libp11#474

rafajunio force-pushed the master branch from d89ebfe to 7addb3c Compare February 9, 2025 15:51

rafajunio force-pushed the master branch from 98ebc16 to a72dad9 Compare March 26, 2025 23:27

mtrojnar requested a review from olszomal March 27, 2025 06:46

olszomal suggested changes Apr 2, 2025

View reviewed changes

rafajunio force-pushed the master branch from 0b9e027 to b025351 Compare April 3, 2025 09:54

olszomal suggested changes Apr 3, 2025

View reviewed changes

rafajunio force-pushed the master branch from 920184c to c0a4327 Compare April 3, 2025 14:52

olszomal suggested changes Apr 4, 2025

View reviewed changes

rafajunio force-pushed the master branch from c0a4327 to b7becff Compare April 4, 2025 08:28

olszomal approved these changes Apr 4, 2025

View reviewed changes

mtrojnar merged commit c315280 into OpenSC:master Apr 4, 2025
11 checks passed

mtrojnar mentioned this pull request Apr 4, 2025

Add PKCS11_generate_ec_key() for private EC key generation #379

Closed

quaresmajose pushed a commit to quaresmajose/libp11 that referenced this pull request Aug 8, 2025

Fix constness of key generation attributes

bd1fa88

Upstream-Status: Submitted [OpenSC#474] Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>

proxyrer reviewed Nov 18, 2025

View reviewed changes

mtrojnar added a commit that referenced this pull request Nov 21, 2025

Revert RSA public exponent change from #474

ffa84a6

Introduce keypair generation as engine ctrl command #474

Introduce keypair generation as engine ctrl command #474

Uh oh!

Conversation

istepic commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istepic commented Oct 5, 2022

Uh oh!

ldts commented Nov 2, 2022

Uh oh!

dengert commented Nov 3, 2022

Uh oh!

mwasilew commented Nov 10, 2022

Uh oh!

istepic commented Nov 14, 2022

Uh oh!

mtrojnar commented Dec 2, 2024

Uh oh!

rafajunio commented Feb 9, 2025

Uh oh!

mtrojnar commented Feb 10, 2025

Uh oh!

mtrojnar commented Mar 17, 2025

Uh oh!

rafajunio commented Mar 26, 2025

Uh oh!

olszomal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olszomal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olszomal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rafajunio commented Apr 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrojnar Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

istepic commented Sep 8, 2022 •

edited

Loading

mtrojnar Nov 18, 2025 •

edited

Loading