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

Jump to bottom

Use keystore root as security root directory, and not contexts folder #410

Merged
merged 4 commits into from
Apr 9, 2020
Merged

Use keystore root as security root directory, and not contexts folder #410

merged 4 commits into from
Apr 9, 2020

Conversation

ivanpauno
Copy link
Member

See ros2/rcl#607.
Both should be merged together.

@ivanpauno ivanpauno added the in review Waiting for review (Kanban column) label Apr 7, 2020
@ivanpauno ivanpauno self-assigned this Apr 7, 2020
mikaelarguedas
mikaelarguedas approved these changes Apr 7, 2020
View reviewed changes
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

hidmic
hidmic approved these changes Apr 7, 2020
View reviewed changes
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.

LGTM

ruffsl
ruffsl approved these changes Apr 7, 2020
View reviewed changes
Copy link
Member

@ruffsl ruffsl left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@ruffsl ruffsl mentioned this pull request Apr 7, 2020
Closed
@ivanpauno
Copy link
Member Author

I cannot reproduce the problem in my Windows VM, I will try to figure it out ...

@ivanpauno
Copy link
Member Author

I cannot reproduce the problem in my Windows VM, I will try to figure it out ...

I forgot -DSECURITY=ON 🤦‍♂️ .

@ivanpauno
Fix Windows
ea42d08 
Signed-off-by: Ivan S Paunovic <[email protected]>
@ivanpauno ivanpauno force-pushed the ivanpauno/correct-security-root-directory branch from 1aa8353 to ea42d08 Compare April 9, 2020 13:04
@ivanpauno
Copy link
Member Author

CI: ros2/rcl#607 (comment) and ros2/rcl#607 (comment).

mikaelarguedas
mikaelarguedas previously approved these changes Apr 9, 2020
View reviewed changes
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.

hooray! pretty confused as of why the path worked before in this package but this particular 'contexts' addition needed special handling 🤔

I forgot -DSECURITY=ON man_facepalming .

Yeah this should probably be removed from the test_security package completely... This is a Fast-RTPS flag and requiring to pass it to test security on any rmw implementation is not really warranted.

@mikaelarguedas
Copy link
Member

mikaelarguedas commented Apr 9, 2020 •
edited
Loading

Were the linux and MacOS tests ran with that change too ? (the path modification seem to happen unconditionally / on all platforms)

@mikaelarguedas mikaelarguedas dismissed their stale review April 9, 2020 14:43

question about platforms tested

@ivanpauno
Conditionally set native path
20cf525 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Copy link
Member Author

Where the linux and MacOS tests ran with that change too ? (the path modification seem to happen unconditionally / on all platforms)

Thank you!
New CI jobs here ros2/rcl#607 (comment).

@ivanpauno
nit
0ec293e 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
mikaelarguedas
mikaelarguedas approved these changes Apr 9, 2020
View reviewed changes
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, one non-blocking nit

Do you want to merge this and we'll do a follow-up for the enclave renaming ?

test_security/CMakeLists.txt
@@ -331,13 +331,17 @@ if(BUILD_TESTING)
ament_find_gtest()

set(KEYSTORE_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/test/test_security_files")
set(VALID_ROS_SECURITY_ROOT_DIRECTORY "${KEYSTORE_DIRECTORY}/contexts")
if(WIN32)
string(REPLACE "/" "\\\\" KEYSTORE_DIRECTORY_NATIVE_PATH "${KEYSTORE_DIRECTORY}")
Copy link
Member

Choose a reason for hiding this comment

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

Nit^10: this could be done in place removing the need for a new variable and an else cause 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.

I'm not sure if this

system_tests/test_security/CMakeLists.txt

Line 355 in 0ec293e

file(REMOVE "${KEYSTORE_DIRECTORY}/contexts/publisher_missing_key/key.pem")

will continue working well, as it's a mixture of a cmake and a "native" path.

I'm going to left this as-is, as I don't want to come back to Windows to test.

@ivanpauno
Copy link
Member Author

Do you want to merge this and we'll do a follow-up for the enclave renaming ?

Yeah, the renaming should be addressed in a separate PR.

@ivanpauno ivanpauno merged commit ce0a94d into master Apr 9, 2020
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/correct-security-root-directory branch April 9, 2020 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikaelarguedas mikaelarguedas mikaelarguedas approved these changes

@hidmic hidmic Awaiting requested review from hidmic

@ruffsl ruffsl Awaiting requested review from ruffsl

Assignees

@ivanpauno ivanpauno

Labels
in review Waiting for review (Kanban column)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ivanpauno @mikaelarguedas @ruffsl @hidmic