Timezone aware datetimes + remove hack from #209 by mikaelarguedas · Pull Request #300 · ros2/sros2

mikaelarguedas · 2024-05-07T08:13:40Z

This PR (basically a redo of #210) does:

Set timezone to UTC explicitly in all security artifacts
- this allows to remove the -1 day hack from start valid date a day before to account for timezone mismatch #209
- I don't think it fixes any kind of issue and would assume the issue have been fixed on the connext side in version 6
- But I think it's more explicit and doesnt hurt
Make python datetime object timezone-aware, as this will be mandatory in future versions of python and reduces unexpected behaviors

I dont have a connext setup (license plugins etc right now) and the ROS 2 CI machines seem to be UTC so maybe they wont reflect the issue. If a reviewer that is behind UTC and has the right setup can give it a try that'd be awesome

mikaelarguedas · 2024-05-07T09:49:09Z

@marcoag @clalancette is it possible to launch a round of CI for this PR please 🙏

ahcorde · 2024-05-07T10:01:20Z

Linux
Linux-aarch64
Windows

sros2/sros2/_utilities.py

sros2/test/sros2/commands/security/verbs/test_create_enclave.py

mikaelarguedas · 2024-05-07T11:11:30Z

@ruffsl interested in having your feedback on this.

(all info in the description of the PR)

mikaelarguedas · 2024-05-07T11:29:57Z

CI on WIndows is failing because CI is using a lower version of Python than the one defined in REP-2000.
More details at ros-infrastructure/ros2-cookbooks#69

clalancette · 2024-05-07T12:24:39Z

CI on WIndows is failing because CI is using a lower version of Python than the one defined in REP-2000.
More details at ros-infrastructure/ros2-cookbooks#69

I commented over there, but in short, we are not going to be able to update Python. So we'll have to figure out another solution.

codecov · 2024-05-08T14:01:01Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.38%. Comparing base (1d36b11) to head (df3fdee).
⚠️ Report is 1 commits behind head on rolling.

Additional details and impacted files

@@             Coverage Diff             @@
##           rolling     #300      +/-   ##
===========================================
- Coverage    88.94%   88.38%   -0.56%     
===========================================
  Files           24       24              
  Lines          615      620       +5     
  Branches        64       66       +2     
===========================================
+ Hits           547      548       +1     
- Misses          50       52       +2     
- Partials        18       20       +2

Flag	Coverage Δ
unittests	`88.38% <ø> (-0.56%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mikaelarguedas · 2024-05-08T14:01:20Z

~~@ahcorde @clalancette is it possible to launch CI on that version to see if Windows is happy with it ?~~

mikaelarguedas · 2024-05-08T15:25:10Z

Linux
Linux-aarch64
Windows
Windows debug

ruffsl · 2024-05-09T01:54:29Z

sros2/sros2/policy/templates/dds/permissions.xsl

-<xsl:param name="not_valid_before" select="'2020-05-01T00:00:00'"/>
-<xsl:param name="not_valid_after" select="'2030-05-01T00:00:00'"/>
+<xsl:param name="not_valid_before" select="'2020-05-01T00:00:00+00:00'"/>
+<xsl:param name="not_valid_after" select="'2030-05-01T00:00:00+00:00'"/>


Do we want to use +00:00 or Z here?

From the OP ticket:

The time zone may be specified as Z (UTC) or (+|-)hh:mm.

Use current time as default start validity time for certs and permissions #211 (comment)

Not sure if this (formatting choice) was the original point of issue here.

Yeah I initially tried this 2754240 but one of the issues is that Python has pretty limited support for it.

The ability to parse a string in that format appeared in Python 3.11.
And even in Python 3.12, the printing in isoformat is not configurable and outputs the +HH:MM.

I guess we can revisit if the testing of this with connext show the issue still exists with version 6.0.1.

ruffsl · 2024-05-09T01:57:46Z

sros2/sros2/keystore/_permission.py

    cert_content = _utilities.load_cert(cert_path)
-    kwargs['not_valid_before'] = etree.XSLT.strparam(cert_content.not_valid_before.isoformat())
-    kwargs['not_valid_after'] = etree.XSLT.strparam(cert_content.not_valid_after.isoformat())
+    # TODO replace "not_valid_before"/"not_valid_after" functions by


Sad to read about that window holding back and blocking the python REP updates, given the Qt release changes. I hope the latest minor release of cryptography is still secure and up-to-date for all platforms. :<

ruffsl · 2024-05-09T02:00:47Z

sros2/sros2/_utilities.py

-            # 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)
+            utcnow


I'm kind of the opinion if it still broken for a DDS vendor by now, then that's more concerning. I've no longer have an active licence, but I could go ask for a renewal to verify this if you'd like.

yeah so its a bit tricky,

the version shipped with the Jazzy binaries fails to run the launcher:

/opt/rti.com/rti_connext_dds-6.0.1/bin/rtilauncher Gtk-Message: 09:06:35.729: Failed to load module "canberra-gtk-module" No RTI workspace was created. Please contact RTI Support (https://support.rti.com)

The only one I saw available on their website is 7.3.0.

And 7.3.0 is not API compatible with 6.0.1 so I cannot launch nodes

I could try to install the rtipkg from commandline but not sure where to download them if not from the rti website or launcher..

Maybe someone at Open Robotics could give this PR a try ? (as osrf has both license and installer backed up)

@clalancette do you know anyone we could reach out for that could test this ?

So there is a larger issue here, how do people install connext with security plugins for any active ROS 2 distro ?

@ahcorde Hey there 👋

The PR I dont have the ability to test myself we were chatting about offline today

this is the only concern before merge, everything else looks good to me.

sloretz · 2024-05-16T22:29:08Z

@marcoag mind giving this a try with connect? 🧇

fujitatomoya

lgtm with green CI

fujitatomoya · 2025-05-07T16:41:08Z

sros2/sros2/_utilities.py

-            # 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)
+            utcnow


this is the only concern before merge, everything else looks good to me.

mikaelarguedas · 2025-05-13T06:53:59Z

@sloretz @marcoag @fujitatomoya I rebased this, anyone has the ability to test it on Connext so we can decide if we merge this or if we need to downscope it ?

Thanks 🙏

fujitatomoya · 2025-05-13T15:51:50Z

@mikaelarguedas how about starting CI? that should include the test with ConnextDDS, right?

fujitatomoya · 2025-05-13T15:52:43Z

Pulls: #300
Gist: https://gist.githubusercontent.com/fujitatomoya/5410ada7801c7d53b9da50e950b6a99a/raw/5c2aeef052ae70656b4f52357d2f2d49421701c0/ros2.repos
BUILD args: --packages-above-and-dependencies sros2
TEST args: --packages-above sros2
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15976

Linux
Linux-aarch64
Linux-rhel
Windows

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

Python 3.8 compatible because windows Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

clalancette · 2025-07-27T15:27:05Z

sros2/sros2/keystore/_permission.py

    cert_content = _utilities.load_cert(cert_path)
-    kwargs['not_valid_before'] = etree.XSLT.strparam(cert_content.not_valid_before.isoformat())
-    kwargs['not_valid_after'] = etree.XSLT.strparam(cert_content.not_valid_after.isoformat())
+    if _utilities.cryptography_version().major >= 42:


I'm not a huge fan of using semver and parsing the version number here. What we've done elsewhere is to be conditional on the API we want existing. In this case, I think we could do something more like:

if hasattr(cert_content, 'not_valid_before_utc'): kwargs = ... else: kwargs = ...

(this also means we don't add another dependency to this package, which I'm always a fan of)

What do you think?

My original take was to get rid of comments stating which version was needed while keeping an easy way to track from which version legacy code could be removed.

I considered parsing the version string but it seemed more hacky than using a maintained package dedicated to that.

I am happy to update the code accordingly if this is the recommended approach. It is explicit in terms of what API is needed.

My only concern is that it hides the version of the module we're actually requiring for this API to be used. Which often leads to confusion or extra work when we ask "from what version can we get rid of this legacy code?" (similar to #347).

In which case I would add back a comment stating version numbers wherever the check are performed.

Would you prefer that to the current approach ? (I'm fine either way 👍)

It would look like this

# TODO use `not_valid_before_utc` unconditionally once cryptography 42 is available on all target platforms if hasattr(cert_content, 'not_valid_before_utc'): cert_not_valid_before_value = cert.not_valid_before_utc else: cert_not_valid_before_value = cert.not_valid_before.replace(tzinfo=datetime.timezone.utc) # TODO use `not_valid_after_utc` unconditionally once cryptography 42 is available on all target platforms if hasattr(cert_content, 'not_valid_after_utc'): cert_not_valid_after_value = cert.not_valid_after_utc else: cert_not_valid_after_value = cert.not_valid_after.replace(tzinfo=datetime.timezone.utc)

v.s. current state:

if _utilities.cryptography_version().major >= 42: cert_not_valid_before_value = cert.not_valid_before_utc cert_not_valid_after_value = cert.not_valid_after_utc else: cert_not_valid_before_value = cert.not_valid_before.replace(tzinfo=datetime.timezone.utc) cert_not_valid_after_value = cert.not_valid_after.replace(tzinfo=datetime.timezone.utc)

In which case I would add back a comment stating version numbers wherever the check are performed.

Would you prefer that to the current approach ? (I'm fine either way 👍)

Yeah, I think that would be better rather than adding the new dependency. Thanks!

Done in df3fdee

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

clalancette

This looks good to me, thanks! I'll run CI on it next.

clalancette · 2025-07-29T13:46:52Z

Pulls: #300
Gist: https://gist.githubusercontent.com/clalancette/e34124a7f2e103824edb0b48ec20078c/raw/01711b92e4e8c5cd5d3935c696cf5f6cc2ca6d8b/ros2.repos
BUILD args: --packages-above-and-dependencies sros2
TEST args: --packages-above sros2
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16645

Linux
Linux-aarch64
Linux-rhel
Windows

mikaelarguedas marked this pull request as ready for review May 7, 2024 09:48

marcoag requested changes May 7, 2024

View reviewed changes

sros2/sros2/_utilities.py Outdated Show resolved Hide resolved

sros2/test/sros2/commands/security/verbs/test_create_enclave.py Outdated Show resolved Hide resolved

mikaelarguedas mentioned this pull request May 7, 2024

Bump Python to 3.12.3 for rolling ros-infrastructure/ros2-cookbooks#69

Closed

mikaelarguedas force-pushed the timezone-aware-datetimes branch from 5006e37 to 1752d32 Compare May 8, 2024 13:23

mikaelarguedas requested a review from marcoag May 8, 2024 14:29

mikaelarguedas mentioned this pull request May 8, 2024

remove deprecated create_key and list_keys verbs #302

Merged

mikaelarguedas force-pushed the timezone-aware-datetimes branch from fefd058 to 03a1e3e Compare May 8, 2024 21:43

ruffsl approved these changes May 9, 2024

View reviewed changes

sloretz assigned marcoag May 16, 2024

mikaelarguedas mentioned this pull request May 6, 2025

Use timezone-aware objects to represent datetimes in UTC. #341

Closed

fujitatomoya approved these changes May 7, 2025

View reviewed changes

mikaelarguedas force-pushed the timezone-aware-datetimes branch from 03a1e3e to f30f050 Compare May 13, 2025 06:51

mikaelarguedas added 3 commits July 26, 2025 12:02

mark timezone explicitly in datetime strings

2d4bd54

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

use timezone aware datetime, remove -1 day hack in validity

5177e73

Python 3.8 compatible because windows Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

missing import

ef808a6

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

mikaelarguedas force-pushed the timezone-aware-datetimes branch from f30f050 to 6890599 Compare July 26, 2025 10:32

cryptography 42 and later compatibility

4fde405

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

mikaelarguedas force-pushed the timezone-aware-datetimes branch from 6890599 to 4fde405 Compare July 26, 2025 10:45

clalancette reviewed Jul 27, 2025

View reviewed changes

get rid of semver and check for api existence instead

df3fdee

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

clalancette approved these changes Jul 29, 2025

View reviewed changes

clalancette merged commit 46e031c into rolling Jul 29, 2025
6 of 7 checks passed

clalancette deleted the timezone-aware-datetimes branch July 29, 2025 17:16

mikaelarguedas mentioned this pull request Jul 29, 2025

CryptographyDeprecationWarning for _permission (not_valid_before and not_valid_after) #347

Open

Conversation

mikaelarguedas commented May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikaelarguedas commented May 7, 2024

Uh oh!

ahcorde commented May 7, 2024

Uh oh!

Uh oh!

Uh oh!

mikaelarguedas commented May 7, 2024

Uh oh!

mikaelarguedas commented May 7, 2024

Uh oh!

clalancette commented May 7, 2024

Uh oh!

codecov bot commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mikaelarguedas commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikaelarguedas commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloretz commented May 16, 2024

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikaelarguedas commented May 13, 2025

Uh oh!

fujitatomoya commented May 13, 2025

Uh oh!

fujitatomoya commented May 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

clalancette commented Jul 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mikaelarguedas commented May 7, 2024 •

edited

Loading

codecov bot commented May 8, 2024 •

edited

Loading

mikaelarguedas commented May 8, 2024 •

edited

Loading

mikaelarguedas commented May 8, 2024 •

edited

Loading