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
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
f6aeb5c
Rewrite the encryption layer for match, keeping backwards compatibility
lacostej Jan 10, 2024
1828882
match: add companion script to enc
lacostej Jan 10, 2024
4e68ebb
Update documentation related to encryption
lacostej Jan 10, 2024
c5d33cd
rubocop
lacostej Jan 10, 2024
e5b271a
Rename match_enc file
lacostej Jan 10, 2024
4332cc9
Remove deprecation warning from OpenSSL
lacostej Jan 10, 2024
8c04014
Attempt at lower casing the cipher to allow older rubies to find it
lacostej Jan 10, 2024
e07a6f2
address code review
lacostej Jan 11, 2024
7170033
Adjust CLI usage and arg checks
lacostej Jan 11, 2024
91f0b00
more doc cleanups
lacostej Jan 11, 2024
133d090
rubocop
lacostej Jan 11, 2024
471c3a0
Update match/lib/match/encryption/encryption.rb
lacostej Jan 15, 2024
cb496ec
Update match/lib/match/encryption/encryption.rb
lacostej Jan 15, 2024
850a472
Update bin/match_file
lacostej Jan 15, 2024
0d864ca
Update match/lib/match/encryption/encryption.rb
lacostej Jan 15, 2024
0394f34
Update match/lib/match/encryption/encryption.rb
lacostej Jan 15, 2024
035cbd1
Monkey patch match_file to support control-c :)
lacostej Jan 15, 2024
10405ee
Fix typo
lacostej Jan 15, 2024
237adc0
Use keyword parameters in main entry point for encryption
lacostej Jan 15, 2024
56578d8
Rubocop
lacostej Jan 16, 2024
7ed2e06
ctrl-c support requires ruby 2.7.1 apparently, so aborting instead
lacostej Jan 16, 2024
4c6f360
encryption: keyword parameters all the way, also add the test that go…
lacostej Jan 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 25 additions & 0 deletions bin/match_enc
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#!/usr/bin/env ruby
lacostej marked this conversation as resolved.
Show resolved Hide resolved
require 'match'

# CLI to encrypt/decrypt files using fastlane match encryption layer

def usage
puts("USAGE: [encrypt|decrypt] input_path password [output_path]")
lacostej marked this conversation as resolved.
Show resolved Hide resolved
exit(-1)
end

if ARGV.count < 3 || ARGV.count > 4
usage
end

method_name = ARGV.shift
unless ['encrypt', 'decrypt'].include?(method_name)
usage
end

begin
Match::Encryption::MatchFileEncryption.new.send(method_name, *ARGV)
rescue => e
puts("ERROR #{method_name}ing. [#{e}]. Check your password")
usage
end
10 changes: 9 additions & 1 deletion fastlane/lib/fastlane/actions/docs/sync_code_signing.md
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,15 @@ Please be careful when using this option and ensure the certificates and profile

### Manual Decrypt

If you want to manually decrypt a file you can.
If you want to manually decrypt or encrypt, you can use the companion script `match_enc`:

```no-highlight
match_enc encrypt "<fileYouWantToEncryptPath>" "<password>" "<encryptedFilePath>"

match_enc decrypt "<fileYouWantToDecryptPath>" "<password>" "<decryptedFilePath>"
```

In versions of fastlane priori to 2.219.0, the encryption was compatible with openssl aes-256-cbc cipher.
lacostej marked this conversation as resolved.
Show resolved Hide resolved

```no-highlight
openssl aes-256-cbc -k "<password>" -in "<fileYouWantToDecryptPath>" -out "<decryptedFilePath>" -a -d -md [md5|sha256]
Expand Down
1 change: 1 addition & 0 deletions match/lib/match/encryption.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
require_relative 'encryption/interface'
require_relative 'encryption/openssl'
require_relative 'encryption/encryption'

module Match
module Encryption
Expand Down
148 changes: 148 additions & 0 deletions match/lib/match/encryption/encryption.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
require 'base64'
require 'openssl'
require 'securerandom'

module Match
module Encryption
# This is to keep backwards compatibility with the old fastlane version which used the local openssl installation.
# The encryption parameters in this implementation reflect the old behavior which was the most common default value in those versions.
# As for decryption, 1.0.x OpenSSL and earlier versions use MD5, 1.1.0c and newer uses SHA256, we try both before giving an error
class EncryptionV1
ALGORITHM = 'aes-256-cbc'

def encrypt(data, password, salt, hash_algorithm = "MD5")
lacostej marked this conversation as resolved.
Show resolved Hide resolved
cipher = ::OpenSSL::Cipher::Cipher.new(ALGORITHM)
cipher.encrypt

keyivgen(cipher, password, salt, hash_algorithm)

encrypted_data = cipher.update(data)
encrypted_data << cipher.final
end
def decrypt(encrypted_data, password, salt, hash_algorithm = "MD5")
lacostej marked this conversation as resolved.
Show resolved Hide resolved
cipher = ::OpenSSL::Cipher::Cipher.new(ALGORITHM)
cipher.decrypt

keyivgen(cipher, password, salt, hash_algorithm)

data = cipher.update(encrypted_data)
data << cipher.final
end

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.

end
end

# The newer encryption mechanism
# A more secure key and IV generation
# IV is randomly generated and provided unencrypted
# 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
lacostej marked this conversation as resolved.
Show resolved Hide resolved
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

ALGORITHM = 'AES-256-GCM'

def encrypt(data, password, salt)
cipher = ::OpenSSL::Cipher::Cipher.new(ALGORITHM)
cipher.encrypt

keyivgen(cipher, password, salt)

encrypted_data = cipher.update(data)
encrypted_data << cipher.final

auth_tag = cipher.auth_tag

{ encrypted_data: encrypted_data, auth_tag: auth_tag }
end

def decrypt(encrypted_data, password, salt, auth_tag)
cipher = ::OpenSSL::Cipher::Cipher.new(ALGORITHM)
cipher.decrypt

keyivgen(cipher, password, salt)

cipher.auth_tag = auth_tag

data = cipher.update(encrypted_data)
data << cipher.final
end

private

def keyivgen(cipher, password, salt)
keyIv = ::OpenSSL::KDF.pbkdf2_hmac(password, salt: salt, iterations: 10_000, length: 32 + 12 + 24, hash: "sha256")
key = keyIv[0..31]
iv = keyIv[32..43]
auth_data = keyIv[44..-1]
cipher.key = key
cipher.iv = iv
cipher.auth_data = auth_data
end
end

class MatchDataEncryption
V1_PREFIX = "Salted__"
V2_PREFIX = "2_Salted__"
lacostej marked this conversation as resolved.
Show resolved Hide resolved

def encrypt(data, password, version = 2)
salt = SecureRandom.random_bytes(8)
if version == 2
e = EncryptionV2.new
encryption = e.encrypt(data, password, salt)
encrypted_data = V2_PREFIX + salt + encryption[:auth_tag] + encryption[:encrypted_data]
else
e = EncryptionV1.new
encrypted_data = V1_PREFIX + salt + e.encrypt(data, password, salt)
lacostej marked this conversation as resolved.
Show resolved Hide resolved
end
Base64.encode64(encrypted_data)
end

# expected_salt is only used when testing
lacostej marked this conversation as resolved.
Show resolved Hide resolved
def decrypt(base64encoded_encrypted, password)
stored_data = Base64.decode64(base64encoded_encrypted)
if stored_data.start_with?(V2_PREFIX)
salt = stored_data[10..17]
auth_tag = stored_data[18..33]
data_to_decrypt = stored_data[34..-1]
e = EncryptionV2.new
e.decrypt(data_to_decrypt, password, salt, auth_tag)
else
salt = stored_data[8..15]
data_to_decrypt = stored_data[16..-1]
e = EncryptionV1.new
begin
e.decrypt(data_to_decrypt, password, salt)
rescue => _ex
# Note that we are not guaranteed to catch the decryption errors here if the password is wrong
# as there's no integrity checks.
# With a wrong password, there's a 0.4% chance it will decrypt garbage and not fail
lacostej marked this conversation as resolved.
Show resolved Hide resolved
fallback_hash_algorithm = "SHA256"
e.decrypt(data_to_decrypt, password, salt, fallback_hash_algorithm)
end
end
end
end

# by default, encrypt or decrypt in place
lacostej marked this conversation as resolved.
Show resolved Hide resolved
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?

output_path = file_path unless output_path
data_to_encrypt = File.binread(file_path)
e = MatchDataEncryption.new
data = e.encrypt(data_to_encrypt, password)
File.write(output_path, data)
end
def decrypt(file_path, password, output_path = nil)
lacostej marked this conversation as resolved.
Show resolved Hide resolved
output_path = file_path unless output_path
content = File.read(file_path)
e = MatchDataEncryption.new
decrypted_data = e.decrypt(content, password)
File.binwrite(output_path, decrypted_data)
end
end
end
end
45 changes: 7 additions & 38 deletions match/lib/match/encryption/openssl.rb
Original file line number Diff line number Diff line change
Expand Up @@ -109,54 +109,23 @@ def fetch_password!
return password
end

# We encrypt with MD5 because that was the most common default value in older fastlane versions which used the local OpenSSL installation
# A more secure key and IV generation is needed in the future
# IV should be randomly generated and provided unencrypted
# salt should be randomly generated and provided unencrypted (like in the current implementation)
# key should be generated with OpenSSL::KDF::pbkdf2_hmac with properly chosen parameters
# Short explanation about salt and IV: https://stackoverflow.com/a/1950674/6324550
def encrypt_specific_file(path: nil, password: nil)
UI.user_error!("No password supplied") if password.to_s.strip.length == 0

data_to_encrypt = File.binread(path)
salt = SecureRandom.random_bytes(8)

# The :: is important, as there is a name clash
cipher = ::OpenSSL::Cipher.new('AES-256-CBC')
cipher.encrypt
cipher.pkcs5_keyivgen(password, salt, 1, "MD5")
encrypted_data = "Salted__" + salt + cipher.update(data_to_encrypt) + cipher.final

File.write(path, Base64.encode64(encrypted_data))
e = MatchFileEncryption.new
e.encrypt(path, password)
rescue FastlaneCore::Interface::FastlaneError
raise
rescue => error
UI.error(error.to_s)
UI.crash!("Error encrypting '#{path}'")
end

# The encryption parameters in this implementations reflect the old behavior which depended on the users' local OpenSSL version
# 1.0.x OpenSSL and earlier versions use MD5, 1.1.0c and newer uses SHA256, we try both before giving an error
def decrypt_specific_file(path: nil, password: nil, hash_algorithm: "MD5")
stored_data = Base64.decode64(File.read(path))
salt = stored_data[8..15]
data_to_decrypt = stored_data[16..-1]

decipher = ::OpenSSL::Cipher.new('AES-256-CBC')
decipher.decrypt
decipher.pkcs5_keyivgen(password, salt, 1, hash_algorithm)

decrypted_data = decipher.update(data_to_decrypt) + decipher.final

File.binwrite(path, decrypted_data)
def decrypt_specific_file(path: nil, password: nil)
e = MatchFileEncryption.new
e.decrypt(path, password)
rescue => error
fallback_hash_algorithm = "SHA256"
if hash_algorithm != fallback_hash_algorithm
decrypt_specific_file(path: path, password: password, hash_algorithm: fallback_hash_algorithm)
else
UI.error(error.to_s)
UI.crash!("Error decrypting '#{path}'")
end
UI.error(error.to_s)
UI.crash!("Error decrypting '#{path}'")
end
end
end
Expand Down