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

1342 pushtoken #1494

Open
wants to merge 31 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@cornelinux
Copy link
Member

cornelinux commented Mar 12, 2019

This PR implements the PUSH Token.

The smartphone app and the push token object in the privacyIDEA server each have
an assymmetric key pair. They share their public keys and the app gets notified via
the Google Firebase service.

(I think currently missing: The documentation for the push policies and the push token itself!)

Closes #1342

@cornelinux cornelinux requested a review from privacyidea/core Mar 12, 2019

@fredreichbier
Copy link
Member

fredreichbier left a comment

I didn't do a full review yet, but I do have some first comments. :-)

Show resolved Hide resolved privacyidea/api/lib/prepolicy.py Outdated
This create a keypair, either RSA or ECC.
The HSM should be used.
# TODO: This must be much nicer...

This comment has been minimized.

@fredreichbier

fredreichbier Mar 14, 2019

Member

I guess we should try not to use the internal PyOpenSSL functions: Wouldn't something like https://cryptography.io/en/latest/hazmat/primitives/asymmetric/rsa/#cryptography.hazmat.primitives.asymmetric.rsa.generate_private_key work? We could even return the RSAPrivateKey instance, as it has a public_key() method that retrieves the public key.

This comment has been minimized.

@cornelinux

cornelinux Mar 19, 2019

Author Member

will do

This comment has been minimized.

@cornelinux

cornelinux Mar 19, 2019

Author Member

done in 79b5d2c

:param key:
:return:
"""
return key.strip().strip("-BEGIN END RSA PUBLIC PRIVATE KEY").strip()

This comment has been minimized.

@fredreichbier

fredreichbier Mar 14, 2019

Member

I think this may corrupt some keys because strip removes all characters "-", "B", "E", etc from the beginning and the end of the string, so we may strip characters from keys whose base64 representation contains any of these characters.

Maybe we can somehow use cryptography functions for that? Like https://cryptography.io/en/latest/hazmat/primitives/asymmetric/rsa/#key-serialization

Show resolved Hide resolved privacyidea/lib/tokens/pushtoken.py Outdated
Show resolved Hide resolved privacyidea/lib/tokens/pushtoken.py Outdated
pem_privkey = self.get_tokeninfo(PRIVATE_KEY_SERVER)
privkey_obj = serialization.load_pem_private_key(str(pem_privkey), None, default_backend())

# Sign the data with PKCS1 padding. Not all Androids support PSS padding.

This comment has been minimized.

@fredreichbier

fredreichbier Mar 14, 2019

Member

I have some thoughts about the choice of RSA signatures with PKCS#1 v1.5 padding:
While PKCS#1 v1.5 signatures are not broken (see here), the general recommendation for new applications seems to be PSS padding (see here).

Instead of RSA, one could even use some elliptic curve signature scheme like ed25519 (recommended by cryptography). In our case, using ed25519 instead of RSA would reduce the size of stored keys (2048 bits to only 256 bits).

The comment suggests that we cannot use PSS because of legacy(?) Androids. I guess ed25519 isn't better in this regard :-)

But what do you think about adding a "signature algorithm" field to the smartphone_data, which currently says "rsa_sha256_pkcs1v15"? This would allow us to upgrade the signature scheme without too much hassle at some point in the future.

This comment has been minimized.

@fredreichbier

fredreichbier Mar 15, 2019

Member

After a good nights sleep, I realized that smartphone_data isn't the right place to add the signature algorithm field, it would need to be part of the enrollment process.

This comment has been minimized.

@cornelinux

cornelinux Mar 15, 2019

Author Member

I like it. But I am not sure, if this should stop our current going forward.

This comment has been minimized.

@fredreichbier

fredreichbier Mar 15, 2019

Member

I agree that this would mean some extra work now. But including a version marker at some place now may save us time and headaches in the future when we want to migrate to some other signature algorithm.

This comment has been minimized.

@cornelinux

cornelinux Mar 19, 2019

Author Member

will do

This comment has been minimized.

@cornelinux

cornelinux Mar 19, 2019

Author Member

We will add "v=1" to the QR Code and the smartphone will check, that it is not v=2.

This comment has been minimized.

@cornelinux

cornelinux Mar 19, 2019

Author Member

done in 21b07b3 (that was easy)

Show resolved Hide resolved privacyidea/lib/tokens/pushtoken.py Outdated
Show resolved Hide resolved privacyidea/lib/tokens/pushtoken.py Outdated
Show resolved Hide resolved privacyidea/lib/tokens/pushtoken.py
Show resolved Hide resolved privacyidea/lib/tokens/pushtoken.py
pem_privkey = self.get_tokeninfo(PRIVATE_KEY_SERVER)
privkey_obj = serialization.load_pem_private_key(str(pem_privkey), None, default_backend())

# Sign the data with PKCS1 padding. Not all Androids support PSS padding.

This comment has been minimized.

@fredreichbier

fredreichbier Mar 15, 2019

Member

I agree that this would mean some extra work now. But including a version marker at some place now may save us time and headaches in the future when we want to migrate to some other signature algorithm.

Show resolved Hide resolved privacyidea/lib/smsprovider/SMSProvider.py
Show resolved Hide resolved privacyidea/lib/tokens/pushtoken.py Outdated
Show resolved Hide resolved privacyidea/lib/tokens/pushtoken.py Outdated
Show resolved Hide resolved privacyidea/lib/tokens/pushtoken.py
Show resolved Hide resolved privacyidea/lib/tokens/pushtoken.py Outdated
Show resolved Hide resolved privacyidea/lib/tokens/pushtoken.py Outdated
Show resolved Hide resolved privacyidea/lib/tokens/pushtoken.py Outdated
Show resolved Hide resolved tests/test_api_lib_policy.py Outdated
Show resolved Hide resolved tests/test_lib_tokens_push.py
Show resolved Hide resolved tests/test_lib_tokens_push.py Outdated

@cornelinux cornelinux marked this pull request as ready for review Mar 19, 2019

cornelinux added some commits Mar 19, 2019

Cleanup keys to be stored as unicode
to run with python2 and python3

@cornelinux cornelinux requested a review from fredreichbier Mar 19, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #1494 into master will increase coverage by 0.04%.
The diff coverage is 95.01%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1494      +/-   ##
==========================================
+ Coverage    96.8%   96.84%   +0.04%     
==========================================
  Files         144      149       +5     
  Lines       17259    17941     +682     
==========================================
+ Hits        16707    17375     +668     
- Misses        552      566      +14
Impacted Files Coverage Δ
privacyidea/lib/challenge.py 86.27% <100%> (+0.56%) ⬆️
privacyidea/api/token.py 98.61% <100%> (ø) ⬆️
privacyidea/api/lib/utils.py 96.66% <100%> (+1.47%) ⬆️
privacyidea/lib/config.py 94.69% <100%> (+0.01%) ⬆️
privacyidea/lib/smsprovider/SMSProvider.py 93.33% <100%> (+0.22%) ⬆️
privacyidea/lib/crypto.py 96.03% <100%> (+0.12%) ⬆️
privacyidea/api/lib/prepolicy.py 97.18% <81.25%> (-0.5%) ⬇️
privacyidea/lib/utils.py 97.7% <89.47%> (-0.34%) ⬇️
privacyidea/lib/tokens/pushtoken.py 95.71% <95.71%> (ø)
privacyidea/lib/smsprovider/FirebaseProvider.py 97.22% <97.22%> (ø)
... and 12 more

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 d8a2963...21b07b3. Read the comment docs.

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.