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.
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.
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.
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.
Pathlib can read ./foo\\bar just fine and resolve it to the same path
There was a problem hiding this comment.
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.
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.
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.
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
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/deepbefore:
after:
Note: a similar fix could be backported to Eloquent and Dashing