Skip to content

Comments

fix(x509.c): fixed -checkend return values#29155

Closed
snowdroppe wants to merge 2 commits intoopenssl:masterfrom
snowdroppe:fix-checkend
Closed

fix(x509.c): fixed -checkend return values#29155
snowdroppe wants to merge 2 commits intoopenssl:masterfrom
snowdroppe:fix-checkend

Conversation

@snowdroppe
Copy link
Contributor

@snowdroppe snowdroppe commented Nov 15, 2025

Fixes #28928

Before

openssl x509 -in /tmp/fullchain.cer -checkend $[3600*24*365*0]; echo $?
Certificate will not expire
0

% openssl x509 -in /tmp/fullchain.cer -checkend $[3600*24*365*10]; echo $?
Certificate will expire
0

openssl x509 -in /tmp/fullchain.cer -multi -checkend $[3600*24*365*0]; echo $?
Certificate will not expire
Certificate will not expire
Certificate will not expire
0

openssl x509 -in /tmp/fullchain.cer -multi -checkend $[3600*24*365*1]; echo $?
Certificate will expire
Certificate will not expire
Certificate will not expire
0

openssl x509 -in /tmp/fullchain.cer -multi -checkend $[3600*24*365*4]; echo $?
Certificate will expire
Certificate will not expire
Certificate will expire
0

openssl x509 -in /tmp/fullchain.cer -multi -checkend $[3600*24*365*20]; echo $?
Certificate will expire
Certificate will expire
Certificate will expire
0

After

openssl x509 -in /tmp/fullchain.cer -checkend $[3600*24*365*0]; echo $?
Certificate will not expire
0

% openssl x509 -in /tmp/fullchain.cer -checkend $[3600*24*365*10]; echo $?
Certificate will expire
1

openssl x509 -in /tmp/fullchain.cer -multi -checkend $[3600*24*365*0]; echo $?
Certificate will not expire
Certificate will not expire
Certificate will not expire
0

openssl x509 -in /tmp/fullchain.cer -multi -checkend $[3600*24*365*1]; echo $?
Certificate will expire
Certificate will not expire
Certificate will not expire
1

openssl x509 -in /tmp/fullchain.cer -multi -checkend $[3600*24*365*4]; echo $?
Certificate will expire
Certificate will not expire
Certificate will expire
1

openssl x509 -in /tmp/fullchain.cer -multi -checkend $[3600*24*365*20]; echo $?
Certificate will expire
Certificate will expire
Certificate will expire
1

@DajanaV DajanaV mentioned this pull request Nov 15, 2025
Open
@snowdroppe snowdroppe force-pushed the fix-checkend branch 2 times, most recently from 1c6d6a8 to 48fb90f Compare November 16, 2025 11:47
@snowdroppe
Copy link
Contributor Author

snowdroppe commented Nov 16, 2025 •
edited
Loading

Force push to comply with styling guide k+1 < ... -> k < ... - 1

This was referenced Nov 16, 2025
Closed
Closed
beldmit
beldmit previously approved these changes Nov 17, 2025
View reviewed changes
Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

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

First, I'm not sure it's worth introducing this change.
To distinguish an error because of, say, wrong format, you still need to parse output.

Second, if this change is introduced, we need to document it

@snowdroppe
Copy link
Contributor Author

snowdroppe commented Nov 17, 2025

First, I'm not sure it's worth introducing this change. To distinguish an error because of, say, wrong format, you still need to parse output.

The current documentation reads Checks if the certificate expires within the next arg seconds and exits non-zero if yes it will expire or zero if not. which holds for versions prior to the implementation of -multi but is uniformly 0 after.

I agree that return codes may mask other errors and am happy to take the organisation's direction on how it should behave.

Second, if this change is introduced, we need to document it

Valid point as a lot of scripts currently depend on the return code to check for expired / near expired certs. I'll update it according to what is agreed.

Is #28928 a better thread to discuss the intent?

shahsb
shahsb reviewed Nov 19, 2025
View reviewed changes
Copy link
Contributor

@shahsb shahsb left a comment

Choose a reason for hiding this comment

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

Thanks @snowdroppe for your efforts on this!

It'll LGTM after the suggested fixes!

@snowdroppe
Copy link
Contributor Author

snowdroppe commented Nov 20, 2025

Pushed new commit which rebases to master and implements the above suggestions. Pending review, I will rebase again and squash into a single commit.

@snowdroppe snowdroppe force-pushed the fix-checkend branch 2 times, most recently from 55ddb4e to a61ba99 Compare November 22, 2025 18:41
@snowdroppe
Copy link
Contributor Author

snowdroppe commented Nov 22, 2025

I believe the PR is now in a fit state for review.
I have squashed changes into a single commit and rebased to master for easier merging.

Please let me know if there are any additional actions on me.
Also thanks to everyone for helping me contribute!

@snowdroppe
fix(x509.c): fixed -checkend return values
d0f2c06 
Fixes openssl#28928
Also adds functionality to -checkend to account for -multi behaviour.
Man page and unit tests updated accordingly.
@snowdroppe snowdroppe requested a review from beldmit November 22, 2025 19:23
@snowdroppe
Copy link
Contributor Author

snowdroppe commented Nov 23, 2025 •
edited
Loading

The unit tests trigger an overflow on the checkend time_t type i.e. year 2038 bug on some build platforms so I will need to revisit this. I'll update once I have a solution.

@snowdroppe
Copy link
Contributor Author

snowdroppe commented Nov 23, 2025

Fixed workflow issues:

  • doc-nits: non-zero -> nonzero
  • unit test:
    Previous -checkend overflowed time_t due to ~100 year delta.
    Now generates certs in the near future (200 days) to test against.

@snowdroppe
Copy link
Contributor Author

snowdroppe commented Nov 25, 2025 •
edited
Loading

@beldmit Could you kindly re-approve the workflow checks please? I believe I addressed the previous erros. Thanks!

@github-actions github-actions bot added severity: fips change The pull request changes FIPS provider sources severity: ABI change This pull request contains ABI changes labels Nov 25, 2025
@snowdroppe
Copy link
Contributor Author

snowdroppe commented Nov 28, 2025

@beldmit I believe the workflow checks are successful and this is ready to be considered for merge. Please let me know if there's anything else I should do.

The failing style check is unrelated to the changes within this PR:
providers/implementations/kdfs/pbkdf2.c:320:indent = 12 != 8

@beldmit beldmit added branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels Nov 28, 2025
beldmit
beldmit approved these changes Nov 28, 2025
View reviewed changes
Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

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

LGTM

@beldmit beldmit added the style: waived exempted from style checks label Nov 28, 2025
@t8m t8m added triaged: bug The issue/pr is/fixes a bug severity: regression The issue/pr is a regression from previous released version tests: exempted The PR is exempt from requirements for testing branch: 3.6 Applies to openssl-3.6 labels Nov 28, 2025
t8m
t8m approved these changes Nov 28, 2025
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

Great work @snowdroppe

@t8m
Copy link
Member

t8m commented Nov 28, 2025

This needs to go into 3.6 branch as well.

@t8m t8m added approval: done This pull request has the required number of approvals tests: present The PR has suitable tests present and removed approval: review pending This pull request needs review by a committer tests: exempted The PR is exempt from requirements for testing labels Nov 28, 2025
@openssl-machine 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 Nov 29, 2025
@openssl-machine
Copy link
Collaborator

openssl-machine commented Nov 29, 2025

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Dec 1, 2025
@snowdroppe @t8m
fix(x509.c): Fixed regression of openssl x509 -checkend return values
679a101 
Fixes #28928

Also adds functionality to -checkend to account for -multi behaviour.
Man page and unit tests updated accordingly.

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29155)
openssl-machine pushed a commit that referenced this pull request Dec 1, 2025
@snowdroppe @t8m
fix(x509.c): Fixed regression of openssl x509 -checkend return values
3d73337 
Fixes #28928

Also adds functionality to -checkend to account for -multi behaviour.
Man page and unit tests updated accordingly.

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29155)

(cherry picked from commit 679a101)
@t8m
Copy link
Member

t8m commented Dec 1, 2025

Squashed and merged to the master and 3.6 branches. Thank you for your contribution.

@t8m t8m closed this Dec 1, 2025
cxx194832 pushed a commit to cxx194832/openssl that referenced this pull request Dec 12, 2025
@snowdroppe @cxx194832
fix(x509.c): Fixed regression of openssl x509 -checkend return values
d311717 
Fixes openssl#28928

Also adds functionality to -checkend to account for -multi behaviour.
Man page and unit tests updated accordingly.

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#29155)
openssl-machine pushed a commit that referenced this pull request Jan 27, 2026
@esyr @t8m
CHANGES.md, NEWS.md: update for 3.6.1
6df5fdd 
3.6.1 CHANGES.md includes the following:
 * #28760
   "Improve the CPUINFO display for RISC-V"
 * #28797
   "Fix regression when X509_V_FLAG_CRL_CHECK_ALL is set"
 * #28955
   "Fix for TLS handshake issue with GnuTLS #28902"
 * #29155
   "fix(x509.c): fixed -checkend return values"
 * #29214
   "s390x: Check and fail on invalid malformed ECDSA signatures"
 * #29245
   "Clang format 3.6"
 * #29251
   "Fix change of behavior of the single stapled OCSP response API"

3.6.1 NEWS.md includes the following:
 * #28797
   "Fix regression when X509_V_FLAG_CRL_CHECK_ALL is set"
 * #28955
   "Fix for TLS handshake issue with GnuTLS #28902"

Co-Authored-by: Tomáš Mráz <[email protected]>
Signed-off-by: Eugene Syromiatnikov <[email protected]>

Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Neil Horman <[email protected]>
Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
MergeDate: Mon Jan 26 20:01:30 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@t8m t8m t8m approved these changes

@beldmit beldmit beldmit approved these changes

+2 more reviewers

@ArchangeGabriel ArchangeGabriel ArchangeGabriel left review comments

@shahsb shahsb shahsb left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch branch: 3.6 Applies to openssl-3.6 severity: ABI change This pull request contains ABI changes severity: fips change The pull request changes FIPS provider sources severity: regression The issue/pr is a regression from previous released version style: waived exempted from style checks tests: present The PR has suitable tests present triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

openssl x509 -checkend should not return 0 for expiring certificates

6 participants

@snowdroppe @t8m @openssl-machine @ArchangeGabriel @beldmit @shahsb