Adding rcutils_to_native_path function

[ruffsl]

Relates to: ros2/sros2#68 (comment)

[mikaelarguedas]

Not sure if we should duplicate this in every function using it.
Is it possible to define those at the top of the file to something like RCUTILS_PATH_DELIMITER and reuse it in all functions using it instead?

[ruffsl]

The top of the header file or the c file?

[mikaelarguedas]

If we don't plan on exporting it to users (which I think we don't have a need for right now) I'd say yeah put it in the c file for now.

[mikaelarguedas]

Sorry maybe that was unclear, I meant creating a macro with #define at the top of the file, not creating actual variables, something like

#ifdef  _WIN32
# define RCUTILS_PATH_DELIMITER "\\"
#else
# define RCUTILS_PATH_DELIMITER "/"
#endif  // _WIN32

[ruffsl]

I meant creating a macro with #define at the top of the file

fixed

[mikaelarguedas]

I'm not sure about the name of the function.
As a user I would expect rcutils_normalize_path to also remove duplicated separators and handle things like ..
Maybe rcutils_to_native_path would be more appropriate? (similar to cmake's TO_NATIVE_PATH, though I think the CMake one handles duplicated separators).

[ruffsl]

also remove duplicated separators

I'd suggest keeping this minimalistic, as having rcutils_normalize_path repair the path could conceal unintended issues where users a not properly coordinating name and namespaces strings down the line.

[mikaelarguedas]

I'd suggest keeping this minimalistic

I agree keeping this patch scoped to only do what we need 👍, my remark was not to increase the featureset on this PR but more about picking the right naming to not confuse users as of what the function is doing.

I think rcutils_to_native_path is good for not setting expectations of modifications made to the path, but just expect changing separators

[mikaelarguedas]

@ruffsl adding tests for this made me wondering. The conversion is currently done only one way / -> \\, would it make sense to also be able to convert a path from \\ separators to / ?

[ruffsl]

would it make sense to also be able to convert a path from \\ separators to / ?

@mikaelarguedas , that sounds something like the reverse of normcase. A possible case could be to infer the node's FQN if given only the node's secure root, but that sounds like a poor idea given in most contexts the FQN is already know, and it would be a hard assumption that node's secure directory is within a secure root structure.

On that last hard assumption note, touching on ros2/sros2#69 (comment) I'd like to eventually add something like ROS_SECURITY_NODE_DIRECTORY to allow the user to escape from the namespace lookup in the ROS_SECURITY_ROOT_DIRECTORY. This would be helpful when you want to pass the node necessary security artifacts, but the namespace or nodename is uniquely set at runtime (i.e. not predetermined) or you want several nodes of different namespaces to share a common set of security artifacts without creating symlinks everyware.

Maybe somewhere else in rcl might have use for something like to_cononcical_path, but I can't think of a good use case currently.

[mikaelarguedas]

that sounds something like the reverse of normcase. A possible case could be to infer the node's FQN if given only the node's secure root, but that sounds like a poor idea given in most contexts the FQN is already know, and it would be a hard assumption that node's secure directory is within a secure root structure.

This is very SROS 2 specific. As this package provides c utilities independent of any ROS concept I was mostly trying to have naming and behavior that is intuitive for users regardless of the project they use this function in.

I think it's fair to say that people will mostly operate on forward slash separated path to start with. It's unclear to me what name is most appropriate so let's leave it as is for now 👍

---

A possible case could be to infer the node's FQN if given only the node's secure root, but that sounds like a poor idea given in most contexts the FQN is already know

I agree that the FQDN should be provided by rcl and not inferred 👍

his would be helpful when you want to pass the node necessary security artifacts, but the namespace is uniquely set at runtime (i.e. not predetermined)

Yeah this would be a good use case to support

you want several nodes of different namespaces to share a common set of security artifacts without creating symlinks everyware.

I'm less sure about this one, While it offers flexibility, I think it leads to a path of favoring relaxed security and cross-talk.

[ruffsl]

I'm less sure about this one, While it offers flexibility, I think it leads to a path of favoring relaxed security and cross-talk.

On the user written permission level, true. But to really address the issue of cross-talk, I think restricting read access to the security artifacts using something like linux security modules to harden the file system or Trusted Execution Environment (TEE). Given the secure root directory can be controlled by shell environment already, I don't see much of a difference in permitting the same shell environment to be explicit in the exact secure node path as well.

Also for cases where only authentication is needed; signing, renewing and revoking a limited number certificates for a whole robot would certainly be more tractable than have to revoke each certificate/private-key for every ROS2 node on a compromised robot.

Users might also want to use sets of permission/certs that align with the system layers (e.g. using a defense-in-depth strategy) rather than aligning to ROS2 namespace concepts, e.g. all ECU related nodes on a single ECU board use the one/only cert allocated to that trusted device.

---

I could start on new PR to add ROS_SECURITY_NODE_DIRECTORY or I could tack it on here scene it deals with the same functions.

[clalancette]

This looks OK to me. I'm going to run CI on this (along with ros2/rcl#300) to see how we fare:

• Linux
• Linux-aarch64
• macOS
• Windows

[clalancette]

Oh, all of the builds are failing, probably because this (and ros2/rcl#300) need a rebase (there have been some internal API changes recently). @ruffsl , mind doing the rebase?

[ruffsl]

@clalancette , I just now rebased both #119 and ros2/rcl#300 , please retry tests.

[clalancette]

Another round of CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[ruffsl]

@clalancette , it looks like in the day or so since last rebase, ros2/rcl#322 was just merged. I've rebased again, so you may want to cancel the rest of the tests and restart the job. If you happen to retrigger the jobs after a day or so, you may want to check if the branches need rebasing just before.

[ruffsl]

@clalancette FYI: ros2/system_tests#308

[clalancette]

@clalancette FYI: ros2/system_tests#308

Thanks. Here is another try:

• Linux
• Linux-aarch64
• macOS
• Windows

[clalancette]

@ruffsl We are getting closer :). It looks like cpplint doesn't like the use of strcpy in ros2/rcl#300, and prefers snprintf. Elsewhere in rcl (like https://github.com/ros2/rcl/blob/master/rcl/src/rcl/rcl.c#L114), we've used memcpy to do string manipulations. I'm not entirely sure what is most appropriate for this situation, but I think I'd prefer to use snprintf if we can. If not, we can fall back to memcpy, since that is what we do elsewhere.

[ruffsl]

Well, I think turing complete snprintf is overkill, as this doesn't need to do any formatting; and the alternative strlcpy doesn't seem to be standard or portable. So I'll just fall back to using memcpy then.

https://www.geeksforgeeks.org/why-strcpy-and-strncpy-are-not-safe-to-use/
https://stackoverflow.com/a/2115072/2577586

Rebased, please trigger again with ready.

[wjwwood]

memcpy doesn't ensure a null terminator, so be careful about that.

There is rcutils_snprintf and rcutils_format_string, which are also available.

[clalancette]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[ruffsl]

Awesome, looks like all tests above are passing! 🎉