Skip to content

start valid date a day before to account for timezone mismatch #209

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.

Sign up for GitHub

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

Merged
mikaelarguedas merged 2 commits into from
May 7, 2020
Merged

start valid date a day before to account for timezone mismatch #209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
40b2b8a
start valid date a day before to account for timezone mismatch
mikaelarguedas May 6, 2020
71cac20
add comment to justify the hack
mikaelarguedas May 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion sros2/sros2/api/_utilities.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,10 @@ def build_key_and_cert(subject_name, *, ca=False, ca_key=None, issuer_name=''):
).serial_number(
x509.random_serial_number()
).not_valid_before(
utcnow
# Using a day earlier here to prevent Connext (5.3.1) from complaining
# when extracting it from the permissions file and thinking it's in the future
# https://github.com/ros2/ci/pull/436#issuecomment-624874296
utcnow - datetime.timedelta(days=1)
Copy link
Member

@kyrofa kyrofa May 6, 2020

Choose a reason for hiding this comment

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

This feels hacky. Can we localize this? Even starting at midnight that day feels a little nicer, although is arguably just as arbitrary 😛 .

Copy link
Member Author

@mikaelarguedas mikaelarguedas May 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That's literally how it was before the refactor #138 😉

https://github.com/ros2/sros2/blame/9f22e40b25b7c1486fbbc6cfdfdb19267ba3e05b/sros2/sros2/api/__init__.py#L236

I'm fine with more or less whatever alternative arbitrary value you prefer to that one though

Copy link
Member

@ruffsl ruffsl May 6, 2020

Choose a reason for hiding this comment

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

I'm against this hack on principle.
Folks should really fix/synchronize there time sources.
I'm not sure what localizing it would do, given timezones are always changing in terms of geography and UTC offset dates.

Copy link
Member

@kyrofa kyrofa May 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That's literally how it was before the refactor #138 😉

Ha! Some genius thought it would be a good idea to throw away that hack 😇 .

Copy link
Member

@kyrofa kyrofa May 6, 2020

Choose a reason for hiding this comment

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

@mikaelarguedas what is the actual breakage observed in connext?

Copy link
Member Author

@mikaelarguedas mikaelarguedas May 6, 2020

Choose a reason for hiding this comment

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

Ha! Some genius thought it would be a good idea to throw away that hack innocent .

At least your consistent in your perception of the hack 😉

what is the actual breakage observed in connext?

Oh my bad, it came up here ros2/ci#436 (comment)

Copy link
Member Author

@mikaelarguedas mikaelarguedas May 6, 2020

Choose a reason for hiding this comment

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

I'm against this hack on principle.
Folks should really fix/synchronize there time sources.

I'm in agreement with that.

The one thing that's not clear to me is if it's a machine time sync issue or a connext issue that's not parsing it correctly.
The spec seems pretty clear about it though:

<!-- Format is CCYY-MM-DDThh:mm:ss[Z|(+|-)hh:mm] The time zone may
be specified as Z (UTC) or (+|-)hh:mm. Time zones that aren't
specified are considered UTC.
-->
<not_before>2013-10-26T00:00:00</not_before>
<not_after>2018-10-26T22:45:30</not_after>

Copy link
Contributor

@sloretz sloretz May 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The one thing that's not clear to me is if it's a machine time sync issue or a connext issue that's not parsing it correctly.

Time seems ok. If it's off, it couldn't be more than a few minutes.

Time in permissions.p7s (time when I built test_security?)

      <validity>^M
        <not_before>2020-05-05T21:57:05</not_before>^M
        <not_after>2030-05-04T21:57:05</not_after>^M
      </validity>^M

Time on mini1 after running test_security tests

$ date
Wed May  6 15:02:58 PDT 2020

Current time UTC after formatting the above text 22:05 https://www.timeanddate.com/worldclock/timezone/utc

Copy link
Member Author

@mikaelarguedas mikaelarguedas May 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

thanks for the info, this seems consistent with the idea that Connext is not parsing it properly and interpreting it as local time and not UTC.

If your clock was off, the date in the certificate will be off and the one extracted from the certificate and copied into the permission file would be off too. So I think it rules out a "time sync" issue.

We can find a nicer fix than this. I can try a thing or 2 tomorrow. If this fix the MacOS jobs I'm fine merging this and #205 for now

).not_valid_after(
# TODO: This should not be hard-coded
utcnow + datetime.timedelta(days=3650)
Expand Down
6 changes: 5 additions & 1 deletion sros2/test/sros2/commands/security/verbs/test_create_key.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,11 @@ def test_cert_pem(enclave_keys_dir):

# Verify the cert is valid for the expected timespan
utcnow = datetime.datetime.utcnow()
assert _datetimes_are_close(cert.not_valid_before, utcnow)

# Using a day earlier here to prevent Connext (5.3.1) from complaining
# when extracting it from the permissions file and thinking it's in the future
# https://github.com/ros2/ci/pull/436#issuecomment-624874296
assert _datetimes_are_close(cert.not_valid_before, utcnow - datetime.timedelta(days=1))
assert _datetimes_are_close(cert.not_valid_after, utcnow + datetime.timedelta(days=3650))

# Verify that the cert ensures this key cannot be used to sign others as a CA
Expand Down