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

check key length when creating tokens in the UI. #1631

Open
cornelinux opened this issue May 15, 2019 · 7 comments

Comments

Projects
None yet
2 participants
@cornelinux
Copy link
Member

commented May 15, 2019

check for the length of the otpkey when creating an HOTP or TOTP token.

Based on the hash algo we can check the otp length to be correct.
I think we should do this in javascript in the webui.

I am not sure if we should avoid creating a token with a wrong key length or if we only should warn.

@plettich

This comment has been minimized.

Copy link
Member

commented May 22, 2019

We should also check if the given/default hash-algorithm matches the key length or set the appropriate hash-algorithm based on the key length (like in importotp.py)
This should be done by the back end, i think.

@cornelinux

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

Yes, we should do this in the API, not UI.

@cornelinux cornelinux self-assigned this Jun 8, 2019

@cornelinux

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2019

I was trying to start to implement this.
But things can get arbitrary complicated.

We have "base32check", when the otpkey is passed base32 encoded.
We have a "shorter" client component of e.g. only 11 bytes, if we do a 2step enrollment. :-/

I wonder if we should add a classmethod, that checks for parameter aspects during enrollment.
I am currently not quite sure, how we should implement this.

For HOTP I added here https://github.com/privacyidea/privacyidea/blob/master/privacyidea/lib/tokens/hotptoken.py#L337

if "otpkey" in upd_param and upd_param.get("otpkeyformat", "hex") == "hex":
            # If the user provides an otpkey, we check the length unless base32check
            check_hash_keylength(hashlibStr, len(otpKey)/2)

But it breaks with 2step enrollment. We could also exclude is_true(params.get("2stepinit")), but a more generic approach would be great.

cornelinux added a commit that referenced this issue Jun 8, 2019

implemenation example.
Does not work out quite, since the keylength/keysize can be any kind
during 2step, can be even set as parameter...

Working on #1631
@cornelinux

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2019

I wonder, if we should put this in the UI.

@plettich

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Maybe we can do the check before the key is written to the db. At this point, the key should be complete.
See tokenclass.py

@cornelinux

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

This is still a bit nasty, since

  1. each token type can have different rules. But this is fine, this can be implemented in the tokenclasses.
  2. the much bigger challenge is, that due to different rollout mechansims like two step etc. there are a lot of different rules for the key length. :-/

I think we should not check the keylength all the time, but maybe only if certain conditions are met like:

  • genkey is not set
  • two_step is not set (since this generates keys anyways)
  • We have an otpkeyformat where the otpkey can be hex or base32... :-/
  • only for hotp and totp...

There are some nasty, old parameters like "keysize". This is totally bullocks, since the keysize depends on used hash algorithm.

@cornelinux

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

Thinking about it again I think it is the best to create a decorator for the API call to token/init.
There is a lot of (policy) logic that is tested on the API level before a DB token is created at all:
https://github.com/privacyidea/privacyidea/blob/master/privacyidea/api/token.py#L115

If we now add a check, that the seed-length for HOTP and TOTP token matches the expected hashlib, then we do not want to create a DB token. And maybe next time another users says, that he wants to be able to create HMAC-SHA1 token with a 64byte long seed. So this will need to be configurable - in a policy. And this would be added as a prepolicy-decorator anyways.
What do you think? @fredreichbier @plettich

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.