ECv2 support #1
ECv2 support #1
Conversation
lib/aliquot-pay.rb
Outdated
| def self.generate_signature(*args) | ||
| args.map do |s| | ||
| four_byte_length(s) + s | ||
| end.join('') |
There was a problem hiding this comment.
Space as separator is default.
There was a problem hiding this comment.
Empty string as separator is default 🙂
There was a problem hiding this comment.
I'm sorry... I thought "empty string" but wrote "space"...
lib/aliquot-pay.rb
Outdated
| def self.generate_token_ecv2(payment, signing_key, intermediate_key, recipient, | ||
| signed_message: nil, expire_time: "#{Time.now.to_i + 3600}000") | ||
| cipher = OpenSSL::Cipher::AES256.new(:CTR) | ||
| signed_message ||= JSON.unparse(encrypt(JSON.unparse(payment), recipient, cipher)) |
There was a problem hiding this comment.
Can you explain the two instances of JSON unparsing here? I am a bit confused 🤔
There was a problem hiding this comment.
The inner payment is #to_json'ed (JSON.unparse'd) before it's encrypted ...
There was a problem hiding this comment.
Exactly. The payment is added as serialized JSON, this is done because it is what is verified by tag, a MAC.
This results in a signed message, which is also a serialized JSON string in the final token, since it is signed by Google.
lib/aliquot-pay/util.rb
Outdated
| input_keying_material = ephemeral_public_key + shared_secret | ||
| if OpenSSL.const_defined?(:KDF) && OpenSSL::KDF.respond_to?(:hkdf) | ||
| h = OpenSSL::Digest::SHA256.new | ||
| hbytes = OpenSSL::KDF.hkdf(input_keying_material, hash: h, salt: '', length: 32, info: info) | ||
| hbytes = OpenSSL::KDF.hkdf(input_keying_material, hash: h, salt: '', length: length * 2, info: info) |
There was a problem hiding this comment.
So 64 is correct and 32 was incorrect? Did it work before?
There was a problem hiding this comment.
Both 64 and 32 are correct.
For ECv1 you need to generate 32 bytes of key material, 128 bits for AES key and 128 bits for MAC key.
For ECv2you need 64 bytes, 256 bits for each.
There was a problem hiding this comment.
Since ECv2 has superseded ECv1 the required 64 bytes of keys is now default.
There was a problem hiding this comment.
What do you mean ct?
Yes, it worked before with ECv1.
lib/aliquot-pay/util.rb
Outdated
| aes_key: hbytes[0..15], | ||
| mac_key: hbytes[16..32], | ||
| aes_key: hbytes[0..length - 1], | ||
| mac_key: hbytes[length..2 * length], |
There was a problem hiding this comment.
Not length..(2 * length) - 1 ? I.e. 32..63.
There was a problem hiding this comment.
Alternatively, length..-1
There was a problem hiding this comment.
Yes, it is. luckily it still performs as it should. Now I'm considering it, I prefer.
aes_key: hbytes[0, length],
mac_key: hbytes[length, length],
lib/aliquot-pay.rb
Outdated
| recipient_id = DEFAULTS[:merchant_id], | ||
| sender_id = DEFAULTS[:info], | ||
| protocol_version = 'ECv1' | ||
| recipient_id: "merchant:#{DEFAULTS[:merchant_id]}", |
There was a problem hiding this comment.
Uhm, but if the recipient_id does always start with "merchant:" then ... couldn't we "hide" this "deeper" so the user calling .signature_string will not have to prepend the merchant ID with "merchant:"? Perhaps it's nicer to do it like you do — for debugging and spec'ing purposes?
There was a problem hiding this comment.
Hmm. I see no benefit in doing it like I do it. As in, we don't use it right now.
I agree that it should be merchant_id as a parameter rather than recipient_id.
There was a problem hiding this comment.
If convenient for debugging/spec'ing, you could allow both 🙂
lib/aliquot-pay/util.rb
Outdated
| @@ -11,18 +11,18 @@ def self.generate_shared_secret(private_key, public_key) | |||
| private_key.dh_compute_key(public_key) | |||
| end | |||
|
|
|||
| def self.derive_keys(ephemeral_public_key, shared_secret, info) | |||
| def self.derive_keys(ephemeral_public_key, shared_secret, info, length: 32) | |||
There was a problem hiding this comment.
Thought: What about supplying the "EC version" rather than length? I think that could perhaps be more user friendly?
There was a problem hiding this comment.
Hmm. Sure, that makes sense. 👍
|
I sort of feel like this library is badly written for generating tokens. |
- `JSON.unparse` => `#to_json` - Derive cipher from protocol_version instead of passing cipher object - Fix indexing of MAC/AES keys
|
@ct-clearhaus @mt-clearhaus Any more comments? |
No description provided.