gh-libtom/libtomcrypt
Watch
1
0
Fork
You've already forked libtomcrypt
 0
Code Issues 42 Pull requests 17 Releases 9 Wiki Activity

X509 import #697

Open
sjaeckel wants to merge 45 commits from x509_import into develop
pull from: x509_import
merge into: gh-libtom:develop
Conversation 10 Commits 45 Files changed 387 +20274 -92
sjaeckel commented 2025-09-08 07:20:50 -05:00 (Migrated from github.com)

Start implemeting X.509 APIs.

In #693 this was requested and I was already at it and now we have the first part: Parsing of an X.509 certificate and the cryptographic validation of such a certificate.

NB: This does not yet contain the logic that is required to determine whether a CA is eligible to sign a certificate etc.

Checklist

  • documentation is added or updated
  • tests are added or updated
Start implemeting X.509 APIs. In #693 this was requested and I was already at it and now we have the first part: Parsing of an X.509 certificate and the cryptographic validation of such a certificate. NB: This does not yet contain the logic that is required to determine whether a CA is eligible to sign a certificate etc. ### Checklist <!-- Remove items that do not apply. For completed items, change [ ] to [x]. --> * [ ] documentation is added or updated * [x] tests are added or updated
levitte (Migrated from github.com) reviewed 2025-09-08 07:20:50 -05:00
karel-m (Migrated from github.com) reviewed 2025-09-08 07:20:50 -05:00
karel-m commented 2026-04-11 16:40:57 -05:00 (Migrated from github.com)

@sjaeckel Why was the CI pipeline with tests and builds not triggered after my last commit?

@sjaeckel Why was the CI pipeline with tests and builds not triggered after my last commit?
sjaeckel commented 2026-04-11 17:21:50 -05:00 (Migrated from github.com)

I guess because the target branch isn't develop!? Not sure though, just a guess.

I guess because the target branch isn't develop!? Not sure though, just a guess.
sjaeckel commented 2026-04-15 05:32:58 -05:00 (Migrated from github.com)

I took the liberty to squash your adaptations into the main commit. I hope that's fine.

I took the liberty to squash your adaptations into the main commit. I hope that's fine.
karel-m commented 2026-04-16 17:59:43 -05:00 (Migrated from github.com)

I've just reviewed this PR and pushed a few commits. Two issues I’m still not happy with:

  • We are not rejecting certificates that contain unrecognized critical extensions, as required by RFC 5280 section 4.2. I've just added a comment to code about this.
  • It is a bit surprising that x509_free takes ltc_x509_certificate ** as its argument. Other _free functions take a single pointer (rsa_free, ecc_free, pka_key_free ...) so x509_free is currently the odd one out.

I also managed to break the CI tests somehow; however, it is too late tonight for me to fix that now.

I've just reviewed this PR and pushed a few commits. Two issues I’m still not happy with: * We are not rejecting certificates that contain unrecognized critical extensions, as required by RFC 5280 section 4.2. I've just added a comment to code about this. * It is a bit surprising that x509_free takes ltc_x509_certificate ** as its argument. Other _free functions take a single pointer (rsa_free, ecc_free, pka_key_free ...) so x509_free is currently the odd one out. I also managed to break the CI tests somehow; however, it is too late tonight for me to fix that now.
karel-m (Migrated from github.com) requested changes 2026-04-16 18:00:19 -05:00
karel-m (Migrated from github.com) left a comment

see my commits

see my commits
sjaeckel (Migrated from github.com) reviewed 2026-04-17 10:09:03 -05:00
sjaeckel (Migrated from github.com) left a comment
  • We are not rejecting certificates that contain unrecognized critical extensions, as required by RFC 5280 section 4.2. I've just added a comment to code about this.

Good point!

  • It is a bit surprising that x509_free takes ltc_x509_certificate ** as its argument. Other _free functions take a single pointer (rsa_free, ecc_free, pka_key_free ...) so x509_free is currently the odd one out.

Because I like the pattern more. I prefer to be on the safe side, see also the commit I just added which introduces the same pattern for s_free().

> * We are not rejecting certificates that contain unrecognized critical extensions, as required by RFC 5280 section 4.2. I've just added a comment to code about this. Good point! > * It is a bit surprising that x509_free takes ltc_x509_certificate ** as its argument. Other _free functions take a single pointer (rsa_free, ecc_free, pka_key_free ...) so x509_free is currently the odd one out. Because I like the pattern more. I prefer to be on the safe side, see also the commit I just added which introduces the same pattern for `s_free()`.
src/pk/asn1/x509/x509_extensions.c
@ -0,0 +743,4 @@
but currently it cannot be rejected here as some critical extensions are not decoded
like: Name Constraints, Inhibit Any Policy, No Revocation Available, OCSP No Check,
Proxy Certificate Information, ac-auditEntity
*/
sjaeckel (Migrated from github.com) commented 2026-04-17 07:43:58 -05:00

While adding some of them as dummy I came to the conclusion: this is just parsing and not validation. Should we maybe track this as "yes, it's a valid topic, but not now"?

I've collected the following infos on the extensions you mentioned which are tracked outside RFC 5280:

  1. No Revocation Available is CRITICALITY { FALSE }
  2. OCSP No Check The RFC says: The CA does so by including the extension id-pkix-ocsp-nocheck. This SHOULD be a non-critical extension. The value of the extension SHALL be NULL.
  3. Proxy Certificate Information is indeed critical
  4. ac-auditEntity I couldn't find anything
While adding some of them as dummy I came to the conclusion: this is just parsing and not validation. Should we maybe track this as "yes, it's a valid topic, but not now"? I've collected the following infos on the extensions you mentioned which are tracked outside RFC 5280: 1. [No Revocation Available](https://www.rfc-editor.org/rfc/rfc9608) is `CRITICALITY { FALSE }` 2. [OCSP No Check](https://www.rfc-editor.org/rfc/rfc6960#section-4.2.2.2.1) The RFC says: `The CA does so by including the extension id-pkix-ocsp-nocheck. This SHOULD be a non-critical extension. The value of the extension SHALL be NULL.` 3. [Proxy Certificate Information](https://www.rfc-editor.org/rfc/rfc3820#section-3.8) is indeed critical 4. `ac-auditEntity` I couldn't find anything
karel-m commented 2026-05-05 13:58:27 -05:00 (Migrated from github.com)

what is the origin of tests/x509/gnutls/template-krb5name-full.pem ?

cert's KeyUsage is a BIT STRING with all bits zero, which is IMO not valid according to RFC 5280 4.2.1.3

should it not be in gnutls/bad/ ?

openssl parses/accepts these

UPDATE: openssl rejects it later github.com/openssl/openssl@e483d93b39/crypto/x509/v3_purp.c (L631)

~what is the origin of `tests/x509/gnutls/template-krb5name-full.pem` ?~ ~cert's KeyUsage is a BIT STRING with all bits zero, which is IMO not valid according to RFC 5280 4.2.1.3~ ~should it not be in `gnutls/bad/` ?~ openssl parses/accepts these UPDATE: openssl rejects it later https://github.com/openssl/openssl/blob/e483d93b393bec71327c3b31779e8dcf37b3f42b/crypto/x509/v3_purp.c#L631
karel-m commented 2026-05-05 14:07:34 -05:00 (Migrated from github.com)

tests/x509/openssl/embeddedSCTs1_issuer.pem = a self-signed CA root with BasicConstraints: CA:TRUE but not critical

maybe it is fine as openssl accepts CA:TRUE without critical

~tests/x509/openssl/embeddedSCTs1_issuer.pem = a self-signed CA root with BasicConstraints: CA:TRUE but not critical~ maybe it is fine as openssl accepts CA:TRUE without critical
This pull request has changes conflicting with the target branch.
  • makefile.mingw
  • makefile.msvc
  • makefile.unix
  • makefile_include.mk
  • tests/sources.cmake
  • tests/test.c
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 'x509_import:x509_import'
git switch 'x509_import'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'develop'
git merge --no-ff 'x509_import'
git switch 'x509_import'
git rebase 'develop'
git switch 'develop'
git merge --ff-only 'x509_import'
git switch 'x509_import'
git rebase 'develop'
git switch 'develop'
git merge --no-ff 'x509_import'
git switch 'develop'
git merge --squash 'x509_import'
git switch 'develop'
git merge --ff-only 'x509_import'
git switch 'develop'
git merge 'x509_import'
git push origin 'develop'
Sign in to join this conversation.
Reviewers
No reviewers
levitte
karel-m
Labels
Clear labels   
bug

   
duplicate

   
enhancement

   
feature

   
help wanted

   
improve API

   
invalid

   
needs documentation

   
not-planned
This ticket is not planned to be implemented and/or fixed from the core team.

  
rtfm

   
stale

   
support

   
wishlist
No labels
bug
duplicate
enhancement
feature
help wanted
improve API
invalid
needs documentation
not-planned
rtfm
stale
support
wishlist
Milestone
Clear milestone
No items
No milestone
next
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-libtom/libtomcrypt!697
No description provided.