samdjstevens / java-totp

A java library for implementing Time-based One Time Passwords for Multi-Factor Authentication.
MIT License
422 stars 103 forks source link

Issue on code verification #16

Closed benjaminpech closed 4 years ago

benjaminpech commented 4 years ago

Hi,

I am trying to implement your library for very simple use. Only code generation from secret and then checking it.

But this verification always fail.

I use the SystemTimeProvider and all defaults parameters.

Could you help me with it ?

Cordially.

samdjstevens commented 4 years ago

Hey,

Thanks for trying out the library!

When generating the QR code, what hashing algorithm, time period, and digits are you using? (If you can provide code sample that would be good)

Can you also provide the code you're using to verify the codes?

benjaminpech commented 4 years ago

Precisely,

I dont want to use QR code, assuming the user has the secret and the code.

Here is my code : SystemTimeProvider timeProvider = new SystemTimeProvider();

    SecretGenerator generator = new DefaultSecretGenerator();
    String secret = generator.generate();

    CodeGenerator codeGenerator = new DefaultCodeGenerator(HashingAlgorithm.SHA512);
    String code = codeGenerator.generate(secret, timeProvider.getTime());

    Thread.sleep(2000);

    CodeVerifier codeVerifier = new DefaultCodeVerifier(codeGenerator, timeProvider);      

    boolean valid = codeVerifier.isValidCode(secret, code);

    System.out.println(code);
    System.out.println(secret);        
    System.out.println(valid);
samdjstevens commented 4 years ago

Hey,

I see what's going on here - in your specific example I assume you are "bypassing" the user-side MFA app for the purpopse of a test by calling the codeGenerator.generate() method directly in code, but this is actually being called in an incorrect way.

The second parameter to this method isn't the current UNIX time (as is being passed in the above snippet), but the time counter, which is the current UNIX time divided by the time period (see code here: https://github.com/samdjstevens/java-totp/blob/master/src/main/java/dev/samstevens/totp/code/DefaultCodeVerifier.java#L30)

So if you changed the code generation bit in your test above to the following:

String code = codeGenerator.generate(secret, Math.floorDiv(timeProvider.getTime(), 30));

Then the generated code would be valid.

benjaminpech commented 4 years ago

Thank you for your help, this works like a charm.

slimeball1234 commented 3 years ago

Hello,

I generating a code exactly like benjaminpech and I then have the problem that the code is not reliably validated after a certain amount of time passes. My code is

CodeGenerator codeGenerator = new DefaultCodeGenerator(); TimeProvider timeProvider = new SystemTimeProvider(); DefaultCodeVerifier verifier = new DefaultCodeVerifier(codeGenerator, timeProvider); SecretGenerator secretGenerator = new DefaultSecretGenerator(); String secret = secretGenerator.generate(); int codeTimePeriod = 30;

String code = "";

try { code = codeGenerator.generate(secret, Math.floorDiv(timeProvider.getTime(), codeTimePeriod)); } catch (CodeGenerationException ex) { LOGGER.error("Error when trying to generate code", ex); }

try { Thread.sleep(23000); } catch (InterruptedException ex) { Logger.getLogger(Main.class.getName()).log(Level.SEVERE, null, ex); }

boolean isValid = verifier.isValidCode(secret, code);

The isValid value is sometimes true, sometimes false. I would expect that if the sleep is less then 30000 that the code is always valid, sometimes it is also not valid with 23000 milliseconds sleep. What am I doing wrong?

Kind regards

Stexxen commented 3 years ago

The Floordiv means that the code generated may have less than 23 seconds of validity. Waiting 23 seconds means that on average 23/30 attempts would fail to validate.

If you allow validation of the next/previous then it would always work.

slimeball1234 commented 3 years ago

The Floordiv means that the code generated may have less than 23 seconds of validity. Waiting 23 seconds means that on average 23/30 attempts would fail to validate.

If you allow validation of the next/previous then it would always work.

Ok, thank you for the reply, then that is what I am not understanding. If the time period is 30s, I generate the code, is the code not valid for the next 30 seconds? I.E should it not always be valid after the Thread.sleep(23000) , as 23 seconds of waiting is less then 30?

To make it more specific: I generate a code at 10:00:00 with a CodeGenerator that has a time window of 30s - should not any code generated between 10:00:00 and 10:00:30 be valid until 10:00:30? And should it not become invalid at 10:00:31?

jarretttaylor commented 3 years ago

This explanation is an oversimplification, but it should help you understand the concept. Let's assume that generated codes are good for 30 seconds. The codes are locked to predefined intervals (e.g. 10:00:00.000 to 10:00:29.999, 10;00:30.000 to 10:00:59.999, 10:01:00.000 to 10:01:29.999). The actual intervals are defined by the algorithm that generates the codes. If you generate a code at 10:00:51.783, that code will only be good for 8.216 seconds (i.e. the remainder of the predefined interval). You can see this in practice when you open an authenticator app and look at a generated code. The code will be in the process of expiring. Unless you time it perfectly, it will have less than 30 seconds remaining before a new code is generated.

Stexxen commented 3 years ago

To make it more specific: I generate a code at 10:00:00 with a CodeGenerator that has a time window of 30s - should not any code generated between 10:00:00 and 10:00:30 be valid until 10:00:30? And should it not become invalid at 10:00:31?

No thats not correct, the time periods of 30 seconds are fixed going back to the Epoch Date (midnight 1st Jan 1970) The period you generate the code in is already predefined. so you generate at 10:00:00, but the validity period could be 09:59:50 to 10:00:20, so it will only be valid for 20 seconds.

slimeball1234 commented 3 years ago

To make it more specific: I generate a code at 10:00:00 with a CodeGenerator that has a time window of 30s - should not any code generated between 10:00:00 and 10:00:30 be valid until 10:00:30? And should it not become invalid at 10:00:31?

No thats not correct, the time periods of 30 seconds are fixed going back to the Epoch Date (midnight 1st Jan 1970) The period you generate the code in is already predefined. so you generate at 10:00:00, but the validity period could be 09:59:50 to 10:00:20, so it will only be valid for 20 seconds.

Ok, then I again do not understand. If the periods I am working with are 30s large how could a period start at 09:59:50 ?

Stexxen commented 3 years ago

To make it more specific: I generate a code at 10:00:00 with a CodeGenerator that has a time window of 30s - should not any code generated between 10:00:00 and 10:00:30 be valid until 10:00:30? And should it not become invalid at 10:00:31?

No thats not correct, the time periods of 30 seconds are fixed going back to the Epoch Date (midnight 1st Jan 1970) The period you generate the code in is already predefined. so you generate at 10:00:00, but the validity period could be 09:59:50 to 10:00:20, so it will only be valid for 20 seconds.

Ok, then I again do not understand. If the periods I am working with are 30s large how could a period start at 09:59:50 ?

Sorry bad example, for 30s you are correct it will always begin at 10:00:00 and finish at 10:00:30 regardless of when you generate the code.

slimeball1234 commented 3 years ago

To make it more specific: I generate a code at 10:00:00 with a CodeGenerator that has a time window of 30s - should not any code generated between 10:00:00 and 10:00:30 be valid until 10:00:30? And should it not become invalid at 10:00:31?

No thats not correct, the time periods of 30 seconds are fixed going back to the Epoch Date (midnight 1st Jan 1970) The period you generate the code in is already predefined. so you generate at 10:00:00, but the validity period could be 09:59:50 to 10:00:20, so it will only be valid for 20 seconds.

Ok, then I again do not understand. If the periods I am working with are 30s large how could a period start at 09:59:50 ?

Sorry bad example, for 30s you are correct it will always begin at 10:00:00 and finish at 10:00:30 regardless of when you generate the code.

Ok, thank you. So back to the original question - if I generate a code at 10:00:15 with a time window of 30s, the code is valid only until 10:00:30?

Stexxen commented 3 years ago

Ok, thank you. So back to the original question - if I generate a code at 10:00:15 with a time window of 30s, the code is valid only until 10:00:30?

yes