Faster hmac sha1 token generation #13
Conversation
|
Hi @Lixxia, is this something you would be interested in merging? Is there anything else you would like me to change / explain as part of this pull request? |
|
Hiya, sorry been on vacation past few weeks. This looks great! I'll take a look this weekend and run some tests. Maybe you could give me a quick overview of which situations you tested? |
|
I tested starting with a couple of existing codes from AWS (which seems to have longer than usual secrets) and one shorter code, and those got picked up and turned into the array buffer format. I also removed them and added the same codes back from the settings page to test that saving new codes is okay. I've been using this version of the app myself for a couple of weeks and the codes have been accepted, so at least for AWS the codes are still being generated correctly. |
|
|
||
| let hex = parsehex(bits); | ||
|
|
||
| if (hex.length % 2 !== 0) { |
There was a problem hiding this comment.
You'll need to add support for the hexpad function/check or an alternative in your new base32ToUint8Array function. With your current proposed changes 26 or other odd length tokens no longer work. This issue (#3) was previously solved with the addition of this function.
There was a problem hiding this comment.
Whoops, sorry. I thought I had covered that, since I had been testing with Amazon tokens, but tokens with 26 characters are definitely broken with my changes. I'll get that sorted and update the branch.
There was a problem hiding this comment.
Turns out just removing the check for leftover bits allows base32 strings with weird lengths to work. Tested with the 26 char test string from app/index.js and some other random-length base32 strings and it seems to decode properly now.
app/tokens.js
Outdated
| } | ||
|
|
||
| // Convert any existing string/base32-encoded tokens to a Uint8Array, and wrap raw ArrayBuffer tokens | ||
| function convertTokensToUint8Array(data) { |
There was a problem hiding this comment.
Given that this isn't a utility function and you're only operating on the this.file.data object I'd prefer that this function be integrated into the AuthToken class.
|
Overall looks good, very solid approach and definitely reduces speeds on load/token regeneration. I've added a few comments in-line regarding a reintroduced bug and some code style preferences. Let me know if you have any questions! |
… boost Roughly doubles the performance of the HMAC-SHA1 part of TOTP generation.
This avoids spending ~50-100ms converting from base32 to the bytes used for the TOTP HMAC-SHA1 every time a code is generated. Existing stored tokens will be converted when the app is first launched, and new tokens will be converted when they are added in settings.
|
Re-tested by removing all codes, reverting to Also tested removing and re-adding the same codes as above, which seemed to work fine. |
|
Hey, not sure if you're still looking at this or not. I did a fresh clone and install of your branch and seems there's still a few issues. There's a leftover reference to the |
|
It has been working fine for me for months after the latest push, not sure what would be causing reference errors. The nodeWrap/p function should never be called in Fitbit because it's guarded by a check for node.js, which should not be true for Fitbit's js interpreter... Is something broken or are there logs somewhere that show the errors? I'll take a closer look later this week when I have time to debug. |
The simulator's companion app appears to be written in node.js, or at least triggers js-sha1's node.js detection.
nhinds
force-pushed
the
faster-hmac-sha1
branch
from
3832052 to
a32bb1d
Compare
|
I found what you were referring to. It worked fine on my watch and phone, but I got this error when running under the simulator: Looks like the simulator's companion is running under node.js, or at least something that looks like node.js to the js-sha1 library. I've added the settings which disable node.js/commonjs support in js-sha1 and it seems to work fine in the simulator now. Are you able to check whether it is working for you as well? |
|
Everything looks good, thanks so much for your work! |
Switch from the jsSHA library to js-sha1, which is roughly twice as fast on my watch.
js-sha1doesn't have HMAC support built in, so that's now a custom implementation incommon/hmac-sha1.js. UsingUint8Arrayeverywhere instead of encoding and decoding hex/base32 strings gives a significant performance boost while computing tokens.To avoid decoding the tokens from base32 each time the app launches, this PR changes the stored CBOR file from storing tokens as base32-encoded strings to storing the decoded byte arrays. Existing stored tokens will be converted when the app is first launched, and new tokens will be converted when they are added in settings. It would be possible to re-encode the byte arrays back to base32 if you want to switch out the libraries for OTP/HMAC/SHA1/etc. in the future.
For me, when I have 3 codes the current app takes around 4s to initially launch, and 2.5s to regenerate codes (tested by turning the screen off and on). This branch brings that to around 2.5s for the initial launch and less than 1s to regenerate codes.