Skip to content

add support for ML-DSA-* etc., various small fixes; C23 and OpenSSL 4.0 compat#72

Merged
DDvO merged 10 commits into
masterfrom
extend_KEY_new
Jun 24, 2026
Merged

add support for ML-DSA-* etc., various small fixes; C23 and OpenSSL 4.0 compat#72
DDvO merged 10 commits into
masterfrom
extend_KEY_new

Conversation

@DDvO

@DDvO DDvO commented Apr 24, 2026

Copy link
Copy Markdown
Member

This adds
EVP_PKEY *KEY_new_ex(const char *spec, OPTIONAL OSSL_LIB_CTX *libctx, OPTIONAL const char *propq)
and defines KEY_new(spec) as KEY_new_ex(spec, NULL, NULL) for backward compatibility.

For OpenSSL 3.0+, a location of the form "tpm2:handle=0x" can be used for key generation within the tpm2-openssl provider.
For OpenSSL 3.5+, the spec parameter may now be a PQC algorithm name (and level), e.g.., "ML-DSA-65".

On this, occasion, due to Copilot review comment on credentials.c,
also fix find_update_cb() crash on NULL tag/description strings.

Moreover:

  • for OpenSSL 4.0 compatibility, fix constness in cdp_util.{c,h} and crl_mgmt.c
  • improve STORE_set1_host_ip() behavior on equal 'name' and 'ip' argument
  • CREDENTIALS_print_cert_verify_cb(): fix output of expected IP address on mismatch
  • improve CONN_IS_HTTP(S) and use it to replace respective occurrences of strncasecmp()
  • conn.c,util.h: add UTIL_SKIP_SCHEME() and improve skip_scheme() using it
  • basic.h: fix compatibility with C23, which provides true and false as keywords (also covered meanwhile by Fix Debian packaging #74)

@DDvO DDvO added the enhancement New feature or request label Apr 24, 2026
@DDvO DDvO requested a review from Copilot April 24, 2026 13:01

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR extends libsecutils key generation to support OpenSSL 3 provider-based algorithms (including ML-DSA-*), and introduces a new KEY_new_ex() API that can generate/provider-reference TPM2-held keys via a TPM2 persistent handle URI.

Changes:

  • Add KEY_new_ex(spec, location, libctx) and redefine KEY_new() as a wrapper.
  • Implement OpenSSL 3 provider-based key generation (name-based, plus TPM2 handle support).
  • Adjust credential saving logic and docs to avoid trying to persist private keys that are not file-backed (engine/TPM2 handle).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/libsecutils/src/credentials/key.c Adds provider-based keygen path (OpenSSL 3), TPM2 handle parsing, and expanded spec parsing.
src/libsecutils/src/credentials/credentials.c Avoids attempting to save private key when key is a TPM2 handle URI.
src/libsecutils/include/secutils/util/util.h Adds an OpenSSL < 3.0 compatibility typedef for OSSL_LIB_CTX.
src/libsecutils/include/secutils/credentials/key.h Declares KEY_new_ex(), adds TPM2 handle prefix constant, and changes KEY_new to a macro wrapper.
src/libsecutils/include/secutils/credentials/credentials.h Updates API docs to reflect provider/TPM2-handle save behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/libsecutils/src/credentials/key.c Outdated
Comment thread src/libsecutils/src/credentials/key.c Outdated
Comment thread src/libsecutils/src/credentials/key.c
Comment thread src/libsecutils/src/credentials/key.c Outdated
Comment thread src/libsecutils/src/credentials/key.c
Comment thread src/libsecutils/include/secutils/credentials/key.h Outdated
Comment thread src/libsecutils/src/credentials/credentials.c Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/libsecutils/include/secutils/connections/conn.h Outdated
Comment thread src/libsecutils/include/secutils/connections/conn.h
Comment thread src/libsecutils/src/credentials/key.c Outdated
Comment thread src/libsecutils/src/credentials/key.c
Comment thread src/libsecutils/src/credentials/key.c Outdated
Comment thread src/libsecutils/src/credentials/credentials.c Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/libsecutils/src/credentials/credentials.c:348

  • strcmp(key, certs) will dereference certs even when certs is NULL (this function allows certs to be optional as long as key is provided). This can crash when saving only a key. Consider guarding with key != NULL && certs != NULL && strcmp(...) == 0 (or restructuring) before calling FILES_get_format().
    file_format_t format = key not_eq 0 and strcmp(key, certs) is_eq 0 ? FILES_get_format(key) : FORMAT_PEM;
    bool res = FILES_store_credentials(pkey, creds->cert, creds->chain, key, certs, format, source, desc);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/libsecutils/include/secutils/credentials/key.h Outdated
Comment thread src/libsecutils/include/secutils/credentials/key.h Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/libsecutils/src/credentials/credentials.c:348

  • certs is an OPTIONAL parameter, but this line unconditionally calls strcmp(key, certs) whenever key != 0. If certs == NULL (allowed by the API), this will dereference a null pointer and crash. Guard the comparison with certs != NULL (and only compute joint-file format when both are non-null).
    file_format_t format = key not_eq 0 and strcmp(key, certs) is_eq 0 ? FILES_get_format(key) : FORMAT_PEM;
    bool res = FILES_store_credentials(pkey, creds->cert, creds->chain, key, certs, format, source, desc);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/libsecutils/src/credentials/credentials.c
@DDvO DDvO force-pushed the extend_KEY_new branch 2 times, most recently from c1dfae3 to f26a4ae Compare April 27, 2026 12:45
@DDvO DDvO requested a review from Copilot April 27, 2026 12:46
@DDvO DDvO added the bug Something isn't working label Apr 27, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/libsecutils/src/credentials/credentials.c:350

  • CREDENTIALS_save() computes format using strcmp(key, certs) without guarding against certs == NULL. Since the function allows certs to be NULL when saving only a key, this can dereference a NULL pointer when key != NULL. Please guard the comparison (e.g., require both key and certs to be non-NULL before calling strcmp) and consider setting key = NULL in the TPM2-handle case to avoid treating the handle string like a filename later in the function.
    if (key != NULL && HAS_PREFIX(key, SECUTILS_TPM2_HANDLE_PREFIX)) {
        /* key refers to TPM2 handle, so cannot save private key */
        if (creds->cert == NULL && creds->chain == NULL)
            goto update; /* well, though nothing is actually saved in this case */
        source = 0;
    } else if (source != NULL && strncmp(source, sec_ENGINE_STR, strlen(sec_ENGINE_STR)) == 0) {
        /* source refers to engine, so cannot save private key */
        source = 0;
    } else {
        pkey = creds->pkey;
    }

    file_format_t format = key not_eq 0 and strcmp(key, certs) is_eq 0 ? FILES_get_format(key) : FORMAT_PEM;
    bool res = FILES_store_credentials(pkey, creds->cert, creds->chain, key, certs, format, source, desc);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/libsecutils/src/credentials/key.c Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/libsecutils/src/credentials/key.c Outdated
Comment thread src/libsecutils/src/certstatus/cdp_util.c Outdated
@DDvO DDvO force-pushed the extend_KEY_new branch 2 times, most recently from 6a944a2 to 85fe1b7 Compare April 27, 2026 17:33
@DDvO DDvO force-pushed the extend_KEY_new branch 2 times, most recently from 4655528 to 11a6441 Compare May 18, 2026 10:07
@DDvO DDvO force-pushed the extend_KEY_new branch from 11a6441 to 2587a15 Compare June 9, 2026 08:11
@DDvO DDvO marked this pull request as ready for review June 9, 2026 08:13
@DDvO DDvO requested a review from Copilot June 9, 2026 08:13
@DDvO DDvO changed the title add support for ML-DSA-* and TPM2-held keys with key handles; OpenSSL 4.0 compat add support for ML-DSA-* etc., various small fixes; C23 and OpenSSL 4.0 compat Jun 9, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.

Comment thread src/libsecutils/include/secutils/util/util.h
Comment thread src/libsecutils/src/credentials/store.c Outdated
Comment thread src/libsecutils/src/credentials/key.c
Comment thread src/libsecutils/include/secutils/credentials/key.h
Comment thread debian/changelog Outdated
@DDvO DDvO force-pushed the extend_KEY_new branch from 2587a15 to 04948c1 Compare June 9, 2026 08:37
@DDvO

DDvO commented Jun 9, 2026

Copy link
Copy Markdown
Member Author

@rajeev-0 as discussed yesterday,
I've removed (for the time being at least) the tentative special support of TPM2 handles during key creation,
which was ad-hoc and premature, not working as expected, and likely unrealistic.

@DDvO

DDvO commented Jun 9, 2026

Copy link
Copy Markdown
Member Author

This PR is now in final shape for reviews.

@rajeev-0 rajeev-0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@DDvO DDvO requested a review from benjamin-schilling June 13, 2026 06:30
@DDvO

DDvO commented Jun 17, 2026

Copy link
Copy Markdown
Member Author

Ping @benjamin-schilling for review/approval

@DDvO DDvO force-pushed the extend_KEY_new branch 2 times, most recently from 018e099 to 2a22aaf Compare June 23, 2026 15:39
@DDvO

DDvO commented Jun 23, 2026

Copy link
Copy Markdown
Member Author

Rebased on latest master, which lead to improving
debian/changelog: add entries and make sure lines are not too long

@DDvO DDvO force-pushed the extend_KEY_new branch from 2a22aaf to 6c69d06 Compare June 23, 2026 15:54

@benjamin-schilling benjamin-schilling left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

only two minor topics, can be merged afterwards.

Comment thread debian/changelog
@@ -1,3 +1,30 @@
libsecutils (2.3) stable; urgency=medium

* TODO: complete this and update date below

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is this still relevant or can it be deleted?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I had added this TODO as a reminder for myself to complete it when doing the next release.
It is not yet fully done, as more changes are to be expected.


/*!*****************************************************************************
* @brief parse hostname or URI of the form "[http[s]://][<userinfo>@]<host>[:<port>][/<path>]"
* @brief parse hostname or URI of the form "[http[s]://][<userinfo>@]<host>[:port][/<path>]"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why remove the <> for port and not for the others?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good spotting - I removed the < and > specifically for port
because, for some weird reason, Doxygen got confused by those.

@DDvO DDvO merged commit 736e707 into master Jun 24, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants