Skip to content

Add x509v3_EXT_nconf - #11951

Open
kojo1 wants to merge 2 commits into
wolfSSL:masterfrom
kojo1:X509V3_nconf
Open

kojo1 wants to merge 2 commits into
wolfSSL:masterfrom
kojo1:X509V3_nconf

Conversation

@kojo1

@kojo1 kojo1 commented Oct 31, 2026

Copy link
Copy Markdown
Contributor

Description

Add X509V3_set_nconf and X509V3_EXT_add_nconf with following tags.

  • basicConstraints
  • keyUsage
  • subjectKeyIdentifier
  • subjectAltName
  • authorityKeyIdentifier
  • xtendedKeyUsage

Fixes zd#22717

Testing

Add test_wolfSSL_X509V3_EXT_add_nconf

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

Copilot AI balanced review requested due to automatic review settings October 31, 2026 06:58
@github-actions

github-actions Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Signing fails to preserve requested extension criticality, producing certificates that differ from their configured security requirements.

4 open issues
What changed in this PR

This PR adds configuration-driven X.509 extension support to wolfSSL’s OpenSSL compatibility layer, replacing the X509V3_EXT_add_nconf stub in supported builds.

Changes:

  • Implements parsing for basic constraints, key usage, extended key usage, key identifiers, and subject alternative names.
  • Adds configuration, issuer, and subject references to the X.509 extension context.
  • Adds tests for certificate generation, malformed configuration values, and SAN rollback on failure.
File Description
wolfssl/​ssl.h Adds borrowed certificate and configuration references to the context.
wolfssl/​openssl/​x509v3.h Declares and conditionally maps X509V3_set_nconf.
tests/​X509V3_EXT_nconf_test.cnf Supplies valid and invalid extension configurations.
tests/​include.am Includes the new configuration fixture in distributions.
tests/​api/​test_ossl_x509_ext.h Declares and registers the new API test.
tests/​api/​test_ossl_x509_ext.c Tests extension configuration, certificate round trips, and failure handling.
src/​x509.c Implements context configuration and extension parsing.

Review effort: Balanced

Comment thread src/x509.c
Comment on lines +18536 to +18539
if (XSTRNCASECMP(v, "critical", 8) == 0 &&
(v[8] == ',' || v[8] == '\0')) {
crit = 1;
v += (v[8] == ',') ? 9 : 8;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed as follows:

  • subjectAltName: the critical flag is now stored in subjAltNameCrit and carried into Cert.altNamesCrit in CertFromX509()/ReqCertFromX509(), so it survives signing.
  • keyUsage / extendedKeyUsage / subjectKeyIdentifier / authorityKeyIdentifier: the current encoder always emits KU as critical and EKU/SKID/AKID as non-critical, so it can't honor other settings. Rather than silently producing a different certificate, X509V3_EXT_add_nconf() now returns failure for these cases (non-critical keyUsage, or critical EKU/SKID/AKID).

Supporting every combination would mean adding criticality fields to the public Cert struct and changing the encoder templates. I'd prefer to handle that in a separate PR.

Tests: a sign/parse round trip checks the BC/KU/EKU/SAN critical flags. New negative cases (bad_ku_noncrit, bad_eku_crit, bad_skid_crit, bad_akid_crit) check the rejections.

Comment thread src/x509.c Outdated
Comment on lines +18202 to +18203
if (GetSequence(der, &idx, &len, maxIdx) < 0) {
return WOLFSSL_FAILURE;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. x509->pubKey.buffer is not always a SubjectPublicKeyInfo: a parsed RSA certificate stores the RSAPublicKey, Ed25519/ML-DSA store the raw key, and X509_set_pubkey() with RSA/ECC stores an SPKI.

The keyid calculation now works like this:

  • If the buffer parses as an SPKI (the outer SEQUENCE spans the whole buffer, the algorithm OID matches pubKeyOID, and the BIT STRING ends at the end of the buffer), the BIT STRING contents are hashed, as OpenSSL does.
  • Otherwise, the whole buffer is hashed, since it is already the subjectPublicKey value.
  • ECC keys are always expected to be an SPKI, so a parse failure is an error.

Regression tests compare the result against the OpenSSL-generated SKIDs in the test certificates for parsed ECC, parsed RSA, RSA via X509_set_pubkey(), parsed Ed25519 and Ed25519 via X509_set_pubkey().

While doing this I also fixed wolfSSL_X509_set_subject_key_id(), which did not drop the subjKeyIdStr cache, so X509_get0_subject_key_id() could return a stale value.

Comment thread src/x509.c Outdated
Comment on lines +18556 to +18558
if (ParseExtKeyUsageStr(v, &cert->extKeyUsage, cert->heap) == 0) {
cert->extKeyUsageCrit = (byte)crit;
ret = WOLFSSL_SUCCESS;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Setting extendedKeyUsage now also builds the DER-encoded OID list (extKeyUsageSrc / extKeyUsageSz / extKeyUsageCount) alongside the extKeyUsage bitmask, so X509_get_ext_d2i(NID_ext_key_usage) returns the configured usages before the certificate is signed. A test checks that the getter returns the 2 configured EKU OIDs on the unsigned certificate.

Comment thread src/x509.c Outdated
Comment on lines +18568 to +18569
/* Criticality is not stored for subjectAltName. */
ret = x509v3_conf_san(conf, cert, v);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. When at least one SAN entry is added successfully, subjAltNameSet is set and subjAltNameCrit records the critical flag. If an entry fails partway, the entries added for it are rolled back, so the flags and the existing alt names are left as they were. Tests check X509_ext_isSet_by_NID(NID_subject_alt_name) and that X509_get_ext_d2i(NID_subject_alt_name) returns all 4 entries before signing, and that the count is unchanged after the failing bad_san_partial section.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants