Implement Bag encryption/decryption.#1206
Conversation
|
|
||
| find_package(console_bridge REQUIRED) | ||
| find_package(catkin REQUIRED COMPONENTS cpp_common roscpp_serialization roscpp_traits rostime roslz4) | ||
| find_package(catkin REQUIRED COMPONENTS cpp_common pluginlib roscpp_serialization roscpp_traits rostime roslz4) |
There was a problem hiding this comment.
The dependency on pluginlib needs to be declared in the package manifest in order to be installed on Jenkins (and become a dependency of the Debian package). Therefore CI currently fails.
In general a dependency on pluginlib will be a bit problematic since currently pluginlib is only part of the higher level group "ROS base". When required here it will need to be moved to "ROS core" (see REP 142).
There was a problem hiding this comment.
I'm in favour of that change, as I'd like to use plugins (or at least plugin discovery) in tools like rostopic.
There was a problem hiding this comment.
That would be "challenging" since rostopic is written in Python and pluginlib in C++ 😉
There was a problem hiding this comment.
Yeah, I think I was thinking that the python plugin discovery stuff that rqt uses was part of pluginlib itself, but it's totally not; it's all just right there in the rqt repo. Well never mind that then. :)
In any case, pluginlib is itself a lightweight dependency, isn't it? Is the pocoo stuff it depends on heavy?
There was a problem hiding this comment.
It's not heavy: mostly pluginlib and class_loader.
The "problem" is that pluginlib depends on rosconsole which is in the ros_comm repo.
So if rosbag_storage would depend on pluginlib that would result in a repository-level circular dependency. To avoid that some parts would likely need to be split out into a separate repo. (In general splitting this repo into multiple smaller ones is beneficial - but also a lot of effort...)
There was a problem hiding this comment.
Declared build/run dependency to libgpgme11-dev, libssl-dev, and pluginlib in package.xml.
There was a problem hiding this comment.
The repo-level circular dependency doesn't impact our builds, so we'd be happy to proceed with this path if the proposed change is otherwise acceptable, and plan to assist with splitting out rosconsole in time for the ROS M release of this repository.
Alternatively, I could look at breaking pluginlib's dependency on rosconsole, potentially by just changing its logging over to use the non-ROS console_bridge API instead. Is that acceptable?
If not, we can retain the idea of encryption being a pluggable capability while not actually allowing external encryption modules to be plugged in (eg, setEncryptorPlugin would take a pointer to an instance rather a plugin name and params).
Let us know how you'd like to proceed here.
There was a problem hiding this comment.
if the proposed change is otherwise acceptable
I will add some higher level comment to the PR. But the general feature sounds like a good idea to me.
... splitting out
rosconsolein time for the ROS M release of this repository.
I don't think we want the repository level circular dependency. So splitting out rosconsole before accepting this feature would be a reasonable path forward.
Alternatively, I could look at breaking
pluginlib's dependency on rosconsole, potentially by just changing its logging over to use the non-ROSconsole_bridgeAPI instead. Is that acceptable?
It might be but you might want to check with the pluginlib maintainer on this. There might be problems which I don't foresee atm.
| compression_ = compression; | ||
| } | ||
|
|
||
| void Bag::setEncryptorPlugin(std::string const& plugin_name, std::string const& plugin_param) { |
There was a problem hiding this comment.
How much overlap is there between the encryption functions and the compression functions? My question about this would be whether it would make more sense to have this as a more generic setStoragePlugin, where bzip2 and lzma compression plugins can be loaded similar to the encryption scheme.
Obviously I don't want the scope to get away from us, but it's just a thought.
There was a problem hiding this comment.
Would setStoragePlugin cover (i) encryption, (ii) compression, and (iii) the combination of the two?
There was a problem hiding this comment.
Honestly, I haven't given it that much thought. If there isn't an obvious path on that, disregard— a plugin hook which applies only to encryption is fine with me.
There was a problem hiding this comment.
Thank you for your input @mikepurvis. Nothing is wrong with having a single 'storage' plugin (except that we might end up with mxn plugin classes for m compression methods and n encryptors). Refactoring existing compression stream classes and combining them with encryptors would require lots of efforts, and I would like leave that as a future task.
eafecb1 to
515189f
Compare
|
In general this feature is a great addition. I would have a few comments / questions beside the considerations of the circular dependencies discussed above:
|
|
|
Added a test case, and move implementations to |
|
Build on CI fails due to missing library |
|
|
|
Thank you @gavanderhoorn. |
|
Added command-line tools to en/decrypt bag files. Encrypting a large bag file takes a long time, and a progress meter should be implemented in future. |
932513e to
a2c0545
Compare
|
@dirk-thomas All of your initial comments have been addressed, and the PR is ready for rereivew:
|
dirk-thomas
left a comment
There was a problem hiding this comment.
I added few more comments inline but haven't taken a too close look to the patch yet.
Since it breaks ABI and should therefore only be considered for the next ROS release (Melodic) we can't merge it until we have a melodic-devel branch. Usually we branch as late as possible (maybe around February) to avoid extra porting effort.
It would be great if other could try this feature in the meantime and comment with feedback here.
| ARCHIVE DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION} | ||
| LIBRARY DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION} | ||
| RUNTIME DESTINATION ${CATKIN_PACKAGE_BIN_DESTINATION}) | ||
| endif() |
There was a problem hiding this comment.
Please put the generic part outside of this conditional block and only create a conditional block for encrypt.
tools/rosbag/src/encrypt.cpp
Outdated
| ("help,h", "produce help message") | ||
| ("quiet,q", "suppress console output") | ||
| ("plugin,p", po::value<std::string>()->default_value("rosbag/AesCbcEncryptor"), "encryptor name") | ||
| ("param,r", po::value<std::string>()->default_value("*"), "encryptor parameter") |
There was a problem hiding this comment.
Please avoid such long blocks of spaces for aligning arguments.
Same below.
tools/rosbag/src/encrypt.cpp
Outdated
| }; | ||
|
|
||
| void EncryptorOptions::buildOutbagName() { | ||
| if (!outbag.empty()) { |
There was a problem hiding this comment.
Please use a consistent style for positioning curly braces. In ROS 1 they are generally on a separate line. But most important is consistency.
Across the code.
There was a problem hiding this comment.
I tried to make the style consistent with sibling files, "play.cpp" and "record.cpp", though the style in those two files are problematic. Let me use consistent separate-line braces in encrypt.cpp.
| src/uncompressed_stream.cpp | ||
| ) | ||
| target_link_libraries(rosbag_storage ${catkin_LIBRARIES} ${Boost_LIBRARIES} ${BZIP2_LIBRARIES} ${console_bridge_LIBRARIES} crypto gpgme) | ||
| endif() |
There was a problem hiding this comment.
Please do not duplicate large chunks of code like this. Instead create a variable for the optional files, fill that variable based on the platform, and then use it as one argument to the add_library call.
There was a problem hiding this comment.
Addressed. Thank you.
|
I'm okay with releasing encryption functionality along with Melodic. |
9fccb54 to
dffec15
Compare
|
All review comments and compiler warnings so far have been addressed. |
tools/rosbag_storage/CMakeLists.txt
Outdated
|
|
||
| catkin_add_gtest(test_aes_encryptor test/test_aes_encryptor.cpp | ||
| WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}/test) | ||
| target_link_libraries(test_aes_encryptor rosbag_storage ${catkin_LIBRARIES} ${Boost_LIBRARIES}) |
There was a problem hiding this comment.
This line needs to be wrapped in a conditional: if(TARGET test_aes_encryptor).
There was a problem hiding this comment.
Addressed. Thank you.
| if(NOT WIN32) | ||
| set(AES_ENCRYPT_SOURCE "src/aes_encryptor.cpp") | ||
| set(AES_ENCRYPT_LIBRARIES "crypto" "gpgme") | ||
| endif() |
There was a problem hiding this comment.
The variables should be set as empty outside of the conditional. Otherwise newer CMake versions will report a warning for using an uninitialized variable.
There was a problem hiding this comment.
Addressed. Thank you.
* Implement Bag encryption/decryption. (ros#1206) Ported to 1.12.12 * Python Bag class supports encryption; all the rosbag tools work for encrypted bags. (ros#1206) * Improve exception messages raised when a public key is missing. * Randomize initialization vectors for encrypt/decrypt. * Fix bag encryption routine (ros#1310) Bag encryption routine was truncating the recorded block by the size of the IV. * Drop const qualifier from *::decryptChunk methods Since decryption can change EVP's context, these methods can't be const anymore. * Move encryption to from openssl software aes to EVP API * Check EVP API results * Add EncryptionOptions to Recorder * Parse encryption and encryption-param options in the record executable * Fix gtests * Fix rostests - With ninja, when `_rostest_ARGS` is empty, the space right before it gets escaped, and the command that ultimately gets executed has a trailing slash. - rospy.log testing fails because our ROSCONSOLE_FORMAT does not print severity - bag.py had a bug in get_info_str() that has been fixed upstream - bz2 performs a few bytes better than expected, failing the rosbag compression test - roswtf tests had an outdated dependency list (TBH I don't understand what this list is)
|
@ros-pull-request-builder retest this please |
|
ROS Melodic CI failing due to missing rosdep. Should be resolved by ros/rosdistro#17588. |
|
@ros-pull-request-builder retest this please |
|
@jwon02 Can you investigate the CI failure which occurred building on Ubuntu Bionic? |
69030f2 to
cd924cc
Compare
cd924cc to
ac8ebd9
Compare
|
Remaining failure is known to be flaky. Thanks for the contribution here, @jwon02 and @guillaumeautran! |
| // Test the message decrypted from the bag file | ||
| bag.open(bag_file_name, rosbag::bagmode::Read); | ||
| rosbag::View view(bag); | ||
| EXPECT_EQ(view.size(), 1); |
There was a problem hiding this comment.
This line might be responsible for the new compiler warning: http://build.ros.org/job/Mdev__ros_comm__ubuntu_bionic_amd64/20/warnings22Result/package.-1053464660/
PS: the warning was also visible in the PR build...
There was a problem hiding this comment.
Should I create another PR to remove the warning?
There was a problem hiding this comment.
That would be great. Thanks.
|
I just noticed that my previous raised concern about the @jwon02 @mikepurvis This needs to be resolved as soon as possible. |
|
Oh blah, that is a fail on my part. The three options given in the original discussion were:
So I don't think I have the necessary permissions to create a new repo for rosconsole, but that would be my preferred resolution to this. If such a repo can be created, I can put up the PRs to drop rosconsole from this repo and add it to the new one. |
|
I will double check the hierarchy first thing Monday morning but I agree that it seems possible to move If all is good I will duplicate this repo and remove |
|
See ros/rosdistro#17919 for tracking the migration. |
|
There's an additional piece of fallout from this PR that we are now seeing on the buildfarm: http://build.ros.org/job/Mbin_ubhf_uBhf__swri_console__ubuntu_bionic_armhf__binary/2/console (look near the end for the error). Looking at the documentation for GPGME, it looks like we'll probably have to add -D_FILE_OFFSET_BITS=64 to make anything that does |
|
I'm testing a potential fix over on https://github.com/ros/ros_comm/tree/armhf-file-offset-bits |
This PR adds plugins to the
Bagclass to encrypt/decrypt chunks and connection records in a bag file. By default, theNoEncryptorplugin is loaded to generate backward-compatible unencrypted bag files. LoadAesCbcEncrytproto encrypt the bag contents using a GPG key installed in the system.ABI compatibility is broken by two new members added to
Bag.