Skip to content

Timezone aware datetimes + remove hack from #209 #300

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
clalancette merged 5 commits into from
Jul 29, 2025
Merged

Timezone aware datetimes + remove hack from #209 #300

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2d4bd54
mark timezone explicitly in datetime strings
mikaelarguedas May 8, 2024
5177e73
use timezone aware datetime, remove -1 day hack in validity
mikaelarguedas May 8, 2024
ef808a6
missing import
mikaelarguedas May 8, 2024
4fde405
cryptography 42 and later compatibility
mikaelarguedas Jul 26, 2025
df3fdee
get rid of semver and check for api existence instead
mikaelarguedas Jul 29, 2025
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
1 change: 1 addition & 0 deletions sros2/package.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
<exec_depend>python3-cryptography</exec_depend>
<exec_depend>python3-importlib-resources</exec_depend>
<exec_depend>python3-lxml</exec_depend>
<exec_depend>python3-semver</exec_depend>
<exec_depend>rclpy</exec_depend>
<exec_depend>ros2cli</exec_depend>

Expand Down
14 changes: 9 additions & 5 deletions sros2/sros2/_utilities.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,15 @@
import os
import pathlib

import cryptography
from cryptography import x509
from cryptography.hazmat.backends import default_backend as cryptography_backend
from cryptography.hazmat.primitives import hashes
from cryptography.hazmat.primitives import serialization
from cryptography.hazmat.primitives.asymmetric import ec

import semver

import sros2.errors

_DOMAIN_ID_ENV = 'ROS_DOMAIN_ID'
Expand All @@ -38,6 +41,10 @@ def create_symlink(*, src: pathlib.Path, dst: pathlib.Path):
os.symlink(src, dst)


def cryptography_version() -> semver.VersionInfo:
return semver.parse_version_info(cryptography.__version__)


def domain_id() -> str:
return os.getenv(_DOMAIN_ID_ENV, '0')

Expand Down Expand Up @@ -80,17 +87,14 @@ def build_key_and_cert(subject_name, *, ca=False, ca_key=None, issuer_name=''):
else:
extension = x509.BasicConstraints(ca=False, path_length=None)

utcnow = datetime.datetime.utcnow()
utcnow = datetime.datetime.now(datetime.timezone.utc)
builder = x509.CertificateBuilder(
).issuer_name(
issuer_name
).serial_number(
x509.random_serial_number()
).not_valid_before(
# 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)
utcnow

@ruffsl ruffsl May 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@mikaelarguedas mikaelarguedas May 9, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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 ?

@mikaelarguedas mikaelarguedas Jun 20, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ahcorde Hey there 👋

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

@fujitatomoya fujitatomoya May 7, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

).not_valid_after(
# TODO: This should not be hard-coded
utcnow + datetime.timedelta(days=3650)
Expand Down
17 changes: 15 additions & 2 deletions sros2/sros2/keystore/_permission.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

import datetime
import os
import pathlib

Expand Down Expand Up @@ -75,8 +76,20 @@ def create_permission_file(path: pathlib.Path, domain_id, policy_element) -> Non

cert_path = path.parent.joinpath('cert.pem')
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:

@clalancette clalancette Jul 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@mikaelarguedas mikaelarguedas Jul 27, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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)

@clalancette clalancette Jul 28, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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!

@mikaelarguedas mikaelarguedas Jul 29, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done in df3fdee

kwargs['not_valid_before'] = etree.XSLT.strparam(
cert_content.not_valid_before_utc
)
kwargs['not_valid_after'] = etree.XSLT.strparam(
cert_content.not_valid_after_utc
)
else:
kwargs['not_valid_before'] = etree.XSLT.strparam(
cert_content.not_valid_before.replace(tzinfo=datetime.timezone.utc).isoformat()
)
kwargs['not_valid_after'] = etree.XSLT.strparam(
cert_content.not_valid_after.replace(tzinfo=datetime.timezone.utc).isoformat()
)

if get_rmw_implementation_identifier() in _RMW_WITH_ROS_GRAPH_INFO_TOPIC:
kwargs['allow_ros_discovery_topic'] = etree.XSLT.strparam('1')
Expand Down
4 changes: 2 additions & 2 deletions sros2/sros2/policy/templates/dds/permissions.xsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
<xsl:output omit-xml-declaration="yes" indent="yes"/>
<xsl:strip-space elements="*"/>

<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'"/>

@ruffsl ruffsl May 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

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

@mikaelarguedas mikaelarguedas May 9, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.


<xsl:variable name="template_validity">
<validity>
Expand Down
8 changes: 4 additions & 4 deletions sros2/test/policies/permissions/add_two_ints/permissions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
<grant name="/add_two_ints/add_two_ints_server">
<subject_name>CN=/add_two_ints/add_two_ints_server</subject_name>
<validity>
<not_before>2020-05-01T00:00:00</not_before>
<not_after>2030-05-01T00:00:00</not_after>
<not_before>2020-05-01T00:00:00+00:00</not_before>
<not_after>2030-05-01T00:00:00+00:00</not_after>
</validity>
<allow_rule>
<domains>
Expand Down Expand Up @@ -56,8 +56,8 @@
<grant name="/add_two_ints/add_two_ints_client">
<subject_name>CN=/add_two_ints/add_two_ints_client</subject_name>
<validity>
<not_before>2020-05-01T00:00:00</not_before>
<not_after>2030-05-01T00:00:00</not_after>
<not_before>2020-05-01T00:00:00+00:00</not_before>
<not_after>2030-05-01T00:00:00+00:00</not_after>
</validity>
<allow_rule>
<domains>
Expand Down
8 changes: 4 additions & 4 deletions sros2/test/policies/permissions/minimal_action/permissions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
<grant name="/minimal_action/minimal_action_server">
<subject_name>CN=/minimal_action/minimal_action_server</subject_name>
<validity>
<not_before>2020-05-01T00:00:00</not_before>
<not_after>2030-05-01T00:00:00</not_after>
<not_before>2020-05-01T00:00:00+00:00</not_before>
<not_after>2030-05-01T00:00:00+00:00</not_after>
</validity>
<allow_rule>
<domains>
Expand Down Expand Up @@ -64,8 +64,8 @@
<grant name="/minimal_action/minimal_action_client">
<subject_name>CN=/minimal_action/minimal_action_client</subject_name>
<validity>
<not_before>2020-05-01T00:00:00</not_before>
<not_after>2030-05-01T00:00:00</not_after>
<not_before>2020-05-01T00:00:00+00:00</not_before>
<not_after>2030-05-01T00:00:00+00:00</not_after>
</validity>
<allow_rule>
<domains>
Expand Down
28 changes: 14 additions & 14 deletions sros2/test/policies/permissions/sample/permissions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
<grant name="/talker_listener/talker">
<subject_name>CN=/talker_listener/talker</subject_name>
<validity>
<not_before>2020-05-01T00:00:00</not_before>
<not_after>2030-05-01T00:00:00</not_after>
<not_before>2020-05-01T00:00:00+00:00</not_before>
<not_after>2030-05-01T00:00:00+00:00</not_after>
</validity>
<allow_rule>
<domains>
Expand Down Expand Up @@ -57,8 +57,8 @@
<grant name="/talker_listener/listener">
<subject_name>CN=/talker_listener/listener</subject_name>
<validity>
<not_before>2020-05-01T00:00:00</not_before>
<not_after>2030-05-01T00:00:00</not_after>
<not_before>2020-05-01T00:00:00+00:00</not_before>
<not_after>2030-05-01T00:00:00+00:00</not_after>
</validity>
<allow_rule>
<domains>
Expand Down Expand Up @@ -111,8 +111,8 @@
<grant name="/add_two_ints/add_two_ints_server">
<subject_name>CN=/add_two_ints/add_two_ints_server</subject_name>
<validity>
<not_before>2020-05-01T00:00:00</not_before>
<not_after>2030-05-01T00:00:00</not_after>
<not_before>2020-05-01T00:00:00+00:00</not_before>
<not_after>2030-05-01T00:00:00+00:00</not_after>
</validity>
<allow_rule>
<domains>
Expand Down Expand Up @@ -166,8 +166,8 @@
<grant name="/add_two_ints/add_two_ints_client">
<subject_name>CN=/add_two_ints/add_two_ints_client</subject_name>
<validity>
<not_before>2020-05-01T00:00:00</not_before>
<not_after>2030-05-01T00:00:00</not_after>
<not_before>2020-05-01T00:00:00+00:00</not_before>
<not_after>2030-05-01T00:00:00+00:00</not_after>
</validity>
<allow_rule>
<domains>
Expand Down Expand Up @@ -221,8 +221,8 @@
<grant name="/minimal_action/minimal_action_server">
<subject_name>CN=/minimal_action/minimal_action_server</subject_name>
<validity>
<not_before>2020-05-01T00:00:00</not_before>
<not_after>2030-05-01T00:00:00</not_after>
<not_before>2020-05-01T00:00:00+00:00</not_before>
<not_after>2030-05-01T00:00:00+00:00</not_after>
</validity>
<allow_rule>
<domains>
Expand Down Expand Up @@ -282,8 +282,8 @@
<grant name="/minimal_action/minimal_action_client">
<subject_name>CN=/minimal_action/minimal_action_client</subject_name>
<validity>
<not_before>2020-05-01T00:00:00</not_before>
<not_after>2030-05-01T00:00:00</not_after>
<not_before>2020-05-01T00:00:00+00:00</not_before>
<not_after>2030-05-01T00:00:00+00:00</not_after>
</validity>
<allow_rule>
<domains>
Expand Down Expand Up @@ -343,8 +343,8 @@
<grant name="/sample_policy/admin">
<subject_name>CN=/sample_policy/admin</subject_name>
<validity>
<not_before>2020-05-01T00:00:00</not_before>
<not_after>2030-05-01T00:00:00</not_after>
<not_before>2020-05-01T00:00:00+00:00</not_before>
<not_after>2030-05-01T00:00:00+00:00</not_after>
</validity>
<allow_rule>
<domains>
Expand Down
4 changes: 2 additions & 2 deletions sros2/test/policies/permissions/single_context/permissions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
<grant name="/single_enclave">
<subject_name>CN=/single_enclave</subject_name>
<validity>
<not_before>2020-05-01T00:00:00</not_before>
<not_after>2030-05-01T00:00:00</not_after>
<not_before>2020-05-01T00:00:00+00:00</not_before>
<not_after>2030-05-01T00:00:00+00:00</not_after>
</validity>
<allow_rule>
<domains>
Expand Down
8 changes: 4 additions & 4 deletions sros2/test/policies/permissions/talker_listener/permissions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
<grant name="/talker_listener/talker">
<subject_name>CN=/talker_listener/talker</subject_name>
<validity>
<not_before>2020-05-01T00:00:00</not_before>
<not_after>2030-05-01T00:00:00</not_after>
<not_before>2020-05-01T00:00:00+00:00</not_before>
<not_after>2030-05-01T00:00:00+00:00</not_after>
</validity>
<allow_rule>
<domains>
Expand Down Expand Up @@ -57,8 +57,8 @@
<grant name="/talker_listener/listener">
<subject_name>CN=/talker_listener/listener</subject_name>
<validity>
<not_before>2020-05-01T00:00:00</not_before>
<not_after>2030-05-01T00:00:00</not_after>
<not_before>2020-05-01T00:00:00+00:00</not_before>
<not_after>2030-05-01T00:00:00+00:00</not_after>
</validity>
<allow_rule>
<domains>
Expand Down
24 changes: 17 additions & 7 deletions sros2/test/sros2/commands/security/verbs/test_create_enclave.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,13 +122,23 @@ def test_cert_pem(enclave_keys_dir):
assert isinstance(cert.signature_hash_algorithm, hashes.SHA256)

# Verify the cert is valid for the expected timespan
utcnow = datetime.datetime.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))
utcnow = datetime.datetime.now(datetime.timezone.utc)

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)

assert _datetimes_are_close(
cert_not_valid_before_value,
utcnow
)
assert _datetimes_are_close(
cert_not_valid_after_value,
utcnow + datetime.timedelta(days=3650)
)

# Verify that the cert ensures this key cannot be used to sign others as a CA
assert len(cert.extensions) == 1
Expand Down