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

Do not increase failcounter for quering status TIQR token #1684

Conversation

Projects
None yet
4 participants
@basvandervlies
Copy link

commented Jun 14, 2019

The failcounter is increased if we check if the qrcode is
accepted, see:

@codecov

This comment has been minimized.

Copy link

commented Jun 14, 2019

Codecov Report

Merging #1684 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1684      +/-   ##
==========================================
+ Coverage   96.94%   96.94%   +<.01%     
==========================================
  Files         148      148              
  Lines       17857    17858       +1     
==========================================
+ Hits        17312    17313       +1     
  Misses        545      545
Impacted Files Coverage Δ
privacyidea/lib/tokens/tiqrtoken.py 98.68% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f282d7...9091a56. Read the comment docs.

@@ -440,6 +440,8 @@ def check_challenge_response(self, user=None, passw=None, options=None):
for challengeobject in challengeobject_list:
# check if we are still in time.
if challengeobject.is_valid():
# This is a valid transaction ID not need to increase fail counter
self.set_failcount(abs(self.get_failcount()) - 1)

This comment has been minimized.

Copy link
@cornelinux

cornelinux Jun 14, 2019

Member

Thanks a lot for your pull request!

It somehow feels a bit wired to decrease the failcounter. I am not sure, which side effects this might have.
I would rather not increase the failcounter in the first place, if possible.

The failcounter is increased in 3 occasions:

If the pin matches, but the otp or challenge/response does not match:
https://github.com/privacyidea/privacyidea/blob/master/privacyidea/lib/token.py#L2266

If the pin does not match, aka nothing matches:
https://github.com/privacyidea/privacyidea/blob/master/privacyidea/lib/token.py#L2293

Or as in your case: the response does not match a challenge:
https://github.com/privacyidea/privacyidea/blob/master/privacyidea/lib/token.py#L2240

If the tokenclass would know, in which case the inc_failcount occurs, than the tokenclass itself could decide in which way to increase the failcounter.
We could add a parameter to inc_failcount to pass the situation or we could add methods to the tokenclass.

What do you think?
@Mipronimo What do you think in regards to our push token? I totally forgot about the tiqr, which works rather similar but checks the answered challenge in another way.

This comment has been minimized.

Copy link
@basvandervlies

basvandervlies Jun 15, 2019

Author

I agree that it is a bit awkward to decrease the failcounter. Can the message then also be adjusted?:

  • "message": "Response did not match the challenge."

Maybe to: message: "User has not yet confirmed"

This comment has been minimized.

Copy link
@cornelinux

cornelinux Jun 17, 2019

Member

I would like to redesign this a bit as mentioned above to also bring this in sync with the push token, since TiQR and push work rather the same.
Currently we are a bit under heavy load, so please excuse, if this takes some time.

This comment has been minimized.

Copy link
@Mipronimo

Mipronimo Jun 18, 2019

Member

I mentioned it now in #1689, which should be discuss here maybe..
We could add the parameter transaction_id to the /auth endpoint.
After /auth username=user password=pass we get a transaction id.
If we then poll /auth username=user transaction_id=tID we would not increase the failcounter for the other tokens, right?

This comment has been minimized.

Copy link
@cornelinux

cornelinux Jun 19, 2019

Member

"polling" /auth with will currently always increase the fail counter.

The interesting thing however is, that the second /auth or /validate/check request as an answer to the first request in a challenge-response-flow would not actually result in a successful authentication. So "Polling" the /auth oder /validate/check endpoint will not enable brute force for tiqr or push, since the authentication occurs out of band.

Basically it happens here:
https://github.com/privacyidea/privacyidea/blob/master/privacyidea/lib/token.py#L2240
This code increases the fail counters of the tokens, that match the challenge, if none of the tokens created a valid response. This is fine.
But not for tiqr and push. So it should be something like this:

A)

        if not res:
            # We did not find any successful response, so we need to increase the
            # failcounters
            for token_obj in challenge_response_token_list:
                if token_obj.get_type().lower() not in ["tiqr", "push"]:
                    token_obj.inc_failcount()

But this of course is bad style, since we should avoid having tokenspecific stuff in the lib/token.py.
So either we add a flag to the tokenclass like is_out_of_band_response_token:
B)

            for token_obj in challenge_response_token_list:
                if not token_obj.is_out_of_band_response_token:
                    token_obj.inc_failcount()

or we add a new method, that is increases the token and is only called here.
C)

            for token_obj in challenge_response_token_list:
                    token_obj.inc_failcount_in_case_of_response()

But I think it is a bit strange to have a method, that is only called in one place and either increases or does nothing.

So I think the token-flag B) might be a way to go...

@basvandervlies

This comment has been minimized.

Copy link
Author

commented Jun 20, 2019

Just a small remark. I have no 2 tokens:

  • TOTP
  • TIQR

When I try to authenticate via TIQR de failcounter for TOTP is increased. Will this patch prevent this?

@cornelinux

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

In some cases, when you fail to authenticate, and privacyidea can not determine the token, the failcounter of TOTP will increase. This is by design and will not be changed.

So the question is: In which case, with which API call?

@basvandervlies

This comment has been minimized.

Copy link
Author

commented Jun 20, 2019

In some cases, when you fail to authenticate, and privacyidea can not determine the token, the failcounter of TOTP will increase. This is by design and will not be changed.

So the question is: In which case, with which API call?

It is via the privacyidea webui. I just fill in my TIQR pincode and then QR image is displayed. After each refresh the failcounter for the TOTP token is increased.

@cornelinux

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

If your TOTP token is configured to be a challenge response token and has the same password/OTPPIN like the TiQR token the failcounter will be (and must be) increased by design.

If the TOTP token is not configured to be a challenge response token or if both tokens have different PINs, the fail counter will not be increased.

@basvandervlies

This comment has been minimized.

Copy link
Author

commented Jun 20, 2019

Each token has unique OTPIN/password so first I do a TIQR pincode request:

curl -X POST 'https://pi-web2.surfsara.nl/validate/check?user=bas&pass=0415'

and then I check if TIQR challenge is accepted via the transaction_id, but I get this error message:

"Response did not match for 2 tokens.",

I assume that the PI webinterface use the same approach

@cornelinux

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Thanks! From the code I see, that currently the TOTP in fact will be increased, since your 2nd request does not identify the token and thus both tokens are assumed be a candidate for the response to the challenge.
See #1699

@fredreichbier

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Hm, I think this should be fixed now by the combination of #1698 and #1700?

@cornelinux

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

Hello @basvandervlies
yes, we implemented this in PR #1698 in a elegant and generic way.
Push and TIQR tokens are out of band, a /validate/check with a transaction id will not increase the failcounter.

So we are closing this pull request.

THanks a lot anyways for the pull request, your idea and bringing this up.

I see you are located in the Netherlands. If by any chance you plan to stop by at the FroSCon, on August 11th we have a developer room for the whole day to discuss new features and new ideas within privacyIDEA.
See: https://community.privacyidea.org/t/call-for-content-for-project-room-at-froscon/1058

@cornelinux cornelinux closed this Jun 25, 2019

@basvandervlies

This comment has been minimized.

Copy link
Author

commented Jul 1, 2019

Hello @basvandervlies
yes, we implemented this in PR #1698 in a elegant and generic way.
Push and TIQR tokens are out of band, a /validate/check with a transaction id will not increase the failcounter.

Thanks for picking this up. I am just back from holidays.

I see you are located in the Netherlands. If by any chance you plan to stop by at the FroSCon, on August 11th we have a developer room for the whole day to discuss new features and new ideas within privacyIDEA.
See: https://community.privacyidea.org/t/call-for-content-for-project-room-at-froscon/1058

Maybe I will attend the conference. I will put it in my agenda. I was at the T-dose conference at Eindhoven where I spoke with you after the presentation.

@cornelinux

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Maybe I will attend the conference. I will put it in my agenda. I was at the T-dose conference at Eindhoven where I spoke with you after the presentation.

Ah! Hi!
It would be great if you could attend. If you would like to share your experiences and give a short talk, drop me a note!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.