Correctly generate signature. #48
Conversation
| let allZeroes = Array(repeating: UInt8(0), count: 32) | ||
| signatureBytes.writeBytes([allZeroes, rb].joined().suffix(32)) | ||
| signatureBytes.writeBytes([allZeroes, sb].joined().suffix(32)) |
There was a problem hiding this comment.
Can you explain to me why this change is necessary? Do I understand this correctly that you prepend 32 zero bytes to the actual bytes only to cut off the last 32 bytes again?!
There was a problem hiding this comment.
Did something break for you? This was needed to fix a flaky test.
There was a problem hiding this comment.
No, nothing broke. It is just our of curiosity, I don't see how sb[0..<lens] could have let to a flaky test. And how your changes are supposed to fix this flaky test?
There was a problem hiding this comment.
lens is sometimes only 31 so this makes sure its always 32 when written in.
There was a problem hiding this comment.
Ah, ok, I see, but why is that? Is the signature still correct when the number is only 31 bytes wide?
There was a problem hiding this comment.
I never saw it fail in the wild, nor was I able to make it fail(become 31) when sending push notifications, but I would imagine the signature would be wrong when its 31.
Our test, specifically checks for the length of 32.
There was a problem hiding this comment.
Also note that rb and sb are already always 32 bytes wide. So I wonder what would happen if you instead just use signatureBytes.writeBytes(rb)
There was a problem hiding this comment.
Also note that
rbandsbare already always 32 bytes wide. So I wonder what would happen if you instead just usesignatureBytes.writeBytes(rb)
Ah, wrong. I think I get it now: the return value of BN_bn2bin is the number of bytes actually written into the buffer. For small numbers this can be less than 32. The size of rb is calculated by BN_num_bits(r)+7)/8. This might also be wrong already for the same reason. So your fix is indeed correct.
There is a method BN_bn2binpad which adds the padding automatically. Maybe we should just use this instead? And also init the rb and sb arrays with a length of 32.
There was a problem hiding this comment.
On second thought: for big-endian, should the padding be in the front, or the end of the buffer?
There was a problem hiding this comment.
Im not entirely sure, im new to this particular signature creation. I can say that if you do
while swift test --sanitize=address -c release -Xswiftc -enable-testing --disable-index-store; do date; done
The old way will eventually fail tests, but with this new way we always pass, and always have the correct length.
|
I think your patch is correct, but I think even better would be a slightly different approach using the |
Unfortunately, this function is not available on all the different versions of SSL. So, I don't think it is helpful to rework this. Thanks for the explanation, and sorry that I missed this padding issue when proposing this code. |
|
@t089 one day we will be able to drop support for openssl 1.0 :) As an aside, and out of curiosity are you running this project on production? |
fixes #39