5
\$\begingroup\$

A simple random password generator in C#. Looking for any improvements around speed or design. Any char[] can be passed to choose random chars from. The array I used for testing was a basic hex-only array

char[] hexChars = ['a', 'b', 'c', 'd', 'e', 'f', 
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9'];

using System.Collections;    
using System.Diagnostics;

public struct PasswordGenerator(int passwordLength, char[] legalChars) : IEnumerable<char>
{
    private readonly int PasswordLength => passwordLength;

    public readonly string Result => new(this.ToArray());
    
    public static IEnumerable<string> GeneratePasswords(int size, PasswordGenerator 
        generator) => generator.Chunk(size).Select(chunk => new string(chunk));

    /// <summary>
    /// Creates a random password
    /// </summary>        
    readonly IEnumerator<char> IEnumerable<char>.GetEnumerator()
    {              
        long charsSupplied = 0;
        bool isInvaidChars = legalChars == null || legalChars.Length == 0;
        if(isInvaidChars || PasswordLength <= charsSupplied) yield break;
        
        while(charsSupplied < PasswordLength)
        {                
            yield return legalChars![Random.Shared
                .Next(0, legalChars.Length)];    
            charsSupplied++;
        }
    }

    [DebuggerHidden, DebuggerStepThrough]
    readonly IEnumerator IEnumerable.GetEnumerator()
    {
        throw new NotImplementedException();
    }
}

Some example output using GeneratePasswords. The initial size of passwordLength = 256 & chunk size 16. produces a 16x16 array of random passwords.

dc97cf1446f444be
87c8317988a82970
8a39de9b17d4261c
bd0590ec321a63a9
c364b91598daa003
52e03dddab13f9a1
f4685739d2b2fe94
eef1cd9b14b379fd
f2f3b29e8c9ed0ba
def6b4bd66ba1c07
931d1e29268fa2c2
d7d3802effc2da19
26332c1388125e3c
1d81cbbc2b439577
1a1ff3b24c6d64fa
88d84617ccc0a434
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Have you read the canonical Community Wiki question/answer? Although it's in Python, there's a lot of transferrable advice in there. \$\endgroup\$ Commented Oct 14 at 7:18
  • 3
    \$\begingroup\$ From security perspective you should use System.Security.Cryptography.RandomNumberGenerator for password generation rather than System.Random. Later this week I could provide a sample how to rewrite to it to make it more secure and performant. \$\endgroup\$ Commented Oct 14 at 10:32

1 Answer 1

Reset to default
4
\$\begingroup\$

  • The public interface of this password generator is bizarre. It exposes three different ways to return passwords that are not obvious in usage:

    1. You can enumerate the PasswordGenerator itself to get a passwordLength number of chars. Why should someone do this instead of just getting a password string through the Result property?
    2. The Result property magically returns a different password every time you read it. This is usually not expected from properties but especially not from a property named Result. "Result" of what? A better name would be Next or NextPassword. But a method would be more appropriate for this use case (e.g. named Create or Generate or Next).
    3. The most surprising thing, however, is the GeneratePasswords method. It is static, but you must pass it a PasswordGenerator as argument. Why not make it an instance method then? Also, I would never guess that the size parameter would take chunks of this size from a passwordLength password. E.g., if you initialize passwordLength with 16, then Result returns a password of this size, but if you call GeneratePasswords with an argument of 5, it returns 3 passwords of length 5 and one with length 1. This does not make any sense.

    Keep your public interface as simple as possible and use the Principle of least astonishment. It could be as simple as:

    public class PasswordGenerator(int passwordLength, char[] legalChars)
{
    /// <summary>
    /// Generates a new password at each call.
    /// </summary>
    public string Create()
    {
        ...
    }

    /// <summary>
    /// Generates 'count' passwords of length 'passwordLength'.
    /// </summary>
    public IEnumerable<string> Enumerate(int count)
    {
        ...
    }
}

  • As Peter Csala already mentioned: use a cryptographic random number generator for more secure passwords.

\$\endgroup\$
6
  • \$\begingroup\$ The PasswordLength is what is used for the enumeration, that makes the output user defined. This is intended to be done all within the enumeration. As your code suggests you would need to do PasswordGenerator gen = new(legalchars); var result = gen.Create(len). my approach PasswordGenerator gen = new(len, legalchars); var result = gen.Result which creates the password automatically within, each call to Result will be different without allowing length to be changed.. the static method you refer to breaks result into chunks of specified size. the best approach is to use sizes / 8. \$\endgroup\$ Commented Oct 14 at 14:15
  • \$\begingroup\$ Example PasswordLength = 2048, chunk size 32 = 64 passwords, 32 character size. if we leave chunk size as 16 with 2048 PasswordLength we get 128 passwords of size 16 \$\endgroup\$ Commented Oct 14 at 14:20
  • \$\begingroup\$ what do you mean by 3 different ways to generate passwords? it provides 1 and 1 only a password of size PasswordLength where each character is taken randomly from the user supplied legal character set. the static method allows that 1 password to be split into many. A test performed generating 2 million passwords never repeated the same result on the 16x16 \$\endgroup\$ Commented Oct 14 at 14:27
  • \$\begingroup\$ I will change the name of result To NextPassword() \$\endgroup\$ Commented Oct 14 at 14:40
  • 1
    \$\begingroup\$ It would be much easier to understand, if you just specified the number of passwords to be returned. They would all have a length of passwordLength (and not a fraction of it). I don't see the point of cutting a long password into smaller pieces (this is an implementation detail which should not be public). By "3 different ways to generate" I am not speaking of the implementation. I am speaking of the public interface. You have 3 different ways of getting passwords. The problem of non cryptographic RNGs is not "repeating the same result", but they might generate a predictable sequence. \$\endgroup\$ Commented Oct 14 at 14:47

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.