6
\$\begingroup\$

I have an application that needs to validate user passwords to a very reasonable custom validator, which is heavily based on NIST-2017 requirements. The validator intends to satisfy the following restrictions:

  • At least n characters (default n is 12);
  • No more than n sequential, neighboring digits, things like 123, 1212, 98767 (default n is 3);
  • No more than n sequential, repeated characters, things like aaa, 111, etc. (default n is 2);
  • No more than n total characters matching the list of InvalidSubstrings, things like password, etc. (default n is 5);
  • At least n distinct characters (default n is 5);

In my opinion, these are very reasonable requirements: there's no requirement of symbols/digits/mixed case, just a somewhat long length requirement, and it cannot have obvious insecure strings in it.

There are two sections to this class: the PasswordValidator C# class, and the Password F# module.

The class is straightforward:

/// <summary>
/// Validates passwords mostly to the NIST-2017 standard.
/// </summary>
/// <remarks>
/// The requirements with this validator are that the password meet all of the following criteria:
/// 
/// <list type="ol">
/// <item>is no less than <see cref="MinimumLength"/> characters;</item>
/// <item>does not contain more than <see cref="SequentialDigitsAllowed"/> sequential digits in ascending or descending order in any position (valid: 123, invalid: 1234, invalid: 1210);</item>
/// <item>does not contain more than <see cref="RepeatedCharactersAllowed"/> sequential repeated characters in any position (valid: aab, invalid: aaa);</item>
/// <item>does not contain more than <see cref="MaximumInvalidSubstringLength"/> total characters matching any string in <see cref="InvalidSubstrings"/> in any case, direction or obsfucation (i.e. 'p@ssword', 'p@ssw0rd');</item>
/// <item>contains at least <see cref="DistinctCharacters"/> distinct characters;</item>
/// </list>
/// </remarks>
public class PasswordValidator : IIdentityValidator<string>
{
    /// <summary>
    /// Minimum number of distinct characters in the password.
    /// </summary>
    public int DistinctCharacters { get; set; } = 5;
    /// <summary>
    /// Minimum required length of a password.
    /// </summary>
    public int MinimumLength { get; set; } = 12;
    /// <summary>
    /// Maximum allowed sequential digits. (That is, a digit preceeded by a digit one value above or below the current digit.)
    /// </summary>
    public int SequentialDigitsAllowed { get; set; } = 3;
    /// <summary>
    /// Maximum allowed sequential repeated characters.
    /// </summary>
    public int RepeatedCharactersAllowed { get; set; } = 2;
    /// <summary>
    /// Maximum total length of all invalid substrings.
    /// </summary>
    public int MaximumInvalidSubstringLength { get; set; } = 5;
    /// <summary>
    /// If true then the entire list of substrings forbidden will be returned, if false only the matched substring will be returned.
    /// </summary>
    public bool ReturnSpecificForbiddenSubstrings { get; set; } = true;

    /// <summary>
    /// These are some of the strings and substrings of commonly used and exploited password.
    /// </summary>
    public static IReadOnlyList<string> InvalidSubstrings { get; } = new List<string> {
        "password",
        "admin",
        "qaz",
        "wsx",
        "qwerty",
        "uiop",
        "asdf",
        "ghjk",
        "zxcv",
        "bnm",
        "login",
        "abc123",
        "123abc",
        "trustno1",
        "trustnoone",
        "azerty",
        "letmein",
        "welcome",
        "123123",
        "1q2w",
        "q1w2",
        "3e4r",
        "e3r4",
        "5t6y",
        "t5y6",
        "7u8i",
        "u7i8",
        "9o0p",
        "o9p0",
        "123qwe",
        "qwe123",
        "google",
        "mynoob",
        "zaq1",
    }.AsReadOnly();

    public IdentityResult Validate(string item)
    {
        var length = item.Length;

        item = item.ToLower();

        if (length < MinimumLength)
        {
            return new IdentityResult($"The password must be at least {MinimumLength} characters in length.");
        }

        string buildSample(int len, Func<int, string> map) =>
            string.Join("", Enumerable.Range(1, len).Select(map));

        if (Password.longestSequentialDigitString(item).Length > SequentialDigitsAllowed)
        {
            return new IdentityResult($"The password cannot have more than {SequentialDigitsAllowed} sequential, adjacent, ordered digits in any position. ('{buildSample(SequentialDigitsAllowed, x => (x % 10).ToString())}' is valid, '{buildSample(SequentialDigitsAllowed + 1, x => (x % 10).ToString())}' is not)");
        }

        if (Password.longestRepeatedString(item).Length > RepeatedCharactersAllowed)
        {
            return new IdentityResult($"The password cannot have more than {RepeatedCharactersAllowed} sequential, repeated characters in any position. ('{buildSample(RepeatedCharactersAllowed, x => "a")}b' is valid, '{buildSample(RepeatedCharactersAllowed + 1, x => "a")}' is not)");
        }

        if (Password.distinctCharacters(item).Count() < DistinctCharacters)
        {
            return new IdentityResult($"The password must contain at least {DistinctCharacters} distinct characters.");
        }

        var deObsfucated = Password.deObsfucate(item);
        var matched = Password.getMatchedSubstrings(InvalidSubstrings, deObsfucated);

        if (matched.Sum(x => x.Length) > MaximumInvalidSubstringLength)
        {
            if (ReturnSpecificForbiddenSubstrings)
            {
                return new IdentityResult($"The password cannot contain any of: {string.Join(", ", InvalidSubstrings.Select(x => $"'{x}'"))} in forward or reverse order.");
            }
            else
            {
                return new IdentityResult($"The password entered contains a phrase which ranks too high on the list of commonly used/attacked/broken passwords. The password cannot contain: {string.Join(", ", matched.Select(x => $"'{x}'"))}.");
            }
        }

        return IdentityResult.Success;
    }

    public Task<IdentityResult> ValidateAsync(string item) =>
        Task.Run(() => Validate(item));
}

The IIdentityValidator<string> interface requires the ValidateAsync method (ASP.NET Simple Membership). The F# portion has two modules: String and Password.

module String =
    let rev : string -> string =
        Seq.toArray
        >> Array.rev
        >> System.String

module Password =
    let private listIsOrderedDigits str =
        str
        |> (List.skip 1
        >> List.fold(fun (result, prev) x ->
            let sequentials =
                match prev with
                | '0' -> ['9'; '1']
                | '1' -> ['0'; '2']
                | '2' -> ['1'; '3']
                | '3' -> ['2'; '4']
                | '4' -> ['3'; '5']
                | '5' -> ['4'; '6']
                | '6' -> ['5'; '7']
                | '7' -> ['6'; '8']
                | '8' -> ['7'; '9']
                | '9' -> ['8'; '0']
                | _ -> []
            ((sequentials |> List.contains x) && result, x)
        ) (true, str |> Seq.head)
        >> fst)

    let isOrderedDigits str =
        str |> (Seq.toList >> listIsOrderedDigits)

    let private deObsfucateChar c =
        match c with
        | '@' -> 'a'
        | '0' -> 'o'
        | '3' -> 'e'
        | '2' -> 'z'
        | '5' -> 's'
        | '1' -> 'i'
        | '!' -> 'i'
        | '4' -> 'a'
        | '7' -> 't'
        | '9' -> 'g'
        | '8' -> 'b'
        | _ -> c

    let deObsfucate str =
        str |> (Seq.toArray >> Array.map deObsfucateChar >> System.String)

    let substringIn (str : string) (substr : string) =
        str.Contains(substr) || str.Contains(substr |> String.rev)

    let getMatchedSubstrings substrings str =
        substrings |> Seq.filter (substringIn str)

    let private pick l1 l2 =
        [l1; l2] |> List.maxBy List.length

    let longestRepeatedString str =
        let folder (longest, curr) x =
            match curr, x with
            | f::_, x when x = f -> (longest, x::curr)
            | f::_, x -> ((longest, curr) ||> pick, [x])
            | _ -> (longest, [x])
        str
        |> Seq.fold folder ([], [])
        ||> pick
        |> List.toArray
        |> System.String

    let longestSequentialDigitString str =
        let folder (longest, curr) x =
            match curr, x with
            | c, x when x::c |> listIsOrderedDigits -> (longest, x::c)
            | _ -> ((longest, curr) ||> pick, [x])
        str
        |> Seq.fold folder ([], [])
        ||> pick
        |> List.toArray
        |> Array.rev
        |> System.String

    let distinctCharacters str =
        str |> (Seq.groupBy id >> Seq.map fst >> Seq.toArray)

I wrote the backbone in F# for three reasons:

  • REPL is easy to test;
  • It's very intuitive;
  • Total line count dropped ~200 lines;

Any comments on either selection of code is appreciated.

\$\endgroup\$
  • 5
    \$\begingroup\$ It's a real shame the interface enforces a single result. It's much more user friendly IMO to run all validations so the user can be informed of many issues at once instead of getting stuck in the "I fixed it. What's wrong now?!" loop. \$\endgroup\$ – RubberDuck Sep 23 '17 at 14:15
  • 1
    \$\begingroup\$ @RubberDuck The IdentityResult supports an array of errors, wouldn't be hard to modify for (that would actually be a very valid answer!). :) \$\endgroup\$ – Der Kommissar Sep 23 '17 at 14:17
5
\$\begingroup\$

I don't know F# at all, but I really like the code.

You almost successfully separated the high-level code (first file) from the low-level code (second file). You could follow this principle also for generating the feedback strings so that the upper code really only consists of a simple sequence of if statements.

I am assuming that you didn't only test your code by trying it in the REPL but also have a set of unit tests for the F# part. If you don't, you should add some, at least for the longer functions.

Contrary to BKSpurgeon's answer I would not add OOP design patterns to the code, which is currently concise and to the point and easily extendible to new requirements.

Did you test the password requirements against auto-generated passwords of 256 bits entropy, encoded either as hex or base64? Most of these passwords should pass the validation, otherwise the requirements are impractical for these users.

I find the ReturnSpecific setting confusing. It should have the opposite meaning since I consider "your password contains the forbidden word X" more specific than "your password contains one of the forbidden words Xs".

\$\endgroup\$
  • \$\begingroup\$ Totally agree on the tests and ReturnSpecific - there aren't any tests yet but there really should be, and that name is very misleading. I'm going to test it on a bunch of relatively high-entropy passwords shortly, to see if it wigs out or not. :) \$\endgroup\$ – Der Kommissar Sep 26 '17 at 13:24
4
\$\begingroup\$

Code to be more OOP:

Here’s why I don’t particularly like it. Let’s suppose that you had to change something: you want to add in another validation. That means you would have to modify the Password Validator class (one change) and you would also have to add in a corresponding method in the F# code (another change). In other words, things would be open for modification. Ideally if we want to extend something, we would want to do so without being forced to make changes to any existing classes. Forcing changes in two separate classes – I don’t like that idea.

Ideally if you wanted to add something you should be able to easily derive another class from a base class (or interface) – in this instance we are not editing any existing code. But in order to instantiate this new class then tehnically we would actually have to modify something i.e. a factory method - but this is more than acceptable is but a minor change that is unavoidable.

Perhaps this is best illustrated by example. Consider the following code:

private static void Main(string[] args)
{
    Vehicle v = Vehicle.Factory(1); // implement it properly, obviously
    v.Drive();
}

abstract class Vehicle
{
    public abstract void Drive();

    internal static Vehicle Factory(int i)
    {
        if (i == 1)
        {
            return new Car();
        }
        else
        {
            return new Bike();
        }
    }
}

class Car : Vehicle
{
    public override void Drive()
    {
        Console.WriteLine("Brrrmm");
    }
}

class Bike : Vehicle
{
    public override void Drive()
    {
        Console.WriteLine("Pedal power.");
    }
}

Things are coming along quite nicely. Until I decide that I want to create another vehicle. I want to create a SUV class. I can easily do so:

class SUV : Vehicle
{
     public override void Drive()
     {
        Console.WriteLine("Move, sir – get out da way, get out da way!");
     }
}

I’ve added this class without modifying anything. Technically I would be editing the Factory method – but that’s unavoidable – for the most part, I can add things without being forced to edit any existing classes (apart from the factory method).

The approach the PasswordValidator class takes is not exactly open to extension yet closed to modification. It could easily be though.

Possible Ideas

public class Validator
{
    List<IIdentityValidator<string>> validations;

    protected Validator()
    {
        this.validations = ValidationsFactory();
    }

    public bool Validate()
    {
        // iterate through the list
        // make sure that everyone single one passes.
        return true; // or false as the case may be - according to whether it passes or fails
    }

    public static List<IIdentityValidator<string>> ValidationsFactory()
    {
        List<IIdentityValidator<string>> validations = new List<IIdentityValidator<string>>() 
            {
                new DistinctCharacters(),
                new MinimumLength()
                // etc. etc.
            };

        return validations;
    }
}

public class DistinctCharacters : IIdentityValidator<string>
{
    // etc  etc/
}

public class MinimumLength  : IIdentityValidator<string>
{

}

Separation of Concerns.

Are the responsibilities kept separate? If you go for the above approach – if you wanted to make a change to the Minimum length class – you could do so – and the only class that would be affected would be the minimum length class (and the factory method, but let’s ignore that for a second). That’s a huge boon – you need to make changes basically in only one place.

Now let’s consider code snippet here:

if (Password.longestRepeatedString(item).Length > RepeatedCharactersAllowed)
{
    ///
}

If I decided to change the logic inside the static Password.longestRepeatedString method, then I would also have to make a corresponding check with the above logic in the if statement – to make sure that all is kosher. In other words, a change in one place forces me to make sure there are no spill over effects in an entirely different class – the PasswordValidator class. In other words, I would say that the PasswordValidator and the static Password class are highly coupled and the concerns are not as cleanly separated as one would possibly like.. You can minimise this coupling by using the approach suggested above.

Break up the methods.

Take the following for example - if you came back in 6 months you'd have to think what is going on here - if not you, then the poor developer who comes after you and is forced to maintain your code. Extract from the following, and simplify it:

 let longestRepeatedString str =
        let folder (longest, curr) x =
            match curr, x with
            | f::_, x when x = f -> (longest, x::curr)
            | f::_, x -> ((longest, curr) ||> pick, [x])
            | _ -> (longest, [x])
        str
        |> Seq.fold folder ([], [])
        ||> pick
        |> List.toArray
        |> System.String

See this for more info:

https://www.jetbrains.com/help/resharper/Refactorings__Extract_Method.html

Other design patterns:

You could possibly employ the decorator pattern, or some have suggested the chain of responsibility pattern. Or to keep it simple, a sub-classing approaching like the above should work just fine.

Hope this gives rise to some ideas - hope it helps.

\$\endgroup\$
  • \$\begingroup\$ I think I'm going to use a combination of this composition with the current setup, instead of implementing each F# method as a new interface in C#, I'll keep the implementation but write a C# wrapper around it that only filters for the current operation. That way it can still be composed as you suggest, but also allow me to keep the F# bit (which I personally find more readable than the C# equivalent). :) \$\endgroup\$ – Der Kommissar Sep 26 '17 at 13:23
  • 1
    \$\begingroup\$ Adding more OOP to this solution doesn't do much but add more complexity. All of the benefits of your proposed solution could be achieved with a list of Tuple<Func, Message>. \$\endgroup\$ – RubberDuck Sep 26 '17 at 18:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.