#18 Add ECv2 support for google pay tokens #20
Conversation
Co-authored-by: Miriam Lauter <[email protected]> Co-authored-by: Spencer Alan <[email protected]>
Co-authored-by: Miriam Lauter <[email protected]> Co-authored-by: Spencer Alan <[email protected]>
There was a problem hiding this comment.
This looks good, thanks for your contribution! I haven't read up on the ECv2 scheme (ECv1 still works so we didn't need an upgrade) so my small comments are mostly on code style.
I'm also not able to merge or release this so I encourage you to reach out to Spreedly for this :)
lib/r2d2/util.rb
Outdated
| def decrypt_message(encrypted_data, symmetric_key, cipher_key_length_bits = 128) | ||
| raise ArgumentError, "Invalid cipher_key_length #{cipher_key_length_bits} must be 128 or 256" unless [128, 256].include?(cipher_key_length_bits) | ||
|
|
||
| decipher = cipher_key_length_bits == 256 ? OpenSSL::Cipher::AES256.new(:CTR) : OpenSSL::Cipher::AES128.new(:CTR) |
There was a problem hiding this comment.
The following reads easier and also doesn't use deprecated constants:
| decipher = cipher_key_length_bits == 256 ? OpenSSL::Cipher::AES256.new(:CTR) : OpenSSL::Cipher::AES128.new(:CTR) | |
| decipher = OpenSSL::Cipher.new("aes-#{cipher_key_length_bits}-ctr") |
lib/r2d2/util.rb
Outdated
| @@ -83,8 +86,8 @@ def secure_compare(a, b) | |||
| end | |||
|
|
|||
| if defined?(OpenSSL::KDF) && OpenSSL::KDF.respond_to?(:hkdf) | |||
| def hkdf(key_material, info) | |||
| OpenSSL::KDF.hkdf(key_material, salt: 0.chr * 32, info: info, length: 32, hash: 'sha256') | |||
| def hkdf(key_material, info, length = 32) | |||
There was a problem hiding this comment.
Similar question on the need for length to have a default value need here (and below for the alternative implementation.
lib/r2d2/google_pay_token.rb
Outdated
| def sender_id | ||
| 'Google' | ||
| end |
There was a problem hiding this comment.
This looks like it should be a constant and all the other 'Google' strings replaced to use it.
lib/r2d2/google_pay_token.rb
Outdated
| end | ||
|
|
||
| def intermediate_key_expired? | ||
| cur_millis = (Time.now.to_f * 1000).floor |
There was a problem hiding this comment.
This is being used twice in the class, could be extracted to a method.
lib/r2d2/google_pay_token.rb
Outdated
|
|
||
| def valid_key_signatures?(signing_keys, signatures, signed) | ||
| signing_keys.product(signatures).any? do |key, sig| | ||
| key.verify(OpenSSL::Digest::SHA256.new, Base64.strict_decode64(sig), signed) |
There was a problem hiding this comment.
Don't use deprecated constants:
| key.verify(OpenSSL::Digest::SHA256.new, Base64.strict_decode64(sig), signed) | |
| key.verify(OpenSSL::Digest.new("SHA256"), Base64.strict_decode64(sig), signed) |
| @@ -44,8 +45,10 @@ def verify_mac(mac_key, encrypted_message, tag) | |||
| raise TagVerificationError unless secure_compare(mac, Base64.decode64(tag)) | |||
| end | |||
|
|
|||
| def decrypt_message(encrypted_data, symmetric_key) | |||
| decipher = OpenSSL::Cipher::AES128.new(:CTR) | |||
| def decrypt_message(encrypted_data, symmetric_key, cipher_key_length_bits = 128) | |||
There was a problem hiding this comment.
Should cipher_key_length_bits have a default value, since we always calculate it on line 30?
| @@ -29,12 +29,13 @@ def generate_shared_secret(private_key, ephemeral_public_key) | |||
| private_key.dh_compute_key(point) | |||
| end | |||
|
|
|||
| def derive_hkdf_keys(ephemeral_public_key, shared_secret, info) | |||
| def derive_hkdf_keys(ephemeral_public_key, shared_secret, info, key_length_bytes = 16) | |||
There was a problem hiding this comment.
Should key_length_bytes have a default value?
There was a problem hiding this comment.
the function is also used by android_pay_token which we didn't want to touch, and 16 bytes was an original assumption of the function
Thank you so much for the feedback @bdewater, will make some updates and try and reach Spreedly! |
Adds ECv2 support according to https://developers.google.com/pay/api/android/guides/resources/payment-data-cryptography
Testing
We've added tests for google_pay ec_v2.
bundle exec rakepasses