Full RFC6238 Compatibility #207
Conversation
|
This looks good to me @ericmann! Could we include some tests for this since you already have them in the upstream repo. |
|
Good catch on the |
ericmann
force-pushed
the
totp-parity
branch
2 times, most recently
from
66a80c0 to
9c7d057
Compare
|
@kasparsd Feedback addressed, and I pulled in the reference tests from the RFC spec. Kind of frustrating to try to work around |
|
Access |
|
@kasparsd any thoughts on a review of this PR and whether it's ready for consideration in v0.8.0? |
|
Looks like Travis is no longer running the unit tests for this repo so I've created #405 to address it. Solving that first will allow us to ensure this change hasn't introduced a regression. |
|
Per yesterday's bug scrub, we're punting this to a future release to allow for narrow focus on the U2F deprecation work in 0.8.0. |
New tests were added for `rest_delete_totp` in WordPress#504 which cover the same functionality. `generate_qr_code_url` was also refactored there, but a test wasn't included.
The internal function accepts a param for flexibility, but currently there's no use case for letting the user choose how many they want.
* Add a rate limit between two-factor login attempts. * Add a warning upon login that the user previously failed to complete the two-factor prompt. Co-authored-by: Ian Dunn <[email protected]>
TOTP: Prevent re-use of TOTP tokens, and prevent a previously-generated token being valid if a newer token is used to login. Co-authored-by: Ian Dunn <[email protected]>
Previously the missing token wasn't tested for, and the FUT was passed an invalid data type for the user.
…Press#519) * Include whitespace between the leading text and the code input. * Switch from using <br> to using CSS.
…of WordPress, update other dependencies. (WordPress#524)
* Add unit tests for Two_Factor_Provider::get_code(). * Add a test that validates an array of characters results in the correct character output * Explicitly check the length of the generated codes, and add a test for a string input of valid characters. --------- Co-authored-by: Ian Dunn <[email protected]>
This makes it convenient to check coverage locally, instead of having to go to Coveralls.
Update screenshots to match the current UI
Improve discoverability
Delete user meta on plugin uninstall
…ly breaking way for integrations
Collect all warnings into same place
Bumps [symfony/process](https://github.com/symfony/process) from 5.4.40 to 5.4.46. - [Release notes](https://github.com/symfony/process/releases) - [Changelog](https://github.com/symfony/process/blob/7.1/CHANGELOG.md) - [Commits](symfony/process@v5.4.40...v5.4.46) --- updated-dependencies: - dependency-name: symfony/process dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>
…ymfony/process-5.4.46 Bump symfony/process from 5.4.40 to 5.4.46
Focus in code input when totp is checked
|
@kasparsd seems like resolving merge conflicts here, checking that tests pass, could get this into the 0.11.0 release perhaps? |
The full TOTP specification supports not only SHA1, but also SHA256 and SHA512. Up til now, the TOTP provider in this plugin (and most other PHP implementations) claims to support a specified hash type, but doesn't actually work with anything other than SHA1. This is due to key lengths and hash lengths being _different_ for the three hash variants. This change introcudes support for both SHA256 and SHA512, porting the implementation directly from https://github.com/ericmann/totp. See https://tools.ietf.org/html/rfc6238#section-1.2 for more information on the `MAY USE` notation for SHA256 and SHA512. See https://tools.ietf.org/html/rfc6238#appendix-A for a fully compliant reference implementation in Java. See https://tools.ietf.org/html/rfc6238#appendix-B for test vectors showing TOTPs generated for specific time values and the three hash variants. See https://github.com/ericmann/totp/blob/master/test/phpunit/ReferenceTest.php for example unit tests verifying this particular implementation before it was ported to the plugin.
According to the linter: > Method name "Two_Factor_Totp::__set_time" is discouraged; PHP has reserved all method names with a double underscore prefix for future use. Rename the function to make the linter happy.
|
I have no idea why GH thinks I'm asking to merge so much - it's literally just the last 3 commits and they're rebased atop |
|
@ericmann maybe an oddity with you originally pushing from a fork? perhaps a new PR that cherrypicks those last 3 commits will show "better" on GitHub? |
|
Closing this to fix the GH display oddity. |
The full TOTP specification supports not only SHA1, but also SHA256 and SHA512. Up til now, the TOTP provider in this plugin (and most other PHP implementations) claims to support a specified hash type, but doesn't actually work with anything other than SHA1.
This is due to key lengths and hash lengths being different for the three hash variants.
This change introcudes support for both SHA256 and SHA512, porting the implementation directly from https://github.com/ericmann/totp.
See https://tools.ietf.org/html/rfc6238#section-1.2 for more information on the
MAY USEnotation for SHA256 and SHA512.See https://tools.ietf.org/html/rfc6238#appendix-A for a fully compliant reference implementation in Java.
See https://tools.ietf.org/html/rfc6238#appendix-B for test vectors showing TOTPs generated for specific time values and the three hash variants.
See https://github.com/ericmann/totp/blob/master/test/phpunit/ReferenceTest.php for example unit tests verifying this particular implementation before it was ported to the plugin.