Add GeneralizedTime #492

DarkaMaul · 2024-10-25T09:53:00Z

Fixes #491

/cc @woodruffw

Signed-off-by: Alexis <alexis.challande@trailofbits.com>

src/types.rs

Signed-off-by: Alexis <alexis.challande@trailofbits.com>

src/types.rs

DarkaMaul · 2024-10-25T12:18:38Z

I've used a nanoseconds precision for the fractional time ( from 1 to 10^9) but I can't find a reference saying that the value is bounded (or not).

From T-REC-X.680-202102-I!!PDF-E.pdf, it seems the precision is arbitrary

2) a string representing the time of day to an accuracy of one hour, one minute, one second, or
fractions of a second (to any degree of accuracy), using either comma or full stop as the decimal sign (as
specified in ISO 8601, 4.2.2.2 and 4.2.2.3 – Basic format); optionally followed by:

And the X.690-0207 spec only says that the second is required:

11.7.2 The seconds element shall always be present.

However, the Quick ASN1 reference uses a 3 digit representation for the floating part.

What would be the best option here?

alex · 2024-10-25T12:24:01Z

Grumble. Nanosecond precision is fine. But this also means the prohibition on leading 0s is wrong, right?

…

On Fri, Oct 25, 2024 at 8:19 AM dm ***@***.***> wrote: I've used a nanoseconds precision for the fractional time ( from 1 to 10^9) but I can't find a reference saying that the value is bounded (or not). From T-REC-X.680-202102-I!!PDF-E.pdf <https://www.itu.int/rec/T-REC-X.680-202102-I>, it seems the precision is arbitrary 2) a string representing the time of day to an accuracy of one hour, one minute, one second, or fractions of a second (to any degree of accuracy), using either comma or full stop as the decimal sign (as specified in ISO 8601, 4.2.2.2 and 4.2.2.3 – Basic format); optionally followed by: And the X.690-0207 spec only says that the second is required: 11.7.2 The seconds element shall always be present. However, the Quick ASN1 reference <https://www.oss.com/asn1/resources/asn1-made-simple/asn1-quick-reference/generalizedtime.html> uses a 3 digit representation for the floating part. What would be the best option here? — Reply to this email directly, view it on GitHub <#492 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBCFKLSONPCZY5IQESTZ5IZLJAVCNFSM6AAAAABQS5DC5CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMZXGYZTGNRRGQ> . You are receiving this because you commented.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

Signed-off-by: Alexis <alexis.challande@trailofbits.com>

DarkaMaul · 2024-10-25T13:03:38Z

Grumble. Nanosecond precision is fine. But this also means the prohibition on leading 0s is wrong, right?

I think this one is still valid as it stems from :

11.7.3 The fractional-seconds elements, if present, shall omit all trailing zeros; if the elements correspond to 0, they
shall be wholly omitted, and the decimal point element also shall be omitted.

alex · 2024-10-25T13:05:01Z

Trailing and leading zeros are the opposite :-)

…

On Fri, Oct 25, 2024 at 9:04 AM dm ***@***.***> wrote: Grumble. Nanosecond precision is fine. But this also means the prohibition on leading 0s is wrong, right? I think this one is still valid as it stems from : 11.7.3 The fractional-seconds elements, if present, shall omit all trailing zeros; if the elements correspond to 0, they shall be wholly omitted, and the decimal point element also shall be omitted. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

DarkaMaul · 2024-10-25T13:11:50Z

Trailing and leading zeros are the opposite :-)
😮‍💨 Yes indeed - let me update the PR ...

Signed-off-by: Alexis <alexis.challande@trailofbits.com>

src/types.rs

Signed-off-by: Alexis <alexis.challande@trailofbits.com>

alex · 2024-10-25T15:32:10Z

Apologies in advance, but For simplicity of review, I think pulling the "rename GeneralizedTime" into its own PR probably is the best path.

README.md

Signed-off-by: William Woodruff <william@trailofbits.com>

alex · 2024-10-30T15:22:57Z

src/types.rs

@@ -914,7 +915,8 @@ fn push_four_digits(dest: &mut WriteBuf, val: u16) -> WriteResult {
 }

 /// A structure representing a (UTC timezone) date and time.
-/// Wrapped by `UtcTime` and `X509GeneralizedTime`.
+/// Wrapped by `UtcTime` and `X509GeneralizedTime` and used in
+/// `GeneralizedTime`.
 #[derive(Debug, Clone, PartialEq, Hash, Eq, PartialOrd)]
 pub struct DateTime {


Should DateTime just have an optional nanoseconds field?

Thoughts here?

Whoops, missed this earlier. I don't have a super strong opinion -- I think @DarkaMaul put the nanos on GeneralizedTime since UTCTime has no fractional time support at all, so having it on the top-level DateTime used by both seems (slightly) off.

OTOH having it on GeneralizedTime also seems slightly off, since it's now (DateTime, nanos). So I'm happy to move if you'd prefer.

Eh, let's get the docs right on the generalized tiem strcut and that'll be good enough. (See my other comment)

src/types.rs

Signed-off-by: William Woodruff <william@trailofbits.com>

Keeps us in a stack buf. Signed-off-by: William Woodruff <william@trailofbits.com>

Signed-off-by: William Woodruff <william@trailofbits.com>

alex

One last open question

alex · 2024-11-03T20:20:38Z

src/types.rs

@@ -914,7 +915,8 @@ fn push_four_digits(dest: &mut WriteBuf, val: u16) -> WriteResult {
 }

 /// A structure representing a (UTC timezone) date and time.
-/// Wrapped by `UtcTime` and `X509GeneralizedTime`.
+/// Wrapped by `UtcTime` and `X509GeneralizedTime` and used in
+/// `GeneralizedTime`.
 #[derive(Debug, Clone, PartialEq, Hash, Eq, PartialOrd)]
 pub struct DateTime {


Thoughts here?

alex · 2024-11-03T20:29:50Z

src/types.rs

@@ -1088,6 +1089,125 @@ impl SimpleAsn1Writable for X509GeneralizedTime {
    }
 }

+/// Used for parsing and writing ASN.1 `GENERALIZED TIME` values accepting
+/// fractional seconds value.
+/// See https://github.com/alex/rust-asn1/issues/491 for discussion.


This comment should be updated to just describe the type, not reference an issue about how things used to be.

alex · 2024-11-03T20:30:13Z

src/types.rs

@@ -914,7 +915,8 @@ fn push_four_digits(dest: &mut WriteBuf, val: u16) -> WriteResult {
 }

 /// A structure representing a (UTC timezone) date and time.
-/// Wrapped by `UtcTime` and `X509GeneralizedTime`.
+/// Wrapped by `UtcTime` and `X509GeneralizedTime` and used in
+/// `GeneralizedTime`.
 #[derive(Debug, Clone, PartialEq, Hash, Eq, PartialOrd)]
 pub struct DateTime {


Eh, let's get the docs right on the generalized tiem strcut and that'll be good enough. (See my other comment)

src/types.rs

Add GeneralizedTimeFractional

c0c2676

Signed-off-by: Alexis <alexis.challande@trailofbits.com>

DarkaMaul commented Oct 25, 2024

View reviewed changes

src/types.rs Outdated Show resolved Hide resolved

Fix linting

48cfa03

Signed-off-by: Alexis <alexis.challande@trailofbits.com>

alex reviewed Oct 25, 2024

View reviewed changes

DarkaMaul added 3 commits October 25, 2024 14:49

Apply suggestions from code review

f4636be

Signed-off-by: Alexis <alexis.challande@trailofbits.com>

Add missing import

b5c8036

Signed-off-by: Alexis <alexis.challande@trailofbits.com>

Exercise precision too high

c4bc6fe

Signed-off-by: Alexis <alexis.challande@trailofbits.com>

Correctly handle fractional values

bf810c8

Signed-off-by: Alexis <alexis.challande@trailofbits.com>

woodruffw reviewed Oct 25, 2024

View reviewed changes

src/types.rs Outdated Show resolved Hide resolved

Use more idiomatic Rust

031bfb4

Signed-off-by: Alexis <alexis.challande@trailofbits.com>

DarkaMaul force-pushed the dm/add-generalized-time-fractional branch from ace99a6 to 031bfb4 Compare October 25, 2024 15:35

DarkaMaul mentioned this pull request Oct 25, 2024

DRAFT: Rename GeneralizedTime #493

Closed

DarkaMaul added 2 commits October 29, 2024 15:57

Merge branch 'main' into dm/add-generalized-time-fractional

8efe577

Lint

7685207

DarkaMaul changed the title ~~Add GeneralizedTimeFractional~~ Add GeneralizedTime Oct 29, 2024

Add CHANGELOG entry

aad351c

DarkaMaul commented Oct 29, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

README: rephrase change

5a9b142

Signed-off-by: William Woodruff <william@trailofbits.com>

alex reviewed Oct 30, 2024

View reviewed changes

woodruffw added 3 commits October 30, 2024 11:54

types: remove old comment

1ad4676

Signed-off-by: William Woodruff <william@trailofbits.com>

use itoa for formatting

6791f17

Keeps us in a stack buf. Signed-off-by: William Woodruff <william@trailofbits.com>

make clippy happy

9096ea1

Signed-off-by: William Woodruff <william@trailofbits.com>

alex reviewed Nov 3, 2024

View reviewed changes

woodruffw reviewed Nov 3, 2024

View reviewed changes

src/types.rs Outdated Show resolved Hide resolved

Update src/types.rs

dbeb462

alex approved these changes Nov 3, 2024

View reviewed changes

alex merged commit e37fa65 into alex:main Nov 3, 2024
12 checks passed

woodruffw deleted the dm/add-generalized-time-fractional branch November 3, 2024 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GeneralizedTime #492

Add GeneralizedTime #492

DarkaMaul commented Oct 25, 2024

DarkaMaul commented Oct 25, 2024

alex commented Oct 25, 2024 via email

DarkaMaul commented Oct 25, 2024

alex commented Oct 25, 2024 via email

DarkaMaul commented Oct 25, 2024

alex commented Oct 25, 2024

alex Oct 30, 2024

alex Nov 3, 2024

woodruffw Nov 3, 2024

alex Nov 3, 2024

alex left a comment

alex Nov 3, 2024

alex Nov 3, 2024

woodruffw Nov 3, 2024

alex Nov 3, 2024

Add GeneralizedTime #492

Add GeneralizedTime #492

Conversation

DarkaMaul commented Oct 25, 2024

DarkaMaul commented Oct 25, 2024

alex commented Oct 25, 2024 via email

DarkaMaul commented Oct 25, 2024

alex commented Oct 25, 2024 via email

DarkaMaul commented Oct 25, 2024

alex commented Oct 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment