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

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

Merged
merged 22 commits into from
Jan 18, 2024

Conversation

lacostej
Copy link
Collaborator

@lacostej lacostej commented Jan 10, 2024

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

@lacostej
Copy link
Collaborator Author

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'

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

def keyivgen(cipher, password, salt, hash_algorithm)
cipher.pkcs5_keyivgen(password, salt, 1, hash_algorithm)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
bin/match_enc Outdated Show resolved Hide resolved
fastlane/lib/fastlane/actions/docs/sync_code_signing.md Outdated Show resolved Hide resolved
match/lib/match/encryption/encryption.rb Outdated Show resolved Hide resolved
# 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
match/lib/match/encryption/encryption.rb Outdated Show resolved Hide resolved
match/lib/match/encryption/encryption.rb Outdated Show resolved Hide resolved
match/lib/match/encryption/encryption.rb Show resolved Hide resolved

# by default, encrypt or decrypt in place
class MatchFileEncryption
def encrypt(file_path, password, output_path = nil)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

I'm all for keyword parameters whenever we can 🙏

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@lacostej lacostej requested a review from milch January 12, 2024 05:34
Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

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 👌

Comment on lines 515 to 517
match_file encrypt "<fileYouWantToEncryptPath>" ["<encryptedFilePath>]"

match_file decrypt "<fileYouWantToDecryptPath>" ["<decryptedFilePath>]"
Copy link
Member

Choose a reason for hiding this comment

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

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>"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

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

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

Choose a reason for hiding this comment

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

I'm all for keyword parameters whenever we can 🙏

match/lib/match/encryption/encryption.rb Outdated Show resolved Hide resolved
bin/match_file Outdated Show resolved Hide resolved
bin/match_file Show resolved Hide resolved
@rogerluan 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
Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
@lacostej lacostej requested a review from rogerluan January 16, 2024 13:45
Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

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

@lacostej lacostej merged commit 387691b into fastlane:master Jan 18, 2024
3 checks passed
@lacostej lacostej deleted the fix/match_encryption branch January 18, 2024 05:32
@arc-v2
Copy link
Contributor

arc-v2 commented Apr 7, 2024

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
Copy link

andre-alves commented Apr 16, 2024

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
Copy link

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.

rasberik added a commit to rasberik/fastlane that referenced this pull request May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

match encryption is outdated and doesn't guarantee it detects invalid passwords. Causes flaky test.
6 participants