Skip to content

Improve credential loading diagnostics, function parameter declarations, etc.#120

Open
DDvO wants to merge 10 commits into
masterfrom
improve_credential_loading
Open

Improve credential loading diagnostics, function parameter declarations, etc.#120
DDvO wants to merge 10 commits into
masterfrom
improve_credential_loading

Conversation

@DDvO
Copy link
Copy Markdown
Member

@DDvO DDvO commented May 16, 2026

  • credential_loading.{c,h}: generalize cert and CRL loading to support HTTP download of PEM/DER encoded data
  • credential_loading.c: fix FILES_load_credentials_ex() double free, diagnostics, and indentation
  • credential_loading.c: move definition of FILES_load_credentials_ex() and CREDENTIALS_load_ex() for consistency with header file
  • credential_loading.{c,h}: rename 'src' parameter of FILES_load_crl_ex() to 'uri'
  • credential_loading.{c,h}: add missing OPTIONAL and remove needless 'maybe_stdin' parameter of FILES_load_certs_ex()
  • credential_loading.c: improve diagnostics of FILES_load_*_ex() and STORE_load_more_check_ex()
  • credential_loading.{c,h}: consistently use 'file_format_t format' and 'bool maybe_stdin'
  • 80-test_cmp_http.t: align with latest upstream OpenSSL version of that script, , fixing CI hangs for OpenSSL 3.6+ (this commit is also part of add support for ML-DSA and TPM2-held keys referenced via handle; OpenSSL 4.0 compat #119, repeated here to avoid needless CI failures)

@DDvO DDvO added the enhancement New feature or request label May 16, 2026
@DDvO DDvO requested a review from Copilot May 16, 2026 20:06
Copy link
Copy Markdown

Copilot AI left a comment

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 updates credential/certificate/CRL loading to better support HTTP-sourced data and improves diagnostics/consistency across the credential-loading APIs, while also syncing the CMP HTTP test recipe with a newer upstream OpenSSL variant.

Changes:

  • Extend cert/CRL loading logic to download and decode PEM/DER over HTTP (with updated logging and parameter conventions).
  • Refactor/align credential-loading function signatures (OPTIONAL annotations, parameter naming, timeout plumbing).
  • Update 80-test_cmp_http.t and test config to match newer upstream behavior (skip conditions, server host/port derivation, improved failure logs).

Reviewed changes

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

File Description
test/recipes/80-test_cmp_http.t Updates CMP HTTP test harness logic (skip gates, server discovery, enhanced failure logging).
test/recipes/80-test_cmp_http_data/test.cnf Adjusts port config to reference $server_port for dynamic substitution.
src/credential_loading.h Updates/aligns credential-loading API declarations (OPTIONAL, formats, parameter naming).
src/credential_loading.c Implements HTTP download+decode helpers and refactors credential/cert/CRL loading and diagnostics.

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

Comment thread src/credential_loading.c Outdated
Comment thread src/credential_loading.c
Comment thread src/credential_loading.c Outdated
Comment thread src/credential_loading.c Outdated
Comment thread test/recipes/80-test_cmp_http.t
Comment thread src/credential_loading.c
Copy link
Copy Markdown

Copilot AI left a comment

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 4 out of 4 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

src/credential_loading.c:908

  • FILES_load_crl_ex() leaks the memory BIO returned by http_get_mem(): mem is not freed after PEM_read_bio_X509_CRL()/d2i_X509_CRL_bio(). Mirror the certificate-loading path and BIO_free(mem) in all cases (including decode failure).
        BIO *mem = http_get_mem(uri, timeout, "-----BEGIN X509 CRL-----", &is_pem, desc);
        if (mem != NULL) {
            crl = is_pem ? PEM_read_bio_X509_CRL(mem, NULL, NULL, NULL) : d2i_X509_CRL_bio(mem, NULL);
            if (crl == NULL)
                LOG(FL_ERR, "Unable to decode %s from %s", desc, uri);
        }

Comment thread src/credential_loading.c Outdated
Comment thread test/recipes/80-test_cmp_http.t
Comment thread src/credential_loading.c
@DDvO DDvO force-pushed the improve_credential_loading branch from d8a478e to 9341a74 Compare May 18, 2026 10:11
@DDvO DDvO requested a review from Copilot May 18, 2026 10:12
Copy link
Copy Markdown

Copilot AI left a comment

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 4 out of 4 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

src/credential_loading.c:916

  • FILES_load_crl_ex() uses CONN_IS_HTTP(uri) / CONN_IS_HTTPS(uri) and calls CRL_check(uri, ...), but uri is an OPTIONAL parameter and may be NULL when reading from stdin. This can lead to crashes/undefined behavior (e.g., %s formatting with a NULL src). Add uri != NULL guards for scheme checks and pass a non-NULL placeholder like "<stdin>" into CRL_check() / error messages when uri is NULL.
    LOG(FL_DEBUG, "Loading %s from %s", desc, uri != NULL ? uri : "<stdin>");
    if (CONN_IS_HTTP(uri) || CONN_IS_HTTPS(uri)) {
        bool is_pem;
        BIO *mem = http_get_mem(uri, timeout, "-----BEGIN X509 CRL-----", &is_pem, desc);
        if (mem != NULL) {
            crl = is_pem ? PEM_read_bio_X509_CRL(mem, NULL, NULL, NULL) : d2i_X509_CRL_bio(mem, NULL);
            BIO_free(mem);
            if (crl == NULL)
                LOG(FL_ERR, "Unable to decode %s from %s", desc, uri);
        }
    } else {
        (void)load_key_certs_crls(libctx, propq,
                                  uri, format, maybe_stdin, NULL, desc, false,
                                  NULL, NULL,  NULL, NULL, NULL, 0, &crl, NULL, 1);
    }
    if (!CRL_check(uri, crl, vpm) && vpm != NULL) {
        X509_CRL_free(crl);
        crl = NULL;

Comment thread src/credential_loading.c Outdated
Comment thread src/credential_loading.c
Comment thread test/recipes/80-test_cmp_http.t
Comment thread test/recipes/80-test_cmp_http.t
@DDvO DDvO force-pushed the improve_credential_loading branch from 9341a74 to 9361201 Compare May 18, 2026 13:23
@DDvO DDvO requested a review from Copilot May 18, 2026 13:26
Copy link
Copy Markdown

Copilot AI left a comment

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 4 out of 4 changed files in this pull request and generated 4 comments.

Comment thread src/credential_loading.c Outdated
Comment thread src/credential_loading.c
Comment thread test/recipes/80-test_cmp_http.t
Comment thread test/recipes/80-test_cmp_http.t
@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
44.9% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants