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

Support BCrypt password encryption #425

Merged
merged 3 commits into from Oct 8, 2020
Merged

Conversation

gquintana
Copy link
Contributor

SHA-256 without salt is considered as breakable.

This pull request adds:

However it still doesn't support SHA-256 with salt and multiple iterations as https://passlib.readthedocs.io/en/stable/lib/passlib.hash.sha256_crypt.html

@tchiotludo
Copy link
Owner

I don't consider this for now.
Why not, but I would prefer to have a configuration option to change the crypt for password and all the user will use the same encryption method.
Also can you add some information on README.md about that ?

@gquintana
Copy link
Contributor Author

BCrypt requires Crypt format ($2?$.*), You may notice I don't remove the prefix with BCrypt.
BCrypt is a modern password hashing algorithm supported by many tools: Ansible in my case.
I would prefer a JVM supported algorithm (like sha256), sadly BCrypt, Argon and PBKDF2 are the accepted password hashing algorithms.
I can remove all the stuff to handle $1$, $5$ and $6$ prefix support if you wish.
I'll fix readme and failing checks.

@tchiotludo
Copy link
Owner

tchiotludo commented Oct 1, 2020

It's why I'm talking about a configuration property that will tell if the user want bcrypt or sha.
It will remove the regexp and if else to try to find which algorithm is used

@gquintana
Copy link
Contributor Author

gquintana commented Oct 1, 2020

I truly understand that this prefix parsing sucks.
However you can not remove $2$ prefx because this is not how BCrypt works.
The configuration property should tell if it's crypt format or not. Not the algorithm id.
Also note that Crypt format and Python's Passlib (used by Ansible) supports Sha algorithms with salt and round options.

@tchiotludo
Copy link
Owner

I was not clear I think 😄 here is what I think.
It will be more explicit and allow to have as many Hash as we need in a future :

@Data
public class BasicAuth {
    String username;
    String password;
    PasswordHash passwordHash;
    List<String> groups = new ArrayList<>();
    
    @SuppressWarnings("UnstableApiUsage")
    public boolean isValidPassword(String password) {
        if (this.passwordHash == PasswordHash.SHA256) {
            // See http://www.mindrot.org/projects/jBCrypt/
            return BCrypt.checkpw(password, this.password);
        } else {
            // Default Sha256 format
            return this.password.equals(
                Hashing.sha256()
                    .hashString(password, StandardCharsets.UTF_8)
                    .toString()
            );
        }
        
        return this.password.equals(
                Hashing.sha256()
                        .hashString(password, StandardCharsets.UTF_8)
                        .toString()
        );
    }
    
    public enum PasswordHash {
        SHA256,
        BCRYPT
    }
}

What do you think about that ?

@gquintana
Copy link
Contributor Author

Yes you were clear.

Except I thought you wanted to make PasswordHash passwordHash a settings shared for all users. I looked at the code, but couldn't find the way to do it. So I resigned to the simplest solution I could think about.

I'll implement that way.

@tchiotludo
Copy link
Owner

BasicAuth is a Configuration Properties that will be filled automatically by micronaut, just add the properties and it's done ;)

@tchiotludo tchiotludo merged commit 78597d7 into tchiotludo:dev Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants