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

Fix list keys verb #219

Merged
merged 4 commits into from
Jun 4, 2020
Merged

Fix list keys verb #219

merged 4 commits into from
Jun 4, 2020

Conversation

mikaelarguedas
Copy link
Member

@mikaelarguedas mikaelarguedas commented May 30, 2020

Fixes #183
setup:
ros2 security generate_artifacts -k keystore -e /foo /foobar/baz /bar /a/b/c/deep

before:

root@5ab52a51093c:/ws# ros2 security list_keys keystore/
foobar
bar
foo
a

after:

root@5ab52a51093c:/ws# ros2 security list_keys keystore/
/a/b/c/deep
/bar
/foo
/foobar/baz

Note: a similar fix could be backported to Eloquent and Dashing

@codecov
Copy link

codecov bot commented May 30, 2020

Codecov Report

Merging #219 into master will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #219      +/-   ##
==========================================
+ Coverage   77.95%   78.02%   +0.07%     
==========================================
  Files          16       16              
  Lines         576      578       +2     
  Branches       52       51       -1     
==========================================
+ Hits          449      451       +2     
  Misses        107      107              
  Partials       20       20              
Flag Coverage Δ
#unittests 78.02% <100.00%> (+0.07%) ⬆️
Impacted Files Coverage Δ
sros2/sros2/api/_key.py 90.41% <100.00%> (+0.27%) ⬆️

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 54e68e8...23e41bf. Read the comment docs.

@mikaelarguedas
Copy link
Member Author

@kyrofa CI on this PR is another occurrence of test_security tests being flaky https://github.com/ros2/sros2/pull/219/checks?check_run_id=722938561

p = pathlib.Path(enclaves_path)
key_file_paths = sorted(p.glob('**/key.pem'))
for key_file_path in key_file_paths:
print('/{}'.format(key_file_path.parent.relative_to(enclaves_path)))
Copy link
Member

Choose a reason for hiding this comment

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

Should this use os.sep?

Copy link
Member

Choose a reason for hiding this comment

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

If it uses the same / used in designating the enclave name path, then the stdout could be piped for more commands involving the enclave name.

Copy link
Member

@kyrofa kyrofa Jun 1, 2020

Choose a reason for hiding this comment

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

That's a good idea, @ruffsl. I actually think my initial comment is wrong; I believe pathlib will use forward slashes regardless of the OS (I don't have a Windows box to confirm-- @Arnatious help), but your suggestion requires a consistency in path handling that I don't think we have right now. We should take a pass on converting things to pathlib and then we can use either slash without issue. That's outside this PR, of course.

Copy link

@Arnatious Arnatious Jun 1, 2020

Choose a reason for hiding this comment

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

Running the code as-is on windows, with a file .\foo\bar\key.pem, gave a printout /foo\bar

Copy link

@Arnatious Arnatious Jun 1, 2020

Choose a reason for hiding this comment

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

Pathlib can read ./foo\\bar just fine and resolve it to the same path

Copy link
Member

@kyrofa kyrofa Jun 1, 2020

Choose a reason for hiding this comment

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

Haha, yeah I'm not sure about that part either, but it's still path-based, regardless of the slashes, so we might as well do the same thing here. Otherwise it could be pretty confusing to print the contents of the files but then have RCL not actually match those up on the backend, you know?

Copy link
Member Author

Choose a reason for hiding this comment

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

fair enough 👍

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 not sure where you're going, but I always saw the list command as a numerating a list of fully qualified enclave names. If the certificates were generated by SROS, then we could also look at the certificate common name, but those aren't guaranteed to match it's location as it can be moved in the key store. But I think we should stick with extrapolating the enclave name from its placement in the key store, as that's how RCL will resolve the enclave name within file system type key stores.

Copy link
Member

Choose a reason for hiding this comment

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

But I think we should stick with extrapolating the enclave name from its placement in the key store, as that's how RCL will resolve the enclave name within file system type key stores.

Sorry if I was unclear, that ^ is what I meant.

Copy link
Member Author

Choose a reason for hiding this comment

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

added as_posix in 23e41bf

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
@mikaelarguedas
Copy link
Member Author

With some non-Linux CI to see the effects:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

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.

Thanks @mikaelarguedas!

@mikaelarguedas mikaelarguedas merged commit 34d0cd6 into master Jun 4, 2020
@mikaelarguedas mikaelarguedas deleted the fix_list_keys branch June 4, 2020 05:53
mikaelarguedas added a commit to mikaelarguedas/sros2 that referenced this pull request Jun 5, 2020
backport and adaptation of ros2#219 to eloquent

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
@mikaelarguedas mikaelarguedas mentioned this pull request Jun 5, 2020
mikaelarguedas added a commit that referenced this pull request Jun 5, 2020
backport and adaptation of #219 to eloquent

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
mikaelarguedas added a commit to mikaelarguedas/sros2 that referenced this pull request Jun 16, 2020
backport and adaptation of ros2#219 to eloquent

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
@mikaelarguedas mikaelarguedas mentioned this pull request Jun 16, 2020
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.

[Dashing/Eloquent] List keys doesn't list nested keys
5 participants