-
Notifications
You must be signed in to change notification settings - Fork 45
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
Fix list keys verb #219
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@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 |
sros2/sros2/api/_key.py
Outdated
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))) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough 👍
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
7d352a1
to
23e41bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mikaelarguedas!
backport and adaptation of ros2#219 to eloquent Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
backport and adaptation of #219 to eloquent Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
backport and adaptation of ros2#219 to eloquent Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Fixes #183
setup:
ros2 security generate_artifacts -k keystore -e /foo /foobar/baz /bar /a/b/c/deep
before:
after:
Note: a similar fix could be backported to Eloquent and Dashing