2
\$\begingroup\$

My goal is to make a very simple library with the authentications basics: create a user and save its hashed password, change its password, have a method to check if the password is correct for login.

The usage I thought (you find everything in my Github repository), is this:

var cs = "Data Source=localhost\\SQLExpress; Integrated Security=SSPI; Initial Catalog=PLAYGROUND;";
var usersRepository = new SQLServerUsersRepository(cs);
var hasher = new Pbkdf2PasswordHasher();
var users = new Users(usersRepository, hasher);

await users.CreateUser(
    username: "user", 
    plainTextPassword: "someVeryNiceRandomPasswordDifferentFromTheOthers", 
    firstName: "user's name", 
    lastName: "user's surname", 
    userType: UserTypes.SimpleUser);

// If the login fails, an AuthenticationFailedException is thrown
var loggedUser = users.Login("user", "wrongPassword");

To do this, I coded a User class which depends on IUsersRepository and IPasswordHasher.

public async Task CreateUser(
    string username,
    string plainTextPassword,
    string firstName,
    string lastName,
    UserTypes userType)
{
    if (string.IsNullOrWhiteSpace(username))
        throw new EmptyFieldException(nameof(username));
    if (string.IsNullOrWhiteSpace(plainTextPassword))
        throw new EmptyFieldException(nameof(plainTextPassword));
    if (string.IsNullOrWhiteSpace(firstName))
        throw new EmptyFieldException(nameof(firstName));
    if (string.IsNullOrWhiteSpace(lastName))
        throw new EmptyFieldException(nameof(lastName));

    var salt = _hasher.GenerateSalt();
    var hashedPassword = _hasher.Hash(plainTextPassword, salt);

    await _usersRepository.Create(new User()
    {
        FirstName = firstName,
        LastName = lastName,
        PasswordHash = hashedPassword,
        PasswordSalt = salt,
        Username = username,
        UserStateId = UserStates.Active,
        UserTypeId = userType,
        RegistrationDate = DateTimeOffset.Now
    });
}

The User table is defined as this:

CREATE TABLE [Users].[Users]
(
    [UserId]            INT IDENTITY(1, 1)  NOT NULL,
    [UserTypeId]        TINYINT             NOT NULL,
    [UserStateId]       TINYINT             NOT NULL,
    [Username]          VARCHAR(64)         NOT NULL,
    [PasswordHash]      BINARY(32)          NOT NULL,
    [PasswordSalt]      BINARY(16)          NOT NULL,
    [FirstName]         VARCHAR(64)         NOT NULL,
    [LastName]          VARCHAR(64)         NOT NULL,
    [RegistrationDate]  DATETIMEOFFSET(0)   NOT NULL

    CONSTRAINT [PK_Users_Users_UserId]           PRIMARY KEY CLUSTERED (UserId ASC),
    CONSTRAINT [UK_Users_Users_Username]         UNIQUE (Username),
    CONSTRAINT [UK_Users_Users_FirstLastName]    UNIQUE (FirstName, LastName),
    CONSTRAINT [FK_Users_UserTypes_UserTypeId]   FOREIGN KEY (UserTypeId) REFERENCES Users.UserTypes(UserTypeId),
    CONSTRAINT [FK_Users_UserStates_UserStateId] FOREIGN KEY (UserStateId) REFERENCES Users.UserStates(UserStateId)
);

I choose DateTimeOffset because I want to not depend on the server time zone.

Then, the security part. What do you think about my choice? I choosen Pbkdf2 algorithm with HMACSHA1 with salt. To check if the two password hashe (the first from database, the other from the just hashed password during login) I used ReadOnlySpan<byte>.SequenceEqual:

public class Pbkdf2PasswordHasher : IPasswordHasher
{
    public byte[] Hash(string plaintextPassword, byte[] salt)
    {
        return KeyDerivation.Pbkdf2(
            password: plaintextPassword,
            salt: salt,
            prf: KeyDerivationPrf.HMACSHA1,
            iterationCount: 20_000,
            numBytesRequested: 256 / 8);
    }

    public byte[] GenerateSalt()
    {
        byte[] salt = new byte[128 / 8];
        using (var rng = RandomNumberGenerator.Create())
        {
            rng.GetBytes(salt);
        }

        return salt;
    }

    public bool AreEquals(ReadOnlySpan<byte> a1, ReadOnlySpan<byte> a2)
    {
        return a1.SequenceEqual(a2);
    }
}

What do you think? Can my code be improved? Do you see any bugs or potential security issues?
Thanks!

\$\endgroup\$
2
  • 1
    \$\begingroup\$ In my experience the unique key (FirstName, LastName) is going to cause problems ;-) \$\endgroup\$ Commented Jul 7, 2020 at 16:06
  • \$\begingroup\$ You are right! Thanks @MathieuGuindon! \$\endgroup\$
    – user42412
    Commented Jul 7, 2020 at 21:04

0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.