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

Proposal: Security functionalities for LCM #467

Open
mojasp opened this issue Aug 16, 2023 · 3 comments
Open

Proposal: Security functionalities for LCM #467

mojasp opened this issue Aug 16, 2023 · 3 comments

Comments

@mojasp
Copy link

mojasp commented Aug 16, 2023

Motivation

LCM does not provide any security features. Tunneling LCM over a secure protocol (when using the UDP-Multicast provider) is difficult due to the - mostly - multicast-based nature of LCM. Therefore, i expect most of the users of this library to use LCM in some sort of isolated network. This however violates against the zero trust principle: "The entire enterprise private network is not considered an implicit trust zone" [1]

Solution

We have developed and implemented a set of security features of LCM. A paper has been submitted for peer review and is available here. The mentioned security features have been implemented in a fork called LCMsec.

The goals of LCMsec are as follows:

  • Provide Confidentiality, Authenticity and Integrity for LCM messages & channelnames via authenticated encryption
  • Provide an access control mechanism with (multicastgroup, port, channelname) granularity
  • be usable whereever LCM is usable (maintain low latency and high throughput characteristics)
  • maintain decentralized, peer-to-peer nature of LCM: includes decentralized peer discovery & certificate exchange

Non-goals are:

  • Differentiating between read- and write access to a channel

Proposal

Merge LCMsec into the LCM library as an optional, secured version of the protocol.

  • To the users of LCM: Are you interested? Would you use such a thing?
  • If there is indeed some interest: I do not believe that LCMsec is at this stage is ready to merge. To be more useable:
    • Support for languages other than C, C++ and Python should be provided
    • Tooling support should be provided (lcm-spy and lcmlogger)
    • a seemless certificate management solution should be provided

If there is indeed interest, i would be happy to work towards a merge. In that case, would one of the maintainers be willing to coordinate with me and make a plan as to what is / is not required to merge?

[1] https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-207.pdf

@ihilt
Copy link
Contributor

ihilt commented Aug 18, 2023

Thank you for this proposal! This does indeed look rather interesting.

From a maintainer perspective, I have a few thoughts:

  • As you mentioned in the proposal, LCMsec should be an opt-in configuration such that users who aren't interested in the security features aren't forced to do anything different after updating than what they are doing currently, e.g. install more dependencies, add CMake options to disable features, etc.
  • It should be as isolated as possible so updates to LCMsec have minimal impact on the rest of the code base. Obviously this is a judgment call so we could work that out if this moves forward.
  • Similar to bullet 1, the existing method of connecting nodes and the LCM message format isn't modified in the non-secure case so existing users aren't required to update all of their nodes to the LCMsec version.

Regarding your question to users of LCM, you may want to post to the Google group https://groups.google.com/g/lcm-dev or https://groups.google.com/g/lcm-users to sample the broader community's interest.

I think that's it from my point of view. I appreciate your interest in contributing to LCM!

@mojasp
Copy link
Author

mojasp commented Aug 19, 2023

I appreciate your interest :) If you are open to move forward with this, provided your comments are met, i will do some cleanup to reduce intrusion etc.

As you mentioned in the proposal, LCMsec should be an opt-in configuration such that users who aren't interested in the security features aren't forced to do anything different after updating than what they are doing currently, e.g. install more dependencies, add CMake options to disable features, etc.

It is not right now, but it should not be a problem to achieve this.

It should be as isolated as possible so updates to LCMsec have minimal impact on the rest of the code base. Obviously this is a judgment call so we could work that out if this moves forward.

Most of the code is indeed seperate, and in a seperate folder. However, i will give you an overview of places where there is some intrusion:

  • There are changes in lcm.h, and lcm.c, to provide an API for lcmsec. However, the API (and ABI, i think) for the unsecured version does not change.
    • For the C++ interface, The API does not change. I think that the ABI is fine as well, since LCM class methods are thankfully inlined.
  • The create function in _lcm_provider_vtable_tis changed to accommodate the new API. Unfortunately i needed a way to pass the user-provided security parameters to the udpm provider - There are quite a few parameters, in my opinion it would be hacky to use the url for this purpose... This means that the files for all other providers are touched as well (to include the additional parameter).
  • There are additional headers used (long and short) for the secured version (changes to udpm_util.h)
  • changes to lcm_udpm.c, since i "intercept" packages on the socket layer to do encryption and decryption. The secured versions of lcm_udpm_send and _recv_* could be moved to a different file if desired, but a few changes remain.

I hope i did not miss anything, but if there are other changes, i think they are either very small or unnesscary and could be cleaned up.

Similar to bullet 1, the existing method of connecting nodes and the LCM message format isn't modified in the non-secure case so existing users aren't required to update all of their nodes to the LCMsec version.

This is the case - a seperate header and Magic Number is used for the secure case. Since i intercept the udp packets, the message format itself is not touched.

I would be happy to hear your thoughts about this - especially about the current level of intermixing between LCM and LCMsec.

@ihilt
Copy link
Contributor

ihilt commented Aug 22, 2023

I appreciate your interest :) If you are open to move forward with this, provided your comments are met, i will do some cleanup to reduce intrusion etc.
It is not right now, but it should not be a problem to achieve this.

Excellent!

Most of the code is indeed seperate, and in a seperate folder. However, i will give you an overview of places where there is some intrusion:

  • There are changes in lcm.h, and lcm.c, to provide an API for lcmsec. However, the API (and ABI, i think) for the unsecured version does not change.

Yes, taking a look at the changes, this appears to be the case.

  • For the C++ interface, The API does not change. I think that the ABI is fine as well, since LCM class methods are thankfully inlined.

Agreed.

  • The create function in _lcm_provider_vtable_tis changed to accommodate the new API. Unfortunately i needed a way to pass the user-provided security parameters to the udpm provider - There are quite a few parameters, in my opinion it would be hacky to use the url for this purpose... This means that the files for all other providers are touched as well (to include the additional parameter).

This should also work since that struct is accessed as an opaque object from users of the LCM library.

  • There are additional headers used (long and short) for the secured version (changes to udpm_util.h)
    changes to lcm_udpm.c, since i "intercept" packages on the socket layer to do encryption and decryption. The secured versions of lcm_udpm_send and recv* could be moved to a different file if desired, but a few changes remain.

That might be a good idea, though, I might need to see a version with the changes you've mentioned thus far. Overall, this all sounds good.

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

No branches or pull requests

2 participants