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

sha256/sha512 secret buffer size #77

Merged

Conversation

mslosarek
Copy link
Contributor

The secret buffer needs to be repeated to the proper size for the one-time passcode to work correctly. Resolves issue #76

@coveralls
Copy link

coveralls commented Feb 9, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 903150d on mslosarek:sha256_sha512_secret_buffer_padding into 2adce34 on speakeasyjs:master.

Copy link
Collaborator

@mikepb mikepb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the pull request! Please see review comments for feedback.

index.js Outdated
secret_buffer_size = 32; // 32 bytes
} else if (algorithm === 'sha512') {
secret_buffer_size = 64; // 64 bytes
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case should check for sha1. The default case should warn when an unsupported algorithm is given; the crypto module accepts algorithms not officially supported by the spec. Throwing an exception would be safer, but would restrict usage. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sha1 is not explicit rather than implied, also added a test for not throwing an error for unofficial algorithms

index.js Outdated
@@ -40,6 +40,16 @@ exports.digest = function digest (options) {
if (encoding === 'base32') { secret = base32.decode(secret); }
secret = new Buffer(secret, encoding);
}
// pad the buffer to the correct size be repeating the secret
var secret_buffer_size;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initialize this variable to 0 or similar for 'no padding'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now handle a no padding state

index.js Outdated
} else {
secret_buffer_size = 20; // 20 bytes
}
secret = new Buffer(Array(secret_buffer_size).join(secret.toString('hex')).substr(0, secret_buffer_size * 2), 'hex');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For efficiency, when padding is not needed, this code should not run.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This step is only performed if the secret_buffer_size is now different from the secret.length

@coveralls
Copy link

coveralls commented Feb 9, 2017

Coverage Status

Coverage decreased (-0.5%) to 99.462% when pulling 5a706e5 on mslosarek:sha256_sha512_secret_buffer_padding into 2adce34 on speakeasyjs:master.

@coveralls
Copy link

coveralls commented Feb 9, 2017

Coverage Status

Coverage decreased (-0.5%) to 99.462% when pulling 4bea3d8 on mslosarek:sha256_sha512_secret_buffer_padding into 2adce34 on speakeasyjs:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 99.462% when pulling 4bea3d8 on mslosarek:sha256_sha512_secret_buffer_padding into 2adce34 on speakeasyjs:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 99.462% when pulling 4bea3d8 on mslosarek:sha256_sha512_secret_buffer_padding into 2adce34 on speakeasyjs:master.

@coveralls
Copy link

coveralls commented Feb 9, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling d16e486 on mslosarek:sha256_sha512_secret_buffer_padding into 2adce34 on speakeasyjs:master.

@coveralls
Copy link

coveralls commented Feb 9, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling d02a29a on mslosarek:sha256_sha512_secret_buffer_padding into 2adce34 on speakeasyjs:master.

@mikepb mikepb merged commit f4b54e1 into speakeasyjs:master Feb 10, 2017
@mikepb
Copy link
Collaborator

mikepb commented Feb 10, 2017

Super, thank you for the quick update!

@mikepb mikepb added this to Bugs in 3.0 Feb 28, 2017
@mikepb mikepb moved this from Bugs to Pull Requests in 3.0 Feb 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
3.0
Pull Requests
Development

Successfully merging this pull request may close these issues.

None yet

3 participants