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.

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

Use Contexts #177

Merged
merged 4 commits into from
Apr 3, 2020
Merged

Use Contexts #177

merged 4 commits into from
Apr 3, 2020

Conversation

ruffsl
Copy link
Member

@ruffsl ruffsl commented Mar 15, 2020

Related: ros2/design#274 (comment)

TODO:

  • Update policy schema with contexts
  • Update policy transforms for DDS
  • Update keystore layout with pubic, private, contexts sub folders
  • Update existing tests and downstream packages

Bonus items

  • Update auto generation
    • would require context aware telemetry from rosgraph API
    • disable otherwise if rosgraph API doesn't expose context info
  • Update documentation
  • Support for metadata and <profiles> type attribute
    • e.g. associating profiles to domains, validity times, singing CA
    • may need more thought, could look back at keymint for config patterns

@ruffsl ruffsl force-pushed the contexts branch 2 times, most recently from 63b6fcb to 0b873ad Compare March 16, 2020 03:56
@ruffsl
Copy link
Member Author

ruffsl commented Mar 16, 2020

Ok, I think the basics are working at least:

$ ros2 security generate_artifacts -k keystore -p ~/ws/ros2/src/ros2/sros2/sros2/test/policies/talker_listener.policy.xml 
keystore is not a valid keystore, creating new keystore
creating keystore: keystore
creating new CA key/cert pair
creating governance file: keystore/contexts/governance.xml
creating signed governance file: keystore/contexts/governance.p7s
all done! enjoy your keystore in keystore
cheers!
creating key for identity: '/talker_listener/talker'
creating cert and key
creating permission file for identity: '/talker_listener/talker'
creating key for identity: '/talker_listener/listener'
creating cert and key
creating permission file for identity: '/talker_listener/listener'

$ tree keystore/
keystore/
├── contexts
│   ├── governance.p7s
│   ├── governance.xml
│   └── talker_listener
│       ├── listener
│       │   ├── cert.pem
│       │   ├── governance.p7s -> ../../governance.p7s
│       │   ├── identity_ca.cert.pem -> ../../../public/identity_ca.cert.pem
│       │   ├── key.pem
│       │   ├── permissions.p7s
│       │   ├── permissions.xml
│       │   └── permissions_ca.cert.pem -> ../../../public/permissions_ca.cert.pem
│       └── talker
│           ├── cert.pem
│           ├── governance.p7s -> ../../governance.p7s
│           ├── identity_ca.cert.pem -> ../../../public/identity_ca.cert.pem
│           ├── key.pem
│           ├── permissions.p7s
│           ├── permissions.xml
│           └── permissions_ca.cert.pem -> ../../../public/permissions_ca.cert.pem
├── private
│   ├── ca.key.pem
│   ├── identity_ca.key.pem -> ca.key.pem
│   └── permissions_ca.key.pem -> ca.key.pem
└── public
    ├── ca.cert.pem
    ├── identity_ca.cert.pem -> ca.cert.pem
    └── permissions_ca.cert.pem -> ca.cert.pem

6 directories, 22 files

ping @mikaelarguedas @kyrofa @ivanpauno

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

I've left some minimal comments and questions.
I will give this a try and comment again.

Thanks @ruffsl for working on this!

sros2/sros2/api/__init__.py Show resolved Hide resolved
sros2/sros2/api/__init__.py Outdated Show resolved Hide resolved
sros2/sros2/api/__init__.py Outdated Show resolved Hide resolved
sros2/sros2/api/__init__.py Outdated Show resolved Hide resolved
sros2/sros2/policy/defaults/policy.xml Outdated Show resolved Hide resolved
sros2/test/policies/add_two_ints.policy.xml Show resolved Hide resolved
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

Some of the verbs arguments or arguments' help should be updated.
e.g.:

'-n', '--node-names', nargs='*', default=[],

or
parser.add_argument('NAME', help='key name, aka ROS node name')

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

Great, thanks @ruffsl !

not a full review, didnt test the code just some fly-by comments

sros2/sros2/api/__init__.py Show resolved Hide resolved
sros2/sros2/api/__init__.py Outdated Show resolved Hide resolved
sros2/sros2/api/__init__.py Show resolved Hide resolved
sros2/sros2/api/__init__.py Outdated Show resolved Hide resolved
sros2/sros2/policy/defaults/policy.xml Outdated Show resolved Hide resolved
sros2/sros2/policy/schemas/policy.xsd Outdated Show resolved Hide resolved
@ivanpauno
Copy link
Member

@ruffsl To allow publishing/subscribing to ros_discovery_info topic, I pushed a branch with two commits:

  • The first commit adds a global parameter in the permissions dds template, to add or not that ALLOW rule.
  • The second commit, sets the parameter to true if the detected rmw implementation is fastrtps based. It uses rclpy get_rmw_implementation_identifier function if available, if not checks the RMW_IMPLEMENTATION environment variable (which can be set in environments without ROS2 installed).

I didn't open a PR to avoid adding extra noise and just in case you want to implement this differently. The first commit can probably be taken as-is. The second one, you maybe want to add something more explicit to the verbs arguments.

Update existing tests and downstream packages

I have already updated system_tests/test_security based on changes here and in rcl.

would require context aware telemetry from rosgraph API

I have already added this to fastrtps, and connext rmw implementations. I have to add something similar in cyclonedds (though they don't support security yet, just for completion), and implement the function in rcl, rclpy. It would be great to complete this in a follow-up though, to limit a bit the size of the changes.

@ivanpauno
Copy link
Member

ivanpauno commented Mar 18, 2020

@ruffsl Friendly ping. It would be great to wrap up this for the beginning of next week (e.g. Wednesday), considering that Foxy deadline is close.

I can help with some of the remaining tasks if you don't have time to handle them (e.g.: updating the tests). Let me know if I can help with anything.

@ruffsl ruffsl force-pushed the contexts branch 2 times, most recently from 6720468 to a2397e6 Compare March 18, 2020 17:51
</topics>
</subscribe>
</allow_rule>
</xsl:if>
Copy link
Member Author

Choose a reason for hiding this comment

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

Lets hope we don't have to keep this fully connected data flow rule forever.

Copy link
Member

@ivanpauno ivanpauno Mar 18, 2020

Choose a reason for hiding this comment

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

Yes. I think that in a near future (two/three months), all the rmw implementation will be migrated.

Copy link
Member

Choose a reason for hiding this comment

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

I believe what @ruffsl mean is that hopefully this will be done out of band and not require a fully connected rule on this topic.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. I think it's definitely possible.
It would be great to finish discussing this alternative in the design document, before the other migrations are done.
Of course, we can first migrate all to use the ros_discovery_info topic, and then change them again to use Participant userData.

@ruffsl
Copy link
Member Author

ruffsl commented Mar 18, 2020

Hey @ivanpauno @mikaelarguedas , I had a moment to push some commits to address things.

The first commit can probably be taken as-is. The second one, you maybe want to add something more explicit to the verbs arguments.

I've cherry picked them into the PR. You're suggesting adding a verb to control this? If its already reading from the env, we may not need to clutter the CLI with a subsumption layer of settings.

It would be great to complete this in a follow-up though, to limit a bit the size of the changes.

Sounds fine.

I can help with some of the remaining tasks if you don't have time to handle them (e.g.: updating the tests). Let me know if I can help with anything.

Thanks! Could you wrap up this PR with the tests and any hanging review comments. I can't immediately think of a pythonic pattern to do the file/dir/symlink checks for the keystore, and I've got another deadline for my lab this week.

@@ -26,10 +26,15 @@ class CreateKeyVerb(VerbExtension):
"""Create key."""

def add_arguments(self, parser, cli_name):
arg = parser.add_argument('ROOT', help='root path of keystore')
arg = parser.add_argument(
'-k', '--keystore-root-path',
Copy link
Member

Choose a reason for hiding this comment

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

To avoid a hard API break, 'ROOT' can be maintained too.
It could be also detected if it was used and print a deprecation warning if later is going to be removed.

P.S.: People will have to read about changes in sros2 for Foxy, so maybe it's not too bad to break this too.

Copy link
Member Author

Choose a reason for hiding this comment

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

People will have to read about changes in sros2 for Foxy, so maybe it's not too bad to break this too.

Yeah, I wanted to use this as an opportunity to make the CLI more consistent. The mix use of positional and keyed command arguments was really annoying in terms if UI.

Copy link
Member

Choose a reason for hiding this comment

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

👍 I was considering doing this before Foxy as well, currently only newly introduced verbs like generate_artifacts have keyed arguments. For clarity it would be better to have them in a separate PR. If we keep it in this one, can we make this in an independent commit (similar to file renaming commits) ?

Copy link
Member

Choose a reason for hiding this comment

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

I will consolidate things in different commits before merging.

sros2/sros2/verb/create_key.py Outdated Show resolved Hide resolved
sros2/sros2/verb/generate_artifacts.py Outdated Show resolved Hide resolved
sros2/sros2/verb/generate_artifacts.py Outdated Show resolved Hide resolved
@ivanpauno
Copy link
Member

Thanks! Could you wrap up this PR with the tests and any hanging review comments. I can't immediately think of a pythonic pattern to do the file/dir/symlink checks for the keystore, and I've got another deadline for my lab this week.

Yes, I will follow up with this tomorrow. Thanks for your prompt answer!!

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

thanks @ruffsl for the iteration!
added a couple inline questions to be able to review the following developments

sros2/sros2/policy/schemas/policy.xsd Outdated Show resolved Hide resolved
@@ -198,17 +242,21 @@ def is_key_name_valid(name):
try:
return (validate_namespace(node_ns) and validate_node_name(node_name))
Copy link
Member

Choose a reason for hiding this comment

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

is this still relevant ?
Seems like key names will now be de correlated from the concept of node name or node namespace

Copy link
Member

@ivanpauno ivanpauno Mar 19, 2020

Choose a reason for hiding this comment

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

I've created a function in rcl: rcl_validate_security_context_name.
It does pretty much the same that rmw_validate_namespace.

I will use the later, until I propagate the rcl function to rclpy.

dest_governance_path = os.path.join(key_dir, 'governance.p7s')
shutil.copyfile(keystore_governance_path, dest_governance_path)
relativepath = os.path.relpath(keystore_governance_path, key_dir)
os.symlink(src=relativepath, dst=dest_governance_path)
Copy link
Member

Choose a reason for hiding this comment

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

What happens in the destination file already exists?

Copy link
Member

Choose a reason for hiding this comment

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

If the file already exists, it will raise a FileExistsError.
Should the old file be deleted and overridden?

Copy link
Member

Choose a reason for hiding this comment

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

We could use a logic similar to the above one above:

  • if it exists and is the same, do nothing
  • if it exists and is different -> delete + override it (this part is not above but I believe would be nicer behavior)

Copy link
Member

@ivanpauno ivanpauno Mar 20, 2020

Choose a reason for hiding this comment

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

Addressed in f5080e8.

Instead of delete + override, I'm raising an error in that last case.
I'm afraid to delete something a distracted user didn't want.

They can cleanup manually and rerun if they really wanted to delete the file.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's my workflow but I find it convenient to change a couple rules in a policy file and just rerun generate_artifacts on my entire policy file and trust the tool to override the changed permission files accordingly. It looks like with this more conservative approach I would need to go prune by hand all the directories that may have a permission change.

I'd prefer not changing the previous behavior, but either way it would be good to provide an option to opt-in (or out) of overwriting existing files

Copy link
Member Author

Choose a reason for hiding this comment

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

In my prior work with keymint, I treated auto generated security files as build artifacts, and thus subsequent build file could be modified at will by the build tool, where the user would recognize the temporal nature of these files if they instructed the build tool to rebuild changed source files. So I'd say let the tool overwrite it the generated files by default.

In keymint, I also separated the pipeline into a src. build, and install folder structure, so we may want to explore using similar workflows that could separate the policy source files, from the generated intermediate representations built, to the final signed security artifacts installed. Keymint had a --dry-run flag that the CLI could check to see if it should warn about detected file diffs and not write to disk.

Copy link
Member

Choose a reason for hiding this comment

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

Done in d523332.

@ivanpauno
Copy link
Member

I've addressed comments and made tests pass.
Though I will do some more updates tomorrow, this is ready to be reviewed again.

raise FileNotFoundError(errno.ENOENT, os.strerror(errno.ENOENT), keystore_path)
if not os.path.exists(contexts_path):
return True
for name in os.listdir(contexts_path):
Copy link
Member Author

@ruffsl ruffsl Mar 19, 2020

Choose a reason for hiding this comment

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

Context paths can be nested. Is listdir recursive?
I.e. will it walk over contexts that aren't just flat in the root of the context sub folder?

Copy link
Member

Choose a reason for hiding this comment

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

👍 to that.

To be fair it was already the case before this change so doesn't have to be addressed here. Should be ticketed though if not addressed

Copy link
Member

Choose a reason for hiding this comment

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

To be fair it was already the case before this change so doesn't have to be addressed here. Should be ticketed though if not addressed

Agreed.
What should be the output of:

-> contexts
  -> context1
    -> security artifacts ...
    -> nested1
      -> security artifacts ...
  ->context2
    ->nested2
      ->security artifacts...

?

IMO, it should be:

/context1
/context1/nested1
/context2/nested2

If the contexts folder have valid security artifacts, / should be showed too.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

thanks @ivanpauno for the follow-up. I did another pass below

dest_governance_path = os.path.join(key_dir, 'governance.p7s')
shutil.copyfile(keystore_governance_path, dest_governance_path)
relativepath = os.path.relpath(keystore_governance_path, key_dir)
os.symlink(src=relativepath, dst=dest_governance_path)
Copy link
Member

Choose a reason for hiding this comment

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

We could use a logic similar to the above one above:

  • if it exists and is the same, do nothing
  • if it exists and is different -> delete + override it (this part is not above but I believe would be nicer behavior)

sros2/sros2/api/__init__.py Outdated Show resolved Hide resolved
sros2/sros2/api/__init__.py Outdated Show resolved Hide resolved
sros2/sros2/api/__init__.py Outdated Show resolved Hide resolved
sros2/sros2/api/__init__.py Outdated Show resolved Hide resolved
sros2/sros2/verb/create_key.py Outdated Show resolved Hide resolved
sros2/sros2/verb/create_permission.py Outdated Show resolved Hide resolved
sros2/sros2/verb/create_permission.py Outdated Show resolved Hide resolved
sros2/sros2/verb/generate_artifacts.py Outdated Show resolved Hide resolved
@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-03-18/13313/1

@ivanpauno ivanpauno marked this pull request as ready for review March 23, 2020 13:34
@ivanpauno
Copy link
Member

@ruffsl @mikaelarguedas
I'm planning to do the following items in a follow-up:

Something else missing to get this approved?

@ruffsl
Copy link
Member Author

ruffsl commented Mar 24, 2020

I was only wondering about that last unresolved comment you linked. If you'd like to follow up that with a septate PR, that's fine. This repo is set so authors of PR can't approve the same PR, but this all LGTM!

Copy link
Member

@kyrofa kyrofa left a comment

Choose a reason for hiding this comment

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

Couple high-level comments from me. This mostly looks good, although @mikaelarguedas is clearly more up-to-date on the design than I am. In the future, I'd prefer to see unrelated changes like the CLI bits in another PR. This one was big.

sros2/setup.py Outdated
@@ -66,7 +66,9 @@ def package_files(directory):
':CreatePermissionVerb',
'distribute_key = sros2.verb.distribute_key:DistributeKeyVerb',
'generate_artifacts = sros2.verb.generate_artifacts:GenerateArtifactsVerb',
'generate_policy = sros2.verb.generate_policy:GeneratePolicyVerb',
# TODO(ivanpauno): Reactivate this after having a way to instronspect
# security context names in rclpy
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 have an issue in rclpy that we can use for tracking this?

Copy link
Member

Choose a reason for hiding this comment

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

+1, TODOs in code usually don't get revisited if there isn't an associated ticket (same for other TODOs below)

Spelling: instronspect -> introspect

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Can we add a reference to ros2/rclpy#529 in this comment, please?

sros2/sros2/api/__init__.py Outdated Show resolved Hide resolved
Comment on lines +37 to +38
def security_context_keys_dir(tmpdir_factory):
keystore_dir = Path(str(tmpdir_factory.mktemp('keystore')))
Copy link
Member

@kyrofa kyrofa Mar 31, 2020

Choose a reason for hiding this comment

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

Has pytest been updated enough we can use tmp_path_factory here now? Would save you a little work. See #145 for reference.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but I think we can left that for a follow-up, because I'm not sure which pytest version we're using in the different platforms.

sros2_cmake/package.xml Outdated Show resolved Hide resolved
@kyrofa
Copy link
Member

kyrofa commented Mar 31, 2020

Alright this looks good to me now. What are we going to do about the failing tests, though? Can we ignore Focal for now in order to get this in before the freeze? @ivanpauno let us know what you learn about Windows.

@ivanpauno
Copy link
Member

Alright this looks good to me now. What are we going to do about the failing tests, though? Can we ignore Focal for now in order to get this in before the freeze? @ivanpauno let us know what you learn about Windows.

Yes, I think it's safe to ignore the tests that are failing in the nightlies.
Of course, there's a chance that I'm introducing a regression without realizing, but the chances are small if it's working ok in the other platforms.

@ruffsl
Copy link
Member Author

ruffsl commented Mar 31, 2020

#177 (comment)

@ivanpauno if you revert/re-sign my commits here, could you move those reverted CLI commits to a new PR? I'd really prefer to remove any positional arguments in SROS from foxy before the API freeze. I think the PR only looks big because of the static xml files generated to test/track the xsd templates.

@mikaelarguedas
Copy link
Member

I think the PR only looks big because of the static xml files generated to test/track the xsd templates.

The motivation for reverting it was not because of the size but the change but their nature. There were doubts on the relevance of making all these non-positional (discussion on matrix). Especially as as-is they were made optional. Currently generate_artifacts is using keyed optional args because they truly are all optional. While the ones like create_keystore do need mandatory arguments and making them keyed doesn't serve much purpose while lengthening user invocation.

So in the end a more complete discussion need to be had and evaluate what UX we are trying to accomplish here. Discussion can happen on such PR though.

@mikaelarguedas
Copy link
Member

Most of them are happening in master too, to be clearer:

Thanks @ivanpauno for the summary.
Figuring out the windows ones would be good indeed: there seem to be some fastrtps failures as well https://ci.ros2.org/job/ci_windows/9674/testReport/junit/(root)/projectroot/test_security_nodes_c__rmw_fastrtps_cpp_2/

Is there a way for maintainers here want to be notified for sros2/test_security failing jobs? I see some of those have been failing for a long time but we weren't aware of it.

@ivanpauno
Copy link
Member

Alright this looks good to me now. What are we going to do about the failing tests, though? Can we ignore Focal for now in order to get this in before the freeze?

I will ignore all existing failures on nightlies. Unfortunately, there's a minimal chance I introduce a regression by doing this.

@ivanpauno if you revert/re-sign my commits here, could you move those reverted CLI commits to a new PR? I'd really prefer to remove any positional arguments in SROS from foxy before the API freeze. I think the PR only looks big because of the static xml files generated to test/track the xsd templates.

https://github.com/ros2/sros2/tree/contexts-backup has the original commits of the PR.

@ivanpauno let us know what you learn about Windows.

That's the last failure I see in my jobs I that's not on the nightlies.
If I'm lucky, I will find the reason today.

Is there a way for maintainers here want to be notified for sros2/test_security failing jobs? I see some of those have been failing for a long time but we weren't aware of it.

IDK. @ros2/team for feedback.

@jacobperron
Copy link
Member

Is there a way for maintainers here want to be notified for sros2/test_security failing jobs? I see some of those have been failing for a long time but we weren't aware of it.

I think at least the ROS 2 buildfarmer should open issues for failures (e.g. on sros2) and consider tagging @ros-security-wg if the issue is in a different repository.

@ivanpauno
Copy link
Member

Figuring out the windows ones would be good indeed: there seem to be some fastrtps failures as well https://ci.ros2.org/job/ci_windows/9674/testReport/junit/(root)/projectroot/test_security_nodes_c__rmw_fastrtps_cpp_2/

The windows failures of security tests were those, and the same test was failing with connext. Fixed them in ros2/system_tests@1d93d97.

I will re-run CI today by end of day. If everything looks good tomorrow, I will merge.

mikaelarguedas
mikaelarguedas previously approved these changes Apr 2, 2020
Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

lgtm, a bionic CI on aarch64 would be good to confirm this doesnt break anything on that platform

@ivanpauno
Copy link
Member

lgtm, a bionic CI on aarch64 would be good to confirm this doesn't break anything on that platform

Done. We're not supporting bionic for Foxy, but running it will add extra confidence in that sros2 wasn't broken.

These are the last jobs I ran ros2/rmw_fastrtps#312 (comment) (only with unrelated failures), and I have to wait to these two before merging ros2/rmw_fastrtps#312 (comment).

Hopefully, I will be merging today.

hidmic
hidmic previously approved these changes Apr 2, 2020
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

Not much to add, LGTM too.

sros2/sros2/api/__init__.py Outdated Show resolved Hide resolved
<subject_name>CN=/add_two_ints/add_two_ints_server</subject_name>
<validity>
<not_before>2013-10-26T00:00:00</not_before>
<not_after>2023-10-26T22:45:30</not_after>
Copy link
Contributor

Choose a reason for hiding this comment

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

@ruffsl @ivanpauno meta: is this supposed to span the foreseeable lifetime of Foxy? Won't this bite us in 3 years time? Same elsewhere.

Copy link
Member

@mikaelarguedas mikaelarguedas Apr 2, 2020

Choose a reason for hiding this comment

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

this is something I'm planning to address separately as unrelated to this PR.

This should be using the time of generation as the start date similarly to how it's done for the certificates. I'll open an issue to make sure it doesnt slip off.
Hackier way to address this is to just change the hardcoded value to start in 2020 which will give us a lot of time to implement it more properly ^^

Edit: opened ticket at #186

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather address #186 instead of pushing the expiration date (and then forgetting about it). Thanks for the explanation @mikaelarguedas!

Copy link
Member

Choose a reason for hiding this comment

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

Me too. Happy to review any PR addressing #186, timing before feature freeze is pretty tight so if it falls on my plate I'll most likely just push the date back

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@547e2c2). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #177   +/-   ##
=========================================
  Coverage          ?   61.57%           
=========================================
  Files             ?       18           
  Lines             ?      838           
  Branches          ?       64           
=========================================
  Hits              ?      516           
  Misses            ?      308           
  Partials          ?       14           
Flag Coverage Δ
#unittests 61.57% <0.00%> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 547e2c2...61fb895. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants