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

Argon2 support #1690

Merged
merged 6 commits into from Mar 19, 2019

Conversation

Projects
None yet
4 participants
@Arvedui
Copy link
Contributor

Arvedui commented Mar 3, 2019

Description of the issue/feature this PR addresses:
This PR implements support for the argon2 password hash and adds the ability to rehash passwords not only for schema changes but also for parameter changes. Right now the later is only implemented for argon2 hashes, but could also be interesting for sha{256,512}crypt hashes.

Current behavior before PR:
no argon2 support

Desired behavior after PR is merged:
working argon2 support with the ability to rehash password for parameter update on login

I have not (yet) written any documentation about the argon2 functionality. Because I'm unsure where it would fit best, any ideas?

#1673

@Arvedui

This comment has been minimized.

Copy link
Contributor Author

Arvedui commented Mar 3, 2019

I added argon2 to the reqirements txt, any idea why it could not be imported?

with self.settings(
MODOBOA_ARGON2_TIME_COST=3,
MODOBOA_ARGON2_MEMORY_COST=1000,
MODOBOA_ARGON2_PARALLELISM=2):

This comment has been minimized.

This comment has been minimized.

Copy link
@tonioo

tonioo Mar 14, 2019

Member

@Arvedui Do you know why it happens?

This comment has been minimized.

Copy link
@Arvedui

Arvedui Mar 14, 2019

Author Contributor

No idea, but I thought I fixed it. codacy does not acknowledge that though and now pylint seems unhappy because of not enough indention …

self.client.logout()
with self.settings(
MODOBOA_ARGON2_TIME_COST=3,
MODOBOA_ARGON2_MEMORY_COST=1000,

This comment has been minimized.

@@ -12,6 +12,8 @@
from django.test import override_settings
from django.urls import reverse

import argon2

This comment has been minimized.

Copy link
@codacy-bot
with self.settings(
MODOBOA_ARGON2_TIME_COST=4,
MODOBOA_ARGON2_MEMORY_COST=10000,
MODOBOA_ARGON2_PARALLELISM=4):

This comment has been minimized.

self.client.post(reverse("core:login"), data)
user.refresh_from_db()
self.assertTrue(user.password.startswith("{ARGON2ID}"))
parameters = argon2.extract_parameters(user.password.lstrip("{ARGON2ID}"))

This comment has been minimized.

Copy link
@codacy-bot
@@ -9,6 +9,9 @@
from __future__ import unicode_literals

from passlib.hash import bcrypt, md5_crypt, sha256_crypt, sha512_crypt
from argon2 import PasswordHasher as argon2_hasher

This comment has been minimized.

Copy link
@codacy-bot

self.client.logout()
with self.settings(
MODOBOA_ARGON2_TIME_COST=4,

This comment has been minimized.

@@ -9,7 +9,7 @@
from django.utils.encoding import smart_text

from modoboa.core.password_hashers.advanced import ( # NOQA:F401
BLFCRYPTHasher, MD5CRYPTHasher, SHA256CRYPTHasher, SHA512CRYPTHasher
BLFCRYPTHasher, MD5CRYPTHasher, SHA256CRYPTHasher, SHA512CRYPTHasher, ARGON2IDHasher

This comment has been minimized.

Copy link
@codacy-bot

self.client.logout()
with self.settings(
MODOBOA_ARGON2_TIME_COST=3,

This comment has been minimized.

self.client.logout()
with self.settings(
MODOBOA_ARGON2_TIME_COST=4,
MODOBOA_ARGON2_MEMORY_COST=10000,

This comment has been minimized.

@tonioo

This comment has been minimized.

Copy link
Member

tonioo commented Mar 12, 2019

@Arvedui I guess it's just a codacy issue... Have you tried to run the test suite?

@Arvedui

This comment has been minimized.

Copy link
Contributor Author

Arvedui commented Mar 13, 2019

I did. My tests all run fine. There are a few failures but I believe they are unrelated to my changes, since they also occur on current master.

@Arvedui Arvedui marked this pull request as ready for review Mar 13, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 13, 2019

Codecov Report

Merging #1690 into master will increase coverage by 0.02%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master    #1690      +/-   ##
==========================================
+ Coverage   86.71%   86.74%   +0.02%     
==========================================
  Files         150      150              
  Lines        7896     7928      +32     
==========================================
+ Hits         6847     6877      +30     
- Misses       1049     1051       +2
@tonioo

This comment has been minimized.

Copy link
Member

tonioo commented Mar 14, 2019

@Arvedui Ok.

@tonioo tonioo merged commit 3e2ddc0 into modoboa:master Mar 19, 2019

3 checks passed

codecov/patch 93.75% of diff hit (target 70%)
Details
codecov/project 86.74% (+0.02%) compared to dd2e8ab
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tonioo tonioo added the enhancement label Mar 19, 2019

@tonioo tonioo added this to the 1.13.2 milestone Mar 19, 2019

@jeromelebleu

This comment has been minimized.

Copy link

jeromelebleu commented Mar 19, 2019

Is it possible to not force the dependency to argon2-cffi please? As the Django documentation says, it requires a third-party library that not everybody can or want to install on it system. One way to do that could be a try...except ImportError statement, I can purpose a PR in this direction if you want. Thanks!

@tonioo

This comment has been minimized.

Copy link
Member

tonioo commented Mar 19, 2019

@jeromelebleu yes please, I didn't notice this.

@jeromelebleu

This comment has been minimized.

Copy link

jeromelebleu commented Mar 19, 2019

@tonioo okay, I will try so...

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.