Add support for datetime format ISO 8601 #14384

wcedmisten · 2021-03-02T00:19:28Z

Use datetime format ISO 8601 instead of ASN.1. Closes #5430

I expect at least one reviewer comment will address refactoring, since many of the code structures specifically reference ASN.1 I did not know what level to refactor these, so I left them as-is, but can return to refactor them if desired. Specific input would be greatly appreciated. Thanks!

Checklist

documentation is added or updated
tests are added or updated

slontis · 2021-03-02T01:01:50Z

@openssl/otc Should this be a configurable option?

paulidale · 2021-03-02T01:24:09Z

Possibly, I worry that it might break users...

vdukhovni · 2021-03-02T01:37:45Z

Possibly, I worry that it might break users...

Changing the default format would likely be disruptive. Offering a way to request a non-default format seems more reasonable.

levitte · 2021-03-02T04:44:38Z

We do have a nameopt, should we have a dateopt?

slontis · 2021-03-02T04:51:41Z

I was thinking at config time
otherwise we end up with a 'asn1_time_print_ex2'

levitte · 2021-03-02T05:09:45Z

I was only pointing out that we do have a mechanism for selecting output formats, and that we could do the same again for dates (which would also be consistent).

Nameopts aren't configurable, btw, but there's nothing stopping us, and the same could go for dateopts. Making configuration the only viable choice to ask for a different format, though?

wcedmisten · 2021-03-02T05:15:40Z

Possibly, I worry that it might break users...

Changing the default format would likely be disruptive. Offering a way to request a non-default format seems more reasonable.

Just out of curiosity, do you know of down stream examples that would be broken by this? I don't want to disrupt existing use cases, but my understanding was this date output was primarily for humans to read.

I would prefer setting ISO 8601 as the default format because I myself was at one point confused by misreading the existing datetime format, and thought switching might be clearer to a human reader (although that is somewhat subjective). I certainly don't want to break existing code written around this format, but hopefully I've made my case for altering the default.

I'll defer to your judgement in weighing the costs and benefits of changing the default.

levitte · 2021-03-02T05:32:55Z

Out of historical interest, I believe that the current date output is inspired by RFC 822. That date format there was quite popular back in the days (still is, just look at the default git log output), and was considered very humanly readable.

I'll defer to your judgement in weighing the costs and benefits of changing the default.

It seems like you got a few brains going 😉

richsalz · 2021-03-02T14:35:00Z

I think output is much less of a problem than input, which has several issues (mostly copied over from the old RT system).

As for "who might break," the project has generally said it has no idea how it is used. Just the other day there was an issue about certificate output in tests for example.

Is there a real problem this is solving beyond closing an issue that has lain fallow for three years?

Please note that I am completely in favor of the ISO format. But please not for this release.

richsalz · 2021-03-02T14:35:38Z

And in a separate comment: I have scripts that will break if the format is changed. They are used to help prune our trust stores.

levitte · 2021-03-02T14:48:44Z

I have scripts that will break if the format is changed. They are used to help prune our trust stores.

Yup, ramifications...

wcedmisten · 2021-03-02T15:00:05Z

Is there a real problem this is solving beyond closing an issue that has lain fallow for three years?

Mostly my belief that ISO 8601 is the superior format, and some minor frustration as a new user around the existing format. That being said, I understand the value of preserving existing behavior.

Please note that I am completely in favor of the ISO format. But please not for this release.

It sounds like the consensus I'm hearing is to allow for ISO as a non-default, configurable option, which I'm happy to implement. That would still leave open the option to migrate formats in the future (which would be my personal preference).

openssl-machine · 2021-04-02T00:07:26Z

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

levitte · 2021-04-02T06:51:02Z

It sounds like the consensus I'm hearing is to allow for ISO as a non-default, configurable option, which I'm happy to implement. That would still leave open the option to migrate formats in the future (which would be my personal preference).

Yes, that would be welcome

openssl-machine · 2021-05-03T00:07:25Z

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

t8m · 2021-05-03T08:23:11Z

This is actually waiting on making the ISO format an optional thing.

slontis · 2021-05-04T00:33:49Z

Just to reiterate - ISO format will NOT be the default option (maybe in the future that might change), so the changes entry would need to indicate the new optional behavior.

wcedmisten · 2021-05-04T00:44:05Z

We're on the same page, trying to implement the optional configuration now. Sorry, I haven't had much time to work on this recently but will try to add that this week.

…

On Mon, May 3, 2021, 8:34 PM Shane ***@***.***> wrote: Just to reiterate - ISO format will NOT be the default option (maybe in the future that might change), so the changes entry would need to indicate the new optional behavior. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#14384 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABC2RUG5I2JNDABPJZGWZ6LTL46H3ANCNFSM4YNT4FDQ> .

mattcaswell · 2021-05-04T08:45:56Z

OTC: Hold lifted assuming that the default behaviour is not changed.

richsalz · 2021-05-04T14:43:32Z

Other places to look #205 (!!!), #6985, #5430. There are probably others.

CHANGES.md

apps/crl.c

wcedmisten · 2021-06-08T20:21:09Z

The check_docs failure is relevant. There is no -dateopt but there is the ENV_DATEOPT environment variable.

That should be documented in doc/man7/openssl-env.pod And when you look at the other variables there, you should come to the conflusion that it should be OPENSSL_DATEOPT for the variable name. :)

I believe this was actually a missing underscore in -date_opt, rather than being an environment variable, but let me know if I'm mistaken. I tried to model the structure after -cert_opt and -name_opt. This should be fixed now, make doc-nits runs without error locally.

richsalz · 2021-06-08T21:29:07Z

And you'll rename the env var to match the other openssl ones? And document it as mentioned above?

wcedmisten · 2021-06-08T22:32:02Z

And you'll rename the env var to match the other openssl ones? And document it as mentioned above?

Ah, I see what I did wrong. I think this should be an OPT_DATEOPT and not an ENV_DATEOPT in ca.c. I'll fix that.

wcedmisten · 2021-06-09T21:33:48Z

Changed ENV_DATEOPT to OPT_DATEOPT

include/crypto/asn1.h

Fixes openssl#5430 Added the configuration file option "date_opt" to the openssl applications ca, crl and x509. Added ASN1_TIME_print_ex which supports the new datetime format using the flag ASN1_DTFLGS_ISO8601

slontis · 2021-06-10T01:22:02Z

Unless you need to rebase, when you are addressing review comments you dont need to force push if you use

git add blarg...
git commit --fixup commitid
(Use git log to get the commitid of the commit you are trying to fixup).
git push

Then any reviewer can see what you have changed without having to re-review all files.

slontis · 2021-06-10T01:23:23Z

ping

richsalz · 2021-06-10T12:59:33Z

apps/ca.c

@@ -99,7 +99,7 @@ static int certify(X509 **xret, const char *infile, int informat,
                   const char *enddate,
                   long days, int batch, const char *ext_sect, CONF *conf,
                   int verbose, unsigned long certopt, unsigned long nameopt,
-                   int default_op, int ext_copy, int selfsign);
+                   int default_op, int ext_copy, int selfsign, unsigned long dateopt);


Can we not add to the "unsigned long" tech debt? See #15231 Or maybe the project doesn't care.

openssl-machine · 2021-06-11T10:00:20Z

This pull request is ready to merge

Fixes #5430 Added the configuration file option "date_opt" to the openssl applications ca, crl and x509. Added ASN1_TIME_print_ex which supports the new datetime format using the flag ASN1_DTFLGS_ISO8601 Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Shane Lontis <shane.lontis@oracle.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #14384)

t8m · 2021-06-11T10:44:59Z

Merged to master. Thank you for the contribution!

Fixes openssl#5430 Added the configuration file option "date_opt" to the openssl applications ca, crl and x509. Added ASN1_TIME_print_ex which supports the new datetime format using the flag ASN1_DTFLGS_ISO8601 Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Shane Lontis <shane.lontis@oracle.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#14384)

t8m changed the title ~~Wedmisten use iso 8601~~ Use datetime format ISO 8601 instead of ASN.1 Mar 2, 2021

t8m added hold: need otc decision The OTC needs to make a decision branch: master Merge to master branch labels Mar 2, 2021

t8m added the stalled: awaiting contributor response This pull request is awaiting a response by the contributor label May 3, 2021

mattcaswell removed the hold: need otc decision The OTC needs to make a decision label May 4, 2021

t8m removed the stalled: awaiting contributor response This pull request is awaiting a response by the contributor label May 4, 2021

wcedmisten changed the title ~~Use datetime format ISO 8601 instead of ASN.1~~ Draft: Use datetime format ISO 8601 instead of ASN.1 May 6, 2021

slontis reviewed May 7, 2021

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

slontis reviewed May 7, 2021

View reviewed changes

apps/crl.c Outdated Show resolved Hide resolved

slontis reviewed May 7, 2021

View reviewed changes

apps/crl.c Outdated Show resolved Hide resolved

wcedmisten force-pushed the wedmisten_use_iso_8601 branch 3 times, most recently from bdf32e9 to 36d0b2a Compare June 8, 2021 20:17

wcedmisten force-pushed the wedmisten_use_iso_8601 branch 2 times, most recently from 57113df to 494c4ca Compare June 9, 2021 21:22

slontis reviewed Jun 9, 2021

View reviewed changes

include/crypto/asn1.h Show resolved Hide resolved

slontis self-requested a review June 9, 2021 22:49

slontis added approval: otc review pending This pull request needs review by an OTC member and removed approval: review pending This pull request needs review by a committer labels Jun 9, 2021

Add support for ISO 8601 datetime format

6d05966

Fixes openssl#5430 Added the configuration file option "date_opt" to the openssl applications ca, crl and x509. Added ASN1_TIME_print_ex which supports the new datetime format using the flag ASN1_DTFLGS_ISO8601

wcedmisten force-pushed the wedmisten_use_iso_8601 branch from 494c4ca to 6d05966 Compare June 9, 2021 23:02

slontis approved these changes Jun 10, 2021

View reviewed changes

t8m approved these changes Jun 10, 2021

View reviewed changes

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 labels Jun 10, 2021

paulidale approved these changes Jun 10, 2021

View reviewed changes

richsalz reviewed Jun 10, 2021

View reviewed changes

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 Jun 11, 2021

t8m closed this Jun 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for datetime format ISO 8601 #14384

Add support for datetime format ISO 8601 #14384

wcedmisten commented Mar 2, 2021

slontis commented Mar 2, 2021

paulidale commented Mar 2, 2021

vdukhovni commented Mar 2, 2021

levitte commented Mar 2, 2021

slontis commented Mar 2, 2021

levitte commented Mar 2, 2021

wcedmisten commented Mar 2, 2021

levitte commented Mar 2, 2021

richsalz commented Mar 2, 2021

richsalz commented Mar 2, 2021

levitte commented Mar 2, 2021

wcedmisten commented Mar 2, 2021

openssl-machine commented Apr 2, 2021

levitte commented Apr 2, 2021

openssl-machine commented May 3, 2021

t8m commented May 3, 2021

slontis commented May 4, 2021

wcedmisten commented May 4, 2021 via email

mattcaswell commented May 4, 2021

richsalz commented May 4, 2021

wcedmisten commented Jun 8, 2021 •

edited

richsalz commented Jun 8, 2021

wcedmisten commented Jun 8, 2021

wcedmisten commented Jun 9, 2021

slontis commented Jun 10, 2021

slontis commented Jun 10, 2021

richsalz Jun 10, 2021

openssl-machine commented Jun 11, 2021

t8m commented Jun 11, 2021

Add support for datetime format ISO 8601 #14384

Add support for datetime format ISO 8601 #14384

Conversation

wcedmisten commented Mar 2, 2021

Checklist

slontis commented Mar 2, 2021

paulidale commented Mar 2, 2021

vdukhovni commented Mar 2, 2021

levitte commented Mar 2, 2021

slontis commented Mar 2, 2021

levitte commented Mar 2, 2021

wcedmisten commented Mar 2, 2021

levitte commented Mar 2, 2021

richsalz commented Mar 2, 2021

richsalz commented Mar 2, 2021

levitte commented Mar 2, 2021

wcedmisten commented Mar 2, 2021

openssl-machine commented Apr 2, 2021

levitte commented Apr 2, 2021

openssl-machine commented May 3, 2021

t8m commented May 3, 2021

slontis commented May 4, 2021

wcedmisten commented May 4, 2021 via email

mattcaswell commented May 4, 2021

richsalz commented May 4, 2021

wcedmisten commented Jun 8, 2021 • edited

richsalz commented Jun 8, 2021

wcedmisten commented Jun 8, 2021

wcedmisten commented Jun 9, 2021

slontis commented Jun 10, 2021

slontis commented Jun 10, 2021

richsalz Jun 10, 2021

Choose a reason for hiding this comment

openssl-machine commented Jun 11, 2021

t8m commented Jun 11, 2021

wcedmisten commented Jun 8, 2021 •

edited