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

fix: Use passlib for totp_secret #94

Merged
merged 1 commit into from
Jun 4, 2019
Merged

fix: Use passlib for totp_secret #94

merged 1 commit into from
Jun 4, 2019

Conversation

jwag956
Copy link
Collaborator

@jwag956 jwag956 commented Jun 2, 2019

onetimepass seems to be unmaintained - and more importantly totp_secret was being
stored in the DB in the clear as well as in the session cookie in the clear.
That is a big security hole.
passlib has an easy way to encrypt these.

Also:

  • updated docs
  • make sure that on logout, all two factor session elements are cleared out
  • Using two-factor now requires a TWO_FACTOR_SECRET as defined by passlib

This change requires the setting of TWO_FACTOR_SECRET, and installing the cryptography
package.

closes: #89

@jwag956 jwag956 requested a review from TillerBurr June 2, 2019 19:17
@jwag956 jwag956 self-assigned this Jun 3, 2019
code = str(onetimepass.get_totp(totp_secret))
session = get_session(response)

# This shows how dangerous it is to have even an encrypted totp_secret
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unsure of any other way to provide the verification of the second layer. I'm sure there's a way, I just don't know what it is.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just store it on the user model and call it from there?

Copy link
Collaborator Author

@jwag956 jwag956 Jun 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my thought as well - but need to look into it more.

When I go through views (next issue) I'll look into this. I would like to be able to make 2FA per-user - I don't really see any reason that it isn't stored immediately in the user model - but I'll have to trace through a bunch of code...

docs/features.rst Outdated Show resolved Hide resolved
TillerBurr
TillerBurr previously approved these changes Jun 3, 2019
Copy link
Collaborator

@TillerBurr TillerBurr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I don't think pyqrcode is maintained anymore either. The package https://github.com/lincolnloop/python-qrcode looks to be up to date. This is probably another pull though.

I say merge it.

@jwag956
Copy link
Collaborator Author

jwag956 commented Jun 3, 2019

Looks good. I don't think pyqrcode is maintained anymore either. The package https://github.com/lincolnloop/python-qrcode looks to be up to date. This is probably another pull though.

I say merge it.

Interesting - the passlib tutorial actually still references pyqrcode....

onetimepass seems to be unmaintained - and more importantly totp_secret was being
stored in the DB in the clear as well as in the session cookie in the clear.
That is a big security hole.
passlib has an easy way to encrypt these.

Also:
- updated docs
- make sure that on logout, all two factor session elements are cleared out
- Using two-factor now requires a TWO_FACTOR_SECRET as defined by passlib

This change requires the setting of TWO_FACTOR_SECRET, and installing the cryptography
package.

closes: #89
Copy link
Collaborator

@TillerBurr TillerBurr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good. Should the encrypt_password funtion be removed entirely and not just from the docs?

@TillerBurr
Copy link
Collaborator

Looks good. I don't think pyqrcode is maintained anymore either. The package https://github.com/lincolnloop/python-qrcode looks to be up to date. This is probably another pull though.
I say merge it.

Interesting - the passlib tutorial actually still references pyqrcode....

The last release was in June 2016. It works for what it's used for. I don't think that the QR codes are necessarily an issue though since they only appear during setup.

@jwag956 jwag956 merged commit 40b01db into master Jun 4, 2019
@jwag956 jwag956 deleted the totp branch June 6, 2019 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Look into using passlib for totp secrets
2 participants