Enable use of Cyclone DDS security features #123

SidFaber · 2020-03-20T21:10:27Z

Add utility function to insert security settings to the cyclone QOS object used to create nodes. Include a utility to find security files and properly format their location to use with DDS.

Code testing included running the ROS2 tutorials with encryption enabled, error checking for missing certificates, testing bad signatures on governance and permissions files, invalid or mismatched domain IDs, and invalid encryption keys.

Add utility function to insert security settings to the cyclone QOS object used to create nodes. Include a utility to find security files and properly format their location to use with DDS. Signed-off-by: Sid Faber <sid.faber@canonical.com>

SidFaber · 2020-03-20T21:20:55Z

This does not pay attention to the ENABLE_SECURITY Cmake option because Cyclone does not generate an include file that uses the option as far as I can tell.

@eboasson, is this something we want to add?

joespeed · 2020-03-21T00:16:55Z

@rotu because rmw

eboasson

Wonderful @SidFaber, thank you! I have only seen one trivial detail in addition to the rather important question concerning the ENABLE_SECURITY option. Here I see two distinct issues:

successfully building the code regardless of the version of Cyclone that is used (the RMW layer covers all versions of Cyclone and of ROS2 since May 2019, no need to throw that away); and
certainty that security settings will never be silently ignored.

The first is an issue because the dds_qset_prop function does not yet exist in the master branch of Cyclone DDS. It got added in the security branch and will only show up in master when the support for security gets merged into it. Simply merging this PR now will therefore cause all ROS2 CI builds to fail. My preference is to define a macro DDS_HAS_QOS_PROPERTY_LIST in Cyclone's public header files if the property list QoS is implemented, and test for that in the RMW layer.

The second has two parts, in my view. One is that rmw_create_node must fail if enforce_security is set but DDS_HAS_QOS_PROPERTY_LIST is not defined.

The other is that a build of Cyclone without support for security needs to refuse the creation of a participant for which some of the DDS security properties have been set. That's straightforward addition to the code in the security branch.

With these changes, I think all bases will be covered. Do you (and @dennis-adlink and @rotu) agree?

eboasson · 2020-03-23T13:19:14Z

README.md

@@ -30,6 +30,5 @@ DDS directly instead of via the ROS2 abstraction.

 ## Known limitations

-Cyclone DDS doesn't yet implement the DDS Security standard, nor does it fully implement


Argh! The lifespan, deadline and liveliness QoS are fully supported nowadays, there aren't really any known limitations anymore! To be fixed in another PR, obviously.

eboasson · 2020-03-23T13:27:40Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+    qos, "dds.sec.access.permissions", "permissions.p7s",
+    security_options);
+
+  dds_qset_prop(qos, "dds.sec.auth.library.path", "libdds_security_auth.so");


The "lib" and ".so" affixes will cause trouble on macOS and Windows. If the specified file does not exist, Cyclone also tries the names extended in the platform's native way (so lib/.dylib for macOS and .dll for Windows). So just "dds_security_auth" seems better (and analogously for the other libraries).

Updated in bca0852

mikaelarguedas

That's fantastic!

I could get this to run out-of-the-box on bionic! however I had issues on Focal.
As I also had issues with other rmw implementations on that platform I opened a meta ticket at ros2/sros2#180

mikaelarguedas · 2020-03-22T09:52:01Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

 #include "rcutils/get_env.h"
 #include "rcutils/logging_macros.h"
 #include "rcutils/strdup.h"
+#include "rcutils/format_string.h"


Nit: alphabetical order for includes

Updated in bca0852

mikaelarguedas · 2020-03-22T10:00:59Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

@@ -666,7 +742,13 @@ extern "C" rmw_node_t * rmw_create_node(
  static_cast<void>(domain_id);
  const dds_domainid_t did = DDS_DOMAIN_DEFAULT;
 #endif
-  (void) security_options;
+
+  if (security_options == nullptr) {


Alternative: Use RCUTILS_CHECK_ARGUMENT_FOR_NULL macro instead

Updated with 1ca2269

mikaelarguedas · 2020-03-22T10:04:21Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

@@ -688,8 +770,18 @@ extern "C" rmw_node_t * rmw_create_node(
 #endif

  dds_qos_t * qos = dds_create_qos();
+  if (qos == nullptr) {


Same nit here, could use RCUTILS_CHECK_FOR_NULL_WITH_MSG instead

@mikaelarguedas, found that this needs to do a bit of cleanup before so this is still here

mikaelarguedas · 2020-03-23T16:19:22Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+    qos, "dds.sec.access.permissions", "permissions.p7s",
+    security_options);
+
+  dds_qset_prop(qos, "dds.sec.auth.library.path", "libdds_security_auth.so");


this looks platfrom specific, maybe there is support for the library basename and cyclone then modifies and look for specific names based on the platform (ala class_loader https://github.com/ros/class_loader/blob/11982ab3efc390608f10f2db1e97edf4037afec5/src/class_loader.cpp#L51-L73)

Fixed with bca0852

Signed-off-by: Sid Faber <sid.faber@canonical.com>

SidFaber · 2020-03-23T21:29:15Z

... My preference is to define a macro DDS_HAS_QOS_PROPERTY_LIST in Cyclone's public header files if the property list QoS is implemented, and test for that in the RMW layer.

@eboasson do you plan to add this to the security branch of Cyclone? If so I'll update this with some code that should handle the items you pointed out. Nice catch on preventing a silent fail-open scenario!

eboasson · 2020-03-24T09:38:03Z

... My preference is to define a macro DDS_HAS_QOS_PROPERTY_LIST in Cyclone's public header files if the property list QoS is implemented, and test for that in the RMW layer.

@eboasson do you plan to add this to the security branch of Cyclone? If so I'll update this with some code that should handle the items you pointed out. Nice catch on preventing a silent fail-open scenario!

Yep, working on it: eclipse-cyclonedds/cyclonedds#448

SidFaber · 2020-03-25T22:56:59Z

Yep, working on it: eclipse-cyclonedds/cyclonedds#448

@eboasson, I have updates here ready to land once this is ready, thanks for the improvements

eboasson · 2020-03-26T07:47:39Z

@SidFaber FIY: I just merged #448

Add in conditional compile based on ENABLE_SECURITY make flag and Cyclone DDS feature availability. Also addressed review comments. Signed-off-by: Sid Faber <sid.faber@canonical.com>

SidFaber · 2020-03-26T18:36:38Z

@eboasson, @mikaelarguedas, thanks for your comments, they should be all addressed in bca0852. Important to note that now node creation fails when security is requested (ROS_SECURITY_ENABLE=true); this change the current fail-open behavior to fail-closed which should be desired.

eboasson

Thanks @SidFaber! It works fine — also on Dashing, which I checked just to be sure. I'd have merged it if it weren't for 4 details, and I am not even sure I didn't overlook one or two before.

I'd have done GitHub suggestions if it would allow me (sometimes it does, sometimes it doesn't), so I have little choice but to ask you to go over them.

eboasson · 2020-03-27T11:55:39Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

@@ -59,6 +61,7 @@
 #include "namespace_prefix.hpp"

 #include "dds/dds.h"
+#include "dds/ddsc/dds_public_qos.h"


This is included automatically by dds/dds.h, and I'm not so sure it will won't change name or location in the future

Removed in 1ca2269

eboasson · 2020-03-27T11:57:21Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+rmw_ret_t configure_qos_for_security(
+  dds_qos_t * qos, const rmw_node_security_options_t * security_options)
+{
+


This empty line triggers a cpplint warning:

/Users/erik/ros2_ws/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp:698: Redundant blank line at the start of a code block should be deleted. [whitespace/blank_line] [2]

Removed in 1ca2269

eboasson · 2020-03-27T12:00:43Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

@@ -688,8 +789,17 @@ extern "C" rmw_node_t * rmw_create_node(
 #endif

  dds_qos_t * qos = dds_create_qos();
+  RCUTILS_CHECK_FOR_NULL_WITH_MSG(
+    security_options, "rmw_create_node: Unable to create qos", return nullptr);


I suspect this was meant to check qos rather than security_options; and if bailing out, a domain will have been instantiated/refcounted already by check_create_domain_locked and, as noted, its work must be undone using node_gone_from_domain_locked (see a few lines below).

Fixed with 1ca2269, thanks for catching!

eboasson · 2020-03-27T12:02:09Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+
+  if (security_options->enforce_security) {
+    if (configure_qos_for_security(qos, security_options) != RMW_RET_OK) {
+      return nullptr;


This, too, can leak the domain reference, and moreover leaks qos. Isn't C just a lovely language, always being on the lookout for ways to get you? 😄

Isn't C just a lovely language, always being on the lookout for ways to get you?

Regarding that: such issues can be significantly reduced by making more substantial use of C++ features here. Can you explain why this is essentially C?

Not really, the RMW layer is intended to be in C++. Part of the reason that it is so C-like is that I am much more experienced with C than with C++ and so whenever I try to write C++ it still ends up looking like C. There is also the fact that Cyclone is in C, that this is therefore relying on a C API, and that some of the niceties of C++ (like destructors) are simply not available on these objects.

There is an implementation of a standard C++11 API of DDS waiting for approval by the Eclipse Foundation's IP review process. That interface covers all the standard features but not all the additional tricks. At some point it might be worth rewriting the RMW implementation to use that API, it's only about a handful of lines of code anyway.

Part of the reason that it is so C-like is that I am much more experienced with C than with C++ and so whenever I try to write C++ it still ends up looking like C.

Fair enough, that's a perfectly valid reason.

There is also the fact that Cyclone is in C, that this is therefore relying on a C API, and that some of the niceties of C++ (like destructors) are simply not available on these objects.

In case you're unaware, shared/unique ptrs can be handed custom deleters that might help with that particular issue.

Anyway, I don't want to digress from the PR too much, just found myself wondering about this. Thanks for the info!

In case you're unaware, shared/unique ptrs can be handed custom deleters that might help with that particular issue.

That sounds like something worth reading up on, thanks!

@eboasson better yet, define a destructors for your classes. Then you don't need to pass a custom deleter to unique_ptr/shared_ptr

@rotu that's not always possible:

[...] this is therefore relying on a C API, and that some of the niceties of C++ (like destructors) are simply not available on these objects.

My suggestion related particularly to that. Another common option is to wrap such C objects in a class responsible for managing it, but the trade off is more code to maintain. It also assumes you're comfortable with those C++ constructs.

I find it's easier to maintain a wrapper class than a deleter, but YMMV. Where there's a matched initializer + deleter pair, I find it easiest to make them into a constructor + destructor. With a factory method + deleter pair, i prefer to use a class method + protected constructor + destructor.

@eboasson, this should all be covered with 1ca2269.

mikaelarguedas · 2020-03-27T14:01:10Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

  std::string user_data = get_node_user_data(name, namespace_);
  dds_qset_userdata(qos, user_data.c_str(), user_data.size());
+
+  if (security_options->enforce_security) {


This logic is incomplete, it sets the security QoS only if enforce_security is set.

The expected behavior is to:
If the security files can be found: setup the plugins
If they cannot be found:

if enforce_security is true -> exit with error message

else -> create a participant without the security plugins instanciated

This is most likely the cause of the failing tests at ros2/system_tests#408

@mikaelarguedas this is how the other middleware behaves, but help me understand if this is the behavior we truly want.

The rcl sets security_options->enforce_security to either RMW_SECURITY_ENFORCEMENT_PERMISSIVE or RMW_SECURITY_ENFORCEMENT_ENFORCE. It's always set to RMW_SECURITY_ENFORCEMENT_PERMISSIVE when ROS_SECURITY_ENABLE is anything other than "true"; that is why it's used to completely bypass security qos here.

With what you propose security will be enabled anytime ROS_SECURITY_ROOT_DIRECTORY exists regardless of ROS_SECURITY_ENABLE and ROS_SECURITY_STRATEGY. The rcl only checks for the directory, it (rightfully) does not ensure all six security files exist and are properly formatted. So node creation would fail any time the middleware cannot start with files in the security directory, even when ROS_SECURITY_ENABLE is set to false.

Similarly ROS_SECURITY_ENABLE=false would not disable security for testing. Instead security would have to be disabled by changing ROS_SECURITY_ROOT_DIRECTORY or moving the security directory on disk.

If don't think the described behavior matches how the other middlewares behave, or the desired behavior.
Overall the expected behavior is the one described at: https://design.ros2.org/articles/ros2_dds_security.html

Putting it in a table for ease of read:

ROS_SECURITY_ENABLE ROS_SECURITY_STRATEGY valid set of security files located under ROS_SECURE_ROOT_DIRECTORY Expected outcome

false * * No security plugins instanciated, creating non-secure participant

true Enforce Yes creates secure participant

true Enforce No fails to create participant

true Permissive Yes creates secure participant

true Permissive No creates unsecure participant

Anything not matching that behavior is a bug IMO.

I agree that the line you are referring to is misleading https://github.com/ros2/rcl/blame/73948da4c50b11d5d133e068b804ce7c2b4f9cb6/rcl/src/rcl/node.c#L316-L317 and in practice serve no purpose and at first glance could be removed completely.
The one that matters in our case is the else branch of that if: https://github.com/ros2/rcl/blame/73948da4c50b11d5d133e068b804ce7c2b4f9cb6/rcl/src/rcl/node.c#L318-L323
If ROS_SECURITY_ENABLE is false: node_security_options.security_root_path is not set and this is the variable that tells you if you should try to instantiate the security plugins or not.

Mimicking the logic of the other RMWs could be a good hint of how to implement it here: https://github.com/ros2/rmw_fastrtps/blob/bd5507b61c7285d6083e514ccd49f00b2945baeb/rmw_fastrtps_shared_cpp/src/rmw_node.cpp#L292

@mikaelarguedas, thanks for the discussion. It was simple to switch the logic here, but you'll also see changes to configure_qos_for_security: all files are checked before any properties are added to qos to enable cyclone dds security.

This is now passing all everything in the test_security, that's been a great help and I'd like to see those test routines continue to mature!

That's awesome.

all files are checked before any properties are added to qos to enable cyclone dds security.

Great improvement, we should consider modifying the other rmw implementations to make sure they do the same

This is now passing all everything in the test_security, that's been a great help and I'd like to see those test routines continue to mature!

🎉

I didnt test the new version as it's conflicting since #106

@SidFaber I gave it a go (with Dashing to verify it supports that, too) and I noticed that it warns

[INFO] [rmw_cyclonedds_cpp]: rmw_create_node: Unable to configure security

whenever it starts when security either isn't supported or couldn't be configured. That is, when security is:

disabled, regardless of whether security support is included in the build;

enabled in permissive mode, when security support not included in the build;

enabled in permissive mode, when security support is included in the build but something is awry with the configuration.

Case (1) seems wrong (there is no expectation of security so also no expectation of getting warnings about it) and case (3) seems like a good thing to me. I am not quite sure what to make of case (2), I'd say it should be a hard fail but perhaps that's not the intent behind permissive mode (I don't quite get that mode anyway). @mikaelarguedas could you comment on that?

Just for completeness: if security is enabled in enforced mode, it only starts when the security configuration is valid and the implementation supports it.

Apart from the warning not being appropriate in all cases, it looks good to me!

Case (1) seems wrong

👍

case (3) seems like a good thing to me

👍

I am not quite sure what to make of case (2), I'd say it should be a hard fail but perhaps that's not the intent behind permissive mode (I don't quite get that mode anyway)

Originally the "Permissive" mode is to allow someone to launch in a single environment a system with some parts using security and some parts without.
I don't think it's been used much in practice.
In the case of 2, the rmw implementation itself doesnt support security (because not included in the build) so there's no chance of having any part of the system using security. So IMO it should hard fail and provide a clearer error message as of why.

An example is such error message in rmw_fastrtps_cpp https://github.com/ros2/rmw_fastrtps/blob/48b403a211b269ab383e6adfcf89d1b4333b2c39/rmw_fastrtps_shared_cpp/src/participant.cpp#L238-L242

How about I just give an info message "Security is enabled" when the cyclone QOS object gets set up properly? It will normally follow the "found security directory" message that comes from rcl. I feel some messaging is important since the rcl message implies that security is enabled which may not be the case.

As I find the rcl message by itself pretty noisy when launching systems with many nodes, I'd rather not add an info message always. I would prefer a warning when users need to know something is not what they requested (case (3)), and nothing (or debug message) otherwise.

@mikaelarguedas, I just removed the info message: there are some tests where you'll get an rcl "found security" message and create a plain text node which is similar to how other middleware behaves. Although not ideal, this is something that would be better solved with logging.

Also remove superfluous include and blank line. Signed-off-by: Sid Faber <sid.faber@canonical.com>

rmw_cyclonedds_cpp/src/rmw_node.cpp

Properly handle downstream effects of ROS_SECURITY_STRATEGY and ROS_SECURITY_ENABLE environment variables through security_options. Improve memory management and make sure to only set security qos properties when all files are sure to exist.

Fix residue from merge conflict in README.md

SidFaber · 2020-04-07T20:04:13Z

This has been updated for the recent context changes.

@eboasson, I followed the existing style to conditionally include based on rmw versions. fastrtps has diverged from this code now as their latest update does more with contexts, but everything here seems to be working fine.

@mikaelarguedas, this passed the current test_security after changing the makefile slightly to also test rmw_cyclonedds_cpp.

Thanks for your help with this one!

eboasson

@SidFaber thank you very much for these updates. I double-checked the builds on Foxy, as well as the full run of all combinations on Dashing and it all works as expected.

I'm good with the updated warning policy as well, especially now that I have discovered that the rcl security code is also as lax as it can possibly be. It appears setting ROS_SECURITY_ENABLE to anything other than "true" (exact match) is silently interpreted as "false", and if enabled, "permissive" mode is the result of setting ROS_SECURITY_STRATEGY to anything other than "Enforce". Given the number of people that have been involved in it, I can only presume this to be the intent. With that background, the current warning policy fits nicely.

I'm taking @mikaelarguedas thumbs-up as an approval as well, so I'm merging it. I'm working on getting the changes in contexts/nodes into Cyclone's RMW and that'll allow me to also include your work in that process.

ivanpauno · 2020-04-28T19:56:54Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+  RMW_SET_ERROR_MSG(
+    "Security was requested but the Cyclone DDS being used does not have security "
+    "support enabled. Recompile Cyclone DDS with the '-DENABLE_SECURITY=ON' "
+    "CMake option");


I'm always getting this error message if I build a ros2 workspace without passing -DENABLE_SECURITY=ON and run any demo (using cyclone)
I think rmw_cyclonedds_cpp should be able to automatically detect if security is available or not at build time.

You're right, this error message should probably only be set if security was specifically requested by the user, but that's only checked in the above branch. @SidFaber what do you think, add an if (security_options->security_root_path != nullptr) here?

See #172 for a possible fix.

This should only happen when ROS_SECURITY_STRATEGY=Enforce and ROS_SECURITY_ENABLE=True. The full interplay of environment variables, RCL and RMW_CYCLONE is outlined here, the message should only show in lines 19, 20 and 22.

@ivanpauno, can you confirm that is or is not what you're seeing? This should work with both the current and the security branches of rmw_cyclonedds_cpp. Thanks!

@ivanpauno, can you confirm that is or is not what you're seeing? This should work with both the current and the security branches of rmw_cyclonedds_cpp. Thanks!

I'm seeing this error without setting any security related environment variable.

#175 should fix this up, thanks for pointing this out.

Enable use of Cyclone DDS security features

99d2738

Add utility function to insert security settings to the cyclone QOS object used to create nodes. Include a utility to find security files and properly format their location to use with DDS. Signed-off-by: Sid Faber <sid.faber@canonical.com>

eboasson reviewed Mar 23, 2020

View reviewed changes

mikaelarguedas reviewed Mar 23, 2020

View reviewed changes

SidFaber added a commit to SidFaber/rmw_cyclonedds that referenced this pull request Mar 23, 2020

Addressed comments from PR ros2#123

072b4c2

Signed-off-by: Sid Faber <sid.faber@canonical.com>

eboasson mentioned this pull request Mar 24, 2020

Improvements to building against & using master/security-enabled/security-but-disabled versions eclipse-cyclonedds/cyclonedds#448

Merged

kyrofa mentioned this pull request Mar 24, 2020

ROS 2 Foxy Fitzroy ros2/ros2#830

Closed

3 tasks

Update conditional compile logic

bca0852

Add in conditional compile based on ENABLE_SECURITY make flag and Cyclone DDS feature availability. Also addressed review comments. Signed-off-by: Sid Faber <sid.faber@canonical.com>

SidFaber force-pushed the dds-security branch from 99071d4 to bca0852 Compare March 26, 2020 18:05

eboasson reviewed Mar 27, 2020

View reviewed changes

mikaelarguedas mentioned this pull request Mar 27, 2020

Run test_security on CycloneDDS as well ros2/system_tests#408

Merged

mikaelarguedas reviewed Mar 27, 2020

View reviewed changes

Fix memory leaks

5e93420

Also remove superfluous include and blank line. Signed-off-by: Sid Faber <sid.faber@canonical.com>

SidFaber commented Mar 27, 2020

View reviewed changes

rmw_cyclonedds_cpp/src/rmw_node.cpp Outdated Show resolved Hide resolved

SidFaber added 3 commits April 2, 2020 12:36

Improve security logic and memory management

1ca2269

Properly handle downstream effects of ROS_SECURITY_STRATEGY and ROS_SECURITY_ENABLE environment variables through security_options. Improve memory management and make sure to only set security qos properties when all files are sure to exist.

Merge branch 'master' into dds-security

581ba38

Removed stray newline

68d8b52

Fix residue from merge conflict in README.md

eboasson mentioned this pull request Apr 7, 2020

API changes to sync with one Participant per Context change in rmw_fastrtps #106

Merged

Merge remote-tracking branch 'origin/master' into dds-security

16134dc

eboasson approved these changes Apr 8, 2020

View reviewed changes

eboasson merged commit d8a8d7f into ros2:master Apr 8, 2020

eboasson mentioned this pull request Apr 8, 2020

Confusion over versioning #128

Closed

mikaelarguedas mentioned this pull request Apr 8, 2020

Foxy support ros2/sros2#180

Closed

ivanpauno reviewed Apr 28, 2020

View reviewed changes

kyrofa deleted the dds-security branch April 28, 2020 20:01

SidFaber mentioned this pull request Apr 29, 2020

Address error message #175

Merged

		@@ -30,6 +30,5 @@ DDS directly instead of via the ROS2 abstraction.

		## Known limitations

		Cyclone DDS doesn't yet implement the DDS Security standard, nor does it fully implement

ROS_SECURITY_ENABLE	ROS_SECURITY_STRATEGY	valid set of security files located under ROS_SECURE_ROOT_DIRECTORY	Expected outcome
false	*	*	No security plugins instanciated, creating non-secure participant
true	Enforce	Yes	creates secure participant
true	Enforce	No	fails to create participant
true	Permissive	Yes	creates secure participant
true	Permissive	No	creates unsecure participant

Enable use of Cyclone DDS security features #123

Enable use of Cyclone DDS security features #123

Conversation

SidFaber commented Mar 20, 2020

SidFaber commented Mar 20, 2020

joespeed commented Mar 21, 2020

eboasson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelarguedas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SidFaber Mar 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SidFaber commented Mar 23, 2020

eboasson commented Mar 24, 2020

SidFaber commented Mar 25, 2020

eboasson commented Mar 26, 2020

SidFaber commented Mar 26, 2020

eboasson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa Mar 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa Mar 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SidFaber commented Apr 7, 2020

eboasson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SidFaber Mar 27, 2020 •

edited

Loading

kyrofa Mar 27, 2020 •

edited

Loading

kyrofa Mar 27, 2020 •

edited

Loading

kyrofa Apr 2, 2020 •

edited

Loading

kyrofa Apr 28, 2020 •

edited

Loading