Skip to content
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

Timezone aware datetimes + remove hack from #209 #300

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from
Open

Timezone aware datetimes + remove hack from #209 #300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
087edef
mark timezone explicitly in datetime strings
mikaelarguedas May 8, 2024
6c5f800
use timezone aware datetime, remove -1 day hack in validity
mikaelarguedas May 8, 2024
03a1e3e
missing import
mikaelarguedas May 8, 2024
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
7 changes: 2 additions & 5 deletions sros2/sros2/_utilities.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,17 +80,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
Copy link
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.

Copy link
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 ?

Copy link
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

).not_valid_after(
# TODO: This should not be hard-coded
utcnow + datetime.timedelta(days=3650)
Expand Down
12 changes: 10 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,15 @@ 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())
# TODO replace "not_valid_before"/"not_valid_after" functions by
Copy link
Member

Choose a reason for hiding this comment

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

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. :<

# "not_valid_before_utc"/"not_valid_after_utc"
# once cryptography 42 is supported on all target platforms
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'"/>
Copy link
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.

Copy link
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
20 changes: 13 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 @@ -123,13 +123,19 @@ 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)

# TODO replace "not_valid_before"/"not_valid_after" functions by
# "not_valid_before_utc"/"not_valid_after_utc"
# once cryptography 42 is supported on all target platforms
assert _datetimes_are_close(
cert.not_valid_before.replace(tzinfo=datetime.timezone.utc),
utcnow
)
assert _datetimes_are_close(
cert.not_valid_after.replace(tzinfo=datetime.timezone.utc),
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
Loading