[match] improve encryption internals, solving flaky test (#21663) #21790

lacostej · 2024-01-10T19:11:35Z

Checklist

I've run bundle exec rspec from the root directory to see all new and existing tests pass
I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
I see several green ci/circleci builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)
I've read the Contribution Guidelines
I've updated the documentation if necessary.
I've added or updated relevant unit tests.

Motivation and Context

Resolves #21663

Description

See discussion in above issue.

Testing Steps

match/lib/match/encryption/encryption.rb

lacostej · 2024-01-10T19:37:08Z

Well apart from the new #21792 flaky test, the proposed implementation won't work with all openssl installations

     # ./match/spec/importer_spec.rb:122:in `block (3 levels) in <top (required)>'
     # ./.bundle/ruby/3.1.0/gems/webmock-3.18.1/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # RuntimeError:
     #   unsupported cipher algorithm (AES-256-GCM)
     #   ./match/lib/match/encryption/encryption.rb:49:in `initialize'

fastlane/lib/fastlane/actions/docs/sync_code_signing.md

lacostej · 2024-01-10T20:35:03Z

match/lib/match/encryption/encryption.rb

+      private
+
+      def keyivgen(cipher, password, salt, hash_algorithm)
+        cipher.pkcs5_keyivgen(password, salt, 1, hash_algorithm)


I considered increasing this very low iteration value, but changing the value requires changing the encryption format anyway, so let's just change the cipher etc.

bin/match_enc

fastlane/lib/fastlane/actions/docs/sync_code_signing.md

match/lib/match/encryption/encryption.rb

milch · 2024-01-10T20:38:24Z

match/lib/match/encryption/encryption.rb

+    # salt should be randomly generated and provided unencrypted (like in the current implementation)
+    # key is generated with OpenSSL::KDF::pbkdf2_hmac with properly chosen parameters
+    # Short explanation about salt and IV: https://stackoverflow.com/a/1950674/6324550
+    class EncryptionV2


Just as a suggestion but nothing blocking, I think this could be refactored so that the IV gen happens in one class per algorithm, and then there's a single encrypt/decrypt function that takes in the IV gen class. I think overall it would just cut down on the duplication a little bit.

You mean as passing the ivgen function as a "strategy" to the same code that is shared between V1 and V2?

I thought about it, but decided against for now because

I preferred to keep V2 on its own, completely unrelated to common code with V1, to make it more readable.

There used to be a few lines, and now there are already 3 classes. I felt it was maybe enough abstractions

match/lib/match/encryption/encryption.rb

lacostej · 2024-01-11T10:55:36Z

match/lib/match/encryption/encryption.rb

+
+    # by default, encrypt or decrypt in place
+    class MatchFileEncryption
+      def encrypt(file_path, password, output_path = nil)


Another question I have: do we want to enforce keywords parameters instead?

I think it usually makes for better API management and backwards compatibility.

Should I update this file and use keywords args instead?

I'm all for keyword parameters whenever we can 🙏

I added some but not in all classes. Makes sense or inconsistent?

rogerluan

Just some nitpick comments about docs and formatting, as I don't quite grasp the whole encryption details 😅 but the overall logic LGTM and I liked your approach of keeping this backwards compatible by trying both V1 and V2 👌

rogerluan · 2024-01-14T13:22:47Z

fastlane/lib/fastlane/actions/docs/sync_code_signing.md

+match_file encrypt "<fileYouWantToEncryptPath>" ["<encryptedFilePath>]"
+
+match_file decrypt "<fileYouWantToDecryptPath>" ["<decryptedFilePath>]"


Shouldn't this be bin/match_file instead of match_file?

Also, this looks like a typo:

Suggested change

match_file encrypt "<fileYouWantToEncryptPath>" ["<encryptedFilePath>]"

match_file decrypt "<fileYouWantToDecryptPath>" ["<decryptedFilePath>]"

match_file encrypt "<fileYouWantToEncryptPath>" "<encryptedFilePath>"

match_file decrypt "<fileYouWantToDecryptPath>" "<decryptedFilePath>"

I think that once you install the gem, everything under bin will be in your CLASSPATH. SO match_file should be enough.

The [] were because the second parameter is optional and match_file decrypts / encrypts in place when omitted.

ooh 😮 gotcha. I didn't know about any of those 2 info you said haha good to know 💪

match/lib/match/encryption/encryption.rb

rogerluan · 2024-01-14T13:29:29Z

match/lib/match/encryption/encryption.rb

+
+    # by default, encrypt or decrypt in place
+    class MatchFileEncryption
+      def encrypt(file_path, password, output_path = nil)


I'm all for keyword parameters whenever we can 🙏

match/lib/match/encryption/encryption.rb

bin/match_file

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

…t lost

bin/match_file

rogerluan

LGTM! 🚀 thanks for addressing the CTRL+C exit 😊

arc-v2 · 2024-04-07T17:22:21Z

Hi @lacostej I'm experiencing some issue with decryption since 2.220.0 was released. Could you confirm the following query:
Does this mean that match repo encrypted with fastlane version 2.220.0 will not work (decrypt) with fastlane version 2.219.0 but vice-versa should keep working?

andre-alves · 2024-04-16T16:35:47Z

Just experienced the same issue. I saved provisioning profiles using fastlane 2.220.0, and fastlane 2.219.0 is no longer able to decrypt them.

stodirascu · 2024-04-23T13:11:54Z

I have the same issue as above. If locally somebody is encrypting a profile or a certificate using 2.220, and the CI is decrypting it with 2.219, it will fail. So it's on a file-by-file basis.

This must be mentioned as a breaking change in the release notes at least.

…tlane#21663) (fastlane#21790)" This reverts commit 387691b.

lacostej added 4 commits January 10, 2024 19:51

Rewrite the encryption layer for match, keeping backwards compatibility

f6aeb5c

match: add companion script to enc

1828882

Update documentation related to encryption

4e68ebb

rubocop

c5d33cd

github-actions bot added tool: match tool: action labels Jan 10, 2024

Rename match_enc file

e5b271a

lacostej commented Jan 10, 2024

View reviewed changes

match/lib/match/encryption/encryption.rb Outdated Show resolved Hide resolved

Remove deprecation warning from OpenSSL

4332cc9

Attempt at lower casing the cipher to allow older rubies to find it

8c04014

lacostej requested review from milch and rogerluan January 10, 2024 20:02

lacostej commented Jan 10, 2024

View reviewed changes

fastlane/lib/fastlane/actions/docs/sync_code_signing.md Outdated Show resolved Hide resolved

lacostej commented Jan 10, 2024

View reviewed changes

milch reviewed Jan 10, 2024

View reviewed changes

lacostej added 4 commits January 11, 2024 07:23

address code review

e07a6f2

Adjust CLI usage and arg checks

7170033

more doc cleanups

91f0b00

rubocop

133d090

lacostej commented Jan 11, 2024

View reviewed changes

lacostej requested a review from milch January 12, 2024 05:34

rogerluan requested changes Jan 14, 2024

View reviewed changes

rogerluan changed the title ~~[match] improve encryption internals, solves flaky test (#21663)~~ [match] improve encryption internals, solving flaky test (#21663) Jan 14, 2024

lacostej and others added 6 commits January 15, 2024 21:17

Update match/lib/match/encryption/encryption.rb

471c3a0

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

Update match/lib/match/encryption/encryption.rb

cb496ec

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

Update bin/match_file

850a472

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

Update match/lib/match/encryption/encryption.rb

0d864ca

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

Update match/lib/match/encryption/encryption.rb

0394f34

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

Monkey patch match_file to support control-c :)

035cbd1

lacostej added 5 commits January 15, 2024 22:11

Fix typo

10405ee

Use keyword parameters in main entry point for encryption

237adc0

Rubocop

56578d8

ctrl-c support requires ruby 2.7.1 apparently, so aborting instead

7ed2e06

encryption: keyword parameters all the way, also add the test that go…

4c6f360

…t lost

lacostej commented Jan 16, 2024

View reviewed changes

bin/match_file Show resolved Hide resolved

lacostej requested a review from rogerluan January 16, 2024 13:45

rogerluan approved these changes Jan 18, 2024

View reviewed changes

lacostej merged commit 387691b into fastlane:master Jan 18, 2024
3 checks passed

lacostej deleted the fix/match_encryption branch January 18, 2024 05:32

joshdholtz mentioned this pull request Apr 4, 2024

Version bump to 2.220.0 #21943

Merged

kahest mentioned this pull request Apr 19, 2024

match behaves differently in v2.220.0: wrong final block length #21973

Open

4 tasks

rasberik added a commit to rasberik/fastlane that referenced this pull request May 14, 2024

Revert "[match] improve encryption internals, solving flaky test (fas…

1a9aa0e

…tlane#21663) (fastlane#21790)" This reverts commit 387691b.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[match] improve encryption internals, solving flaky test (#21663) #21790

[match] improve encryption internals, solving flaky test (#21663) #21790

lacostej commented Jan 10, 2024 •

edited

lacostej commented Jan 10, 2024

lacostej Jan 10, 2024

milch Jan 10, 2024

lacostej Jan 11, 2024

lacostej Jan 11, 2024

rogerluan Jan 14, 2024

lacostej Jan 15, 2024

rogerluan left a comment

rogerluan Jan 14, 2024

lacostej Jan 15, 2024

rogerluan Jan 18, 2024

rogerluan Jan 14, 2024

rogerluan left a comment

arc-v2 commented Apr 7, 2024

andre-alves commented Apr 16, 2024 •

edited

stodirascu commented Apr 23, 2024

		match_file encrypt "<fileYouWantToEncryptPath>" ["<encryptedFilePath>]"

		match_file decrypt "<fileYouWantToDecryptPath>" ["<decryptedFilePath>]"

[match] improve encryption internals, solving flaky test (#21663) #21790

[match] improve encryption internals, solving flaky test (#21663) #21790

Conversation

lacostej commented Jan 10, 2024 • edited

Checklist

Motivation and Context

Description

Testing Steps

lacostej commented Jan 10, 2024

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

rogerluan 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

rogerluan left a comment

Choose a reason for hiding this comment

arc-v2 commented Apr 7, 2024

andre-alves commented Apr 16, 2024 • edited

stodirascu commented Apr 23, 2024

lacostej commented Jan 10, 2024 •

edited

andre-alves commented Apr 16, 2024 •

edited