Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apps: req/x509 add explicit start and end date #21716

Conversation

Atomisirsi
Copy link
Contributor

@Atomisirsi Atomisirsi commented Aug 10, 2023

This feature adds options to explicitly set the start and end dates in X509v3 certificate creation with the req and x509 apps.
I stumbled upon this several times now, and I think this adds some convenience to the apps.

For the req app, I borrowed the naming from the ca app, hence the parameters are called -startdate and -enddate. I also added the same handling for default_startdate and default_enddate to the respective configuration file section.
The -enddate option conflicts with the -days option, hence only one of both can be used. If a start date is given, but no end date, the -days option is processed instead. Currently, this results in an offset of days from the current date, not from the start date.

For the x509 app, I had to select different names, as -startdate and -enddate are already in use to print the notBefore and notAfter fields of a certificate. Therefore, I called the options -notbefore and -notafter. The handling in respect to the -days option is similar to the req app, but no configuration file parameters exist.

Changes:

  • removed new configuration options from req app.
  • renamed options -startdate and -enddate to -not_before and -not_after in req app.
  • added options -not_before and -not_after also to x509 app.
  • Ensure not_after >= not_before in both apps req and x509.
  • added aliases to ca app: -not_before -> -startdate, -not_after -> -enddate.
  • added helper function check_cert_time_string to lib/apps.
  • moved redundant code to helper function check_cert_times.
  • Improved docs.
Checklist
  • documentation is added or updated
  • tests are added or updated

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Aug 10, 2023
apps/x509.c Outdated Show resolved Hide resolved
apps/req.c Outdated Show resolved Hide resolved
apps/req.c Outdated Show resolved Hide resolved
apps/req.c Outdated Show resolved Hide resolved
Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

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

Is notafter < notbefore allowed..
Same question applies to days and notbefore.

@Atomisirsi
Copy link
Contributor Author

Is notafter < notbefore allowed.. Same question applies to days and notbefore.

Yes, there's currently no check to ensure this. Will add one to req app in next commit.

@Atomisirsi Atomisirsi force-pushed the feature/apps-req-x509-add-explicit-startdate-enddate branch 2 times, most recently from 6fa9038 to 2b2b2f4 Compare August 15, 2023 23:06
@Atomisirsi
Copy link
Contributor Author

Closing and reopening to pass cla-check

@Atomisirsi Atomisirsi closed this Aug 16, 2023
@Atomisirsi Atomisirsi reopened this Aug 16, 2023
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Aug 16, 2023
@Atomisirsi Atomisirsi force-pushed the feature/apps-req-x509-add-explicit-startdate-enddate branch from 2b2b2f4 to 859b0f0 Compare August 17, 2023 06:52
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Aug 28, 2023
@t8m t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member tests: deferred Tests will be added in a subsequent PR (label should be removed when the PR with tests is merged) and removed approval: otc review pending This pull request needs review by an OTC member labels Aug 28, 2023
@t8m
Copy link
Member

t8m commented Aug 28, 2023

It would be nice to have some tests for these new options.

@Atomisirsi Atomisirsi force-pushed the feature/apps-req-x509-add-explicit-startdate-enddate branch from 859b0f0 to 276f844 Compare September 23, 2023 12:03
@Atomisirsi
Copy link
Contributor Author

It would be nice to have some tests for these new options.

I will try to get some tests ready!

@openssl-machine
Copy link
Collaborator

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

@t8m t8m added this to the Post 3.2.0 milestone Oct 26, 2023
@DDvO
Copy link
Contributor

DDvO commented Oct 27, 2023

I will try to get some tests ready!

When do you think you can do that?
I meanwhile noticed that you were waiting for a confirmation on the _ added to the option names.
I'd say you can meanwhile see this as confirmed :)

@DDvO
Copy link
Contributor

DDvO commented Oct 27, 2023

Meanwhile there is a (trivial) merge conflict in CHANGES.md.

Anyway, the new text there meanwhile will need to be moved to an upcoming section on changes to version 3.3
since this nice little feature unfortunately did not make it into 3.2.

Copy link
Contributor

@DDvO DDvO left a comment

Choose a reason for hiding this comment

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

Nice work.
Just a couple of small further improvements suggested.
Plus the other changes and test extensions already planned for.

CHANGES.md Outdated Show resolved Hide resolved
apps/req.c Outdated Show resolved Hide resolved
apps/req.c Outdated Show resolved Hide resolved
apps/req.c Outdated Show resolved Hide resolved
apps/req.c Outdated Show resolved Hide resolved
apps/x509.c Outdated Show resolved Hide resolved
apps/x509.c Outdated Show resolved Hide resolved
apps/x509.c Outdated Show resolved Hide resolved
@Atomisirsi
Copy link
Contributor Author

Very nice contribution!

Thank you very much for your patience and the great feedback!

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.

Good work! Just a few nits remaining.

CHANGES.md Outdated Show resolved Hide resolved
apps/lib/apps.c Outdated Show resolved Hide resolved
apps/ca.c Outdated Show resolved Hide resolved
apps/lib/apps.c Outdated Show resolved Hide resolved
apps/x509.c Outdated Show resolved Hide resolved
apps/req.c Show resolved Hide resolved
Copy link
Contributor

@DDvO DDvO left a comment

Choose a reason for hiding this comment

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

Version number and style nits are fixed.

@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: otc review pending This pull request needs review by an OTC member approval: done This pull request has the required number of approvals labels Apr 5, 2024
@t8m t8m closed this Apr 5, 2024
@t8m t8m reopened this Apr 5, 2024
@t8m
Copy link
Member

t8m commented Apr 5, 2024

CI is relevant - the number of tests in test_req is already bumped to 108 in current master branch. Please rebase this again and bump the number to 109. I am sorry for this hassle.

- Added options `-not_before` (start date) and `-not-after` (end date)
  for explicit setting of the validity period of a certificate in the
  apps `ca`, `req` and `x509`
- The new options accept time strings or "today"
- In app `ca`, use the new options as aliases of the already existing
  options `-startdate` and `-enddate`
- When used in apps `req` and `x509`, the end date must be >= the start
  date, in app `ca` end date < start date is also accepted
- In any case, `-not-after` overrides the `-days` option
- Added helper function `check_cert_time_string` to validate given
  certificate time strings
- Use the new helper function in apps `ca`, `req` and `x509`
- Moved redundant code for time string checking into `set_cert_times`
  helper function.
- Added tests for explicit start and end dates in apps `req` and `x509`
- test: Added auxiliary functions for parsing fields from `-text`
  formatted output to `tconversion.pl`
- CHANGES: Added to new section 3.4

Signed-off-by: Stephan Wurm <atomisirsi@gsklan.de>
25-test_req.t: Increment number of planned tests

Signed-off-by: Stephan Wurm <atomisirsi@gsklan.de>
@Atomisirsi Atomisirsi force-pushed the feature/apps-req-x509-add-explicit-startdate-enddate branch from 7be6702 to 1d7582b Compare April 5, 2024 15:34
@Atomisirsi Atomisirsi requested a review from t8m April 5, 2024 15:40
@t8m t8m added the approval: done This pull request has the required number of approvals label Apr 5, 2024
@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 Apr 6, 2024
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member

t8m commented Apr 9, 2024

Merged to the master branch. Thank you for your contribution.

@t8m t8m closed this Apr 9, 2024
openssl-machine pushed a commit that referenced this pull request Apr 9, 2024
- Added options `-not_before` (start date) and `-not-after` (end date)
  for explicit setting of the validity period of a certificate in the
  apps `ca`, `req` and `x509`
- The new options accept time strings or "today"
- In app `ca`, use the new options as aliases of the already existing
  options `-startdate` and `-enddate`
- When used in apps `req` and `x509`, the end date must be >= the start
  date, in app `ca` end date < start date is also accepted
- In any case, `-not-after` overrides the `-days` option
- Added helper function `check_cert_time_string` to validate given
  certificate time strings
- Use the new helper function in apps `ca`, `req` and `x509`
- Moved redundant code for time string checking into `set_cert_times`
  helper function.
- Added tests for explicit start and end dates in apps `req` and `x509`
- test: Added auxiliary functions for parsing fields from `-text`
  formatted output to `tconversion.pl`
- CHANGES: Added to new section 3.4

Signed-off-by: Stephan Wurm <atomisirsi@gsklan.de>

Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #21716)
@Atomisirsi Atomisirsi deleted the feature/apps-req-x509-add-explicit-startdate-enddate branch April 10, 2024 14:17
vdukhovni pushed a commit to vdukhovni/openssl that referenced this pull request Apr 15, 2024
The tests used localtime to format "today's" date, but then extracted a
GMT date from the cert.  The comparison breaks when run late in the
evening west of UTC, or early in the AM hours east of UTC.

Also took care of case when test runs at stroke of midnight, by
accepting either the "today" before the cert creation, or the
"today" after, should they be different.

Fixes fragile tests in openssl#21716
vdukhovni pushed a commit that referenced this pull request Apr 16, 2024
The tests used localtime to format "today's" date, but then extracted a
GMT date from the cert.  The comparison breaks when run late in the
evening west of UTC, or early in the AM hours east of UTC.

Also took care of case when test runs at stroke of midnight, by
accepting either the "today" before the cert creation, or the
"today" after, should they be different.

Fixes fragile tests in #21716
openssl-machine pushed a commit that referenced this pull request Apr 18, 2024
The tests used localtime to format "today's" date, but then extracted a
GMT date from the cert.  The comparison breaks when run late in the
evening west of UTC, or early in the AM hours east of UTC.

Also took care of case when test runs at stroke of midnight, by
accepting either the "today" before the cert creation, or the
"today" after, should they be different.

Fixes fragile tests in #21716

Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24139)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch tests: present The PR has suitable tests present triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants