Config encrypt

[VakarisZ]

What does this PR do?

Fixes #1172

Add any further explanations here.

PR Checklist

• Have you added an explanation of what your changes do and why you'd like to include them?
• Is the TravisCI build passing?
• Was the CHANGELOG.md updated to reflect the changes?
• Was the documentation framework updated to reflect the changes?

Testing Checklist

• Added relevant unit tests?
• Have you successfully tested your changes locally? Elaborate:

  Tested by unit tests of encryption/decryption
  Manual tests of different config imports/exports

• If applicable, add screenshots or log transcripts of the feature working
  
  
  

[ghost]

DeepCode's analysis on #57f35f found:

• ℹ️ 2 minor issues. 👇

Top issues

Description	Example fixes
No catch method for promise. This may result in an unhandled promise rejection. Occurrences: ExportConfigModal.tsx:26 ImportConfigModal.tsx:44	🔧 Example fixes
Missing close for open, add close or use a with block. Occurrences: conftest.py:19	🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin.

[VakarisZ]

Do we need to add documentation about this? I'm not sure.

[mssalvatore]

I've done a partial review of this. Before we spend more time on this, I'd like to make sure we checked with Barak that this is the best path. I think it's a better user experience just to give them a checkbox that lets them exclude sensitive information from the export. It's fewer clicks, less complex, and we don't have to worry about any compliance issues.

Additionally, this project is already more complex than we our team can really handle. We need to be removing complexity, not adding it. I think a warning to the user that the exported config contains the credentials is all that's really necessary to mitigate any security risk. Adding a checkbox to exclude the credentials is also straight forward and requires no changes to the import process.

Finally, since the scenarios feature will fundamentally change the way that users interact with configuration, I don't think we should be investing too much time adding in complexities to the existing import/export capability.

In summary, I don't think the cost of the complexity is worth it.

[VakarisZ]

I think it's a better user experience just to give them a checkbox that lets them exclude sensitive information from the export. It's fewer clicks, less complex, and we don't have to worry about any compliance issues.

More complex to implement and maintain, as we can't always know what's sensitive and what's not. What compliance issues do we worry about? Just make the encryption an industry standard and if it's not enough user can always encrypt manually.

I think a warning to the user that the exported config contains the credentials is all that's really necessary to mitigate any security risk. Adding a checkbox to exclude the credentials is also straight forward and requires no changes to the import process.

So a warning means that the user needs a popup or some other UI element to be introduced to the security concern. If we need this, we might as well offer a proper solution to this problem right there and then. Re-entering all of the sensitive fields is definitely not more UX friendly than just entering a password for an export.

Finally, since the scenarios feature will fundamentally change the way that users interact with configuration, I don't think we should be investing too much time adding in complexities to the existing import/export capability.

I don't see any fundamental changes for configuration on the horizon. Scenarios will use configuration, so scenario export encryption == config export encryption.

In summary, I don't think the cost of the complexity is worth it.

Maybe a better solution would've been to just encrypt by default based on user's credentials. The argument that we'd get a lot of support emails is not that valid IMO, for multiple reasons:

1. We don't get any support emails about config imports even though our exported configs are not compatible among different versions.
2. Because sharing configs is not something you would do (except for dev). The typical use case is that there's only one person for one island on the network.
3. Credential reset is not that common either. What's the main reason for credential change?

[codecov]

Codecov Report

Merging #1204 (abaeafc) into develop (e6bb481) will increase coverage by 0.09%.
The diff coverage is 33.64%.

❗ Current head abaeafc differs from pull request most recent head 57f35f9. Consider uploading reports for the commit 57f35f9 to get more accurate results

@@             Coverage Diff             @@
##           develop    #1204      +/-   ##
===========================================
+ Coverage    28.63%   28.73%   +0.09%     
===========================================
  Files          427      434       +7     
  Lines        12888    13192     +304     
===========================================
+ Hits          3691     3791     +100     
- Misses        9197     9401     +204     

Impacted Files	Coverage Δ
monkey/monkey_island/cc/app.py	0.00% <0.00%> (ø)
...monkey_island/cc/resources/configuration_export.py	0.00% <0.00%> (ø)
...monkey_island/cc/resources/configuration_import.py	0.00% <0.00%> (ø)
monkey/common/utils/exceptions.py	100.00% <100.00%> (ø)
...nkey_island/cc/services/utils/config_encryption.py	100.00% <100.00%> (ø)
monkey/monkey_island.py	0.00% <0.00%> (ø)
monkey/monkey_island/cc/server_setup.py	0.00% <0.00%> (ø)
monkey/monkey_island/cc/setup/mongo/mongo_setup.py	0.00% <0.00%> (ø)
...key/monkey_island/cc/server_utils/island_logger.py	100.00% <0.00%> (ø)
... and 8 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6bb481...57f35f9. Read the comment docs.