X509: use V3 when extensions present; certs generated by apps bear X.509 V3 by default by DDvO · Pull Request #19271 · openssl/openssl

DDvO · 2022-09-24T22:11:02Z

When preparing #19076 I found that by default (when no X.509 v3 extensions are used),
certs generated by the x509, ca, and req apps still have X.509 version 1.
This is anachronistic and causes various issues, such as missing key identifiers for chain building
and no clear distinction between CA certs and EE certs.

This PR makes sure V3 is used,
with the exception of a new -x509v1 option of the req app, which implies the -x509 option while ~~forcing~~ using V1 unless extensions are given (which requires V3).

On this occasion, also

apps/req.c: properly report parse errors by duplicated(); simplify the function
~~CHANGES.md: 'added' -> 'add', 'extended' -> 'extend' for OpenSSL 3.1 entries~~
~~test/trace_api_test.c: fix gcc error on -Werror=strict-prototypes~~

Checklist

documentation is added or updated
tests are added or updated

DDvO · 2022-09-24T22:28:25Z

The failing external-tests CI run with error 79 at 1 depth lookup: invalid CA certificate
are most likely due to missing basicConstraints = CA:true, which I also fixed in 90-test_store.t.
@beldmit, please verify and fix gost-engine.

DDvO · 2022-09-25T06:00:48Z

And how about the API level?

I suggest adapting X509_sign{_ctx}() to set version 3 if extensions are present,
which prevents X509_V_ERR_EXTENSIONS_REQUIRE_VERSION_3 on verification.

beldmit · 2022-09-25T09:42:57Z

@DDvO could you please try the gost-engine/engine@a6b9052 ?

DDvO · 2022-09-25T14:25:57Z

@DDvO could you please try the gost-engine/engine@a6b9052 ?

Thank you for your immediate handling - with this updated submodule reference it works fine :)

t8m

I would drop the first commit. IMO the past tense is correct.

test/trace_api_test.c

apps/lib/apps.c

DDvO · 2022-09-26T10:47:28Z

I would drop the first commit. IMO the past tense is correct.

Will do with the next push.

DDvO · 2022-10-08T13:02:18Z

@t8m, ok now?

t8m

Just a few nits to fix

CHANGES.md

DDvO · 2022-10-18T16:26:46Z

Just a few nits to fix

Thanks @t8m for your further comments, which I had overlooked so far, but now they are handled.

t8m

CI failures are unrelated.

DDvO · 2022-11-17T15:06:13Z

Rebased to fix merge conflict in x_all.c

Ping for 2nd review.

…alues

…e function

… is given

DDvO · 2022-11-21T11:35:39Z

Rebased to fix trivial merge conflict in CHANGES.md.

openssl-machine · 2022-12-22T00:08:55Z

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

t8m · 2022-12-22T09:19:29Z

Adding some more tests would be nice.

…req app is given

DDvO · 2022-12-22T13:09:33Z

Adding some more tests would be nice.

Done.

DDvO · 2022-12-22T15:30:00Z

@t8m wanna reconfirm, now since more tests have been added?
I also added note to the HISTORY section of the three app docs.

t8m

CI failure is not related.

openssl-machine · 2023-01-22T00:09:41Z

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

t8m · 2023-01-23T09:52:59Z

ping for second review

openssl-machine · 2023-01-24T13:00:22Z

This pull request is ready to merge

DDvO · 2023-01-24T14:17:57Z

Merged - thanks @t8m and @hlandau for the reviews.

…alues Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Hugo Landau <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from #19271)

…e function Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Hugo Landau <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from #19271)

… is given Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Hugo Landau <[email protected]> Reviewed-by: David von Oheimb <[email protected]> (Merged from #19271)

DDvO added branch: master Applies to master branch approval: review pending This pull request needs review by a committer approval: otc review pending triaged: feature The issue/pr requests/adds a feature labels Sep 24, 2022

DDvO changed the title ~~APPS: generated certs bear X.509 V3 by default~~ APPS: make generated certs bear X.509 V3 by default Sep 24, 2022

DDvO requested review from t8m and vdukhovni September 24, 2022 22:22

t8m requested changes Sep 26, 2022

View reviewed changes

t8m reviewed Sep 26, 2022

View reviewed changes

test/trace_api_test.c Outdated Show resolved Hide resolved

t8m reviewed Sep 26, 2022

View reviewed changes

apps/lib/apps.c Show resolved Hide resolved

DDvO mentioned this pull request Sep 26, 2022

test/trace_api_test.c: fix gcc error on -Werror=strict-prototypes #19277

Closed

DDvO changed the title ~~APPS: make generated certs bear X.509 V3 by default~~ X509: use V3 when extensions present; certs generated by apps bear X.509 V3 by default Sep 28, 2022

DDvO force-pushed the X509_v3_by_default branch from 7276dc9 to a3eb6c1 Compare September 28, 2022 19:27

DDvO requested a review from t8m September 29, 2022 10:46

DDvO mentioned this pull request Sep 29, 2022

Fix regression in i2d_re_X509_REQ_tbs() #19299

Closed

t8m requested changes Oct 10, 2022

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

CHANGES.md Outdated Show resolved Hide resolved

CHANGES.md Outdated Show resolved Hide resolved

t8m approved these changes Oct 19, 2022

View reviewed changes

t8m removed the approval: otc review pending label Oct 19, 2022

DDvO force-pushed the X509_v3_by_default branch from fb22f54 to 24cfffa Compare November 17, 2022 15:05

t8m approved these changes Nov 18, 2022

View reviewed changes

DDvO added 2 commits November 21, 2022 12:34

X509{,_CRL,_REVOKED}_{set,sign}*(): fix 'modified' field and return v…

5965853

…alues

apps/req.c: properly report parse errors by duplicated(); simplify th…

5e98726

…e function

APPS: generated certs bear X.509 V3, unless -x509v1 option of req app…

68c853e

… is given

DDvO force-pushed the X509_v3_by_default branch from 24cfffa to 68c853e Compare November 21, 2022 11:35

t8m approved these changes Dec 22, 2022

View reviewed changes

t8m added the tests: present The PR has suitable tests present label Dec 22, 2022

fixup! APPS: generated certs bear X.509 V3, unless -x509v1 option of …

abbdbf1

…req app is given

t8m approved these changes Dec 22, 2022

View reviewed changes

hlandau approved these changes Jan 23, 2023

View reviewed changes

hlandau added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 23, 2023

openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jan 24, 2023

DDvO closed this Jan 24, 2023

bdewater mentioned this pull request Dec 29, 2023

Tests failing when using OpenSSL library v3 cedarcode/webauthn-ruby#417

Closed

rhenium mentioned this pull request Jun 8, 2024

Certificate.new vs Certificate.new(string) ruby/openssl#519

Closed

Uh oh!

Comments

Conversation

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

Uh oh!

Checklist

Uh oh!

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

Uh oh!

Uh oh!

DDvO commented Sep 25, 2022

Uh oh!

beldmit commented Sep 25, 2022

Uh oh!

DDvO commented Sep 25, 2022

Uh oh!

t8m left a comment • 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!

Uh oh!

Uh oh!

DDvO commented Sep 26, 2022

Uh oh!

DDvO commented Oct 8, 2022

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DDvO commented Oct 18, 2022

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

DDvO commented Nov 17, 2022

Uh oh!

DDvO commented Nov 21, 2022

Uh oh!

openssl-machine commented Dec 22, 2022

Uh oh!

t8m commented Dec 22, 2022

Uh oh!

DDvO commented Dec 22, 2022

Uh oh!

DDvO commented Dec 22, 2022

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

openssl-machine commented Jan 22, 2023

Uh oh!

t8m commented Jan 23, 2023

Uh oh!

openssl-machine commented Jan 24, 2023

Uh oh!

DDvO commented Jan 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DDvO commented Sep 24, 2022 •

edited

Loading

DDvO commented Sep 24, 2022 •

edited

Loading

t8m left a comment •

edited

Loading