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

Use Sodium for secret encryption and decryption #128

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

jkmassel
Copy link
Contributor

@jkmassel jkmassel commented Jun 8, 2020

Fixes an openSSL warning:

openssl aes-256-cbc -md sha256 -d -in .circleci/.firebase.secrets.json.enc -out .circleci/.firebase.secrets.json -k “${FIREBASE_SECRETS_ENCRYPTION_KEY}”
*** WARNING : deprecated key derivation used.
Using -iter or -pbkdf2 would be better.

Also gets us out of manual crypto, but the tradeoff is an additional homebrew dependency. IMHO this isn't a huge deal given the existence of wordpress-mobile/WordPress-iOS#14100, but we should wait to merge this in until the libsodium dependency can be added to that script.

This is a breaking change and should be carefully merged to avoid breaking projects.

To Test

  1. Check out the code and make sure it looks ok (including test changes)
  2. Ensure that CI passes

@jkmassel jkmassel added the enhancement New feature or request label Jun 8, 2020
@jkmassel jkmassel self-assigned this Jun 8, 2020
@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2020

Codecov Report

Merging #128 into develop will decrease coverage by 0.40%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #128      +/-   ##
===========================================
- Coverage    45.04%   44.64%   -0.41%     
===========================================
  Files           81       81              
  Lines         3563     3537      -26     
===========================================
- Hits          1605     1579      -26     
  Misses        1958     1958              
Impacted Files Coverage Δ
...ugin/wpmreleasetoolkit/helper/encryption_helper.rb 100.00% <100.00%> (ø)
spec/encryption_helper_spec.rb 100.00% <100.00%> (ø)

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 f04b19f...9bab7ae. Read the comment docs.

@jkmassel jkmassel requested a review from loremattei June 11, 2020 04:43
jkmassel added 2 commits June 10, 2020 22:44
Fixes an openSSL warning:

```
openssl aes-256-cbc -md sha256 -d -in .circleci/.firebase.secrets.json.enc -out .circleci/.firebase.secrets.json -k “${FIREBASE_SECRETS_ENCRYPTION_KEY}”
*** WARNING : deprecated key derivation used.
Using -iter or -pbkdf2 would be better.
```

Also gets us out of manual crypto.

This is a breaking change and should be carefully merged to avoid breaking projects.
@jkmassel jkmassel force-pushed the use/sodium-for-secret-encryption branch from 991d4a0 to 9bab7ae Compare June 11, 2020 04:45
Copy link
Contributor

@loremattei loremattei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkmassel should we add a check to verify that libsodium is installed? It looks like it fails with an impossible to understand error if it's not 😅

Also, I tested bundle exec fastlane run configure_update on a test branch forked out the current WPiOS develop and I get this error when it tries to re_apply:

RbNaCl::LengthError: [!] Nonce was 25 bytes (Expected 24)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants