start valid date a day before to account for timezone mismatch #209
Conversation
Signed-off-by: Mikael Arguedas <[email protected]>
| @@ -87,7 +87,7 @@ def build_key_and_cert(subject_name, *, ca=False, ca_key=None, issuer_name=''): | |||
| ).serial_number( | |||
| x509.random_serial_number() | |||
| ).not_valid_before( | |||
| utcnow | |||
| utcnow - datetime.timedelta(days=1) | |||
There was a problem hiding this comment.
This feels hacky. Can we localize this? Even starting at midnight that day feels a little nicer, although is arguably just as arbitrary 😛 .
There was a problem hiding this comment.
That's literally how it was before the refactor #138 😉
I'm fine with more or less whatever alternative arbitrary value you prefer to that one though
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😇 .
There was a problem hiding this comment.
@mikaelarguedas what is the actual breakage observed in connext?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
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>^MTime on mini1 after running test_security tests
$ date
Wed May 6 15:02:58 PDT 2020Current time UTC after formatting the above text 22:05 https://www.timeanddate.com/worldclock/timezone/utc
There was a problem hiding this comment.
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
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
=======================================
Coverage 77.95% 77.95%
=======================================
Files 16 16
Lines 576 576
Branches 52 52
=======================================
Hits 449 449
Misses 107 107
Partials 20 20
Continue to review full report at Codecov.
|
|
As #205 broke nightly builds (smart guy didnt run CI on the PR thinking all tests were disabled :| ) and that nicer alternatives don't seem to fix the issue Below are some CI jobs (testing sros2 and test_security in Debug mode) 🤞
|
|
CI (testing just Not using ros2/ci#436 because the RTI version of openssl is installed on mini1 instead of osrf/homebrew-ros2#8
|
Signed-off-by: Mikael Arguedas <[email protected]>
apparently #205 broke Connext
Opening as draft for now pending testing and then CI