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

Add PasswordUtil.GeneratePassword #1469

Closed
wants to merge 1 commit into from

Conversation

bgrainger
Copy link
Contributor

@bgrainger bgrainger commented Dec 19, 2023

This method generates a random password that is guaranteed to contain a mix of character types, and does not need to be escaped if used in a connection string.

Based on a comment here: https://github.com/dotnet/aspire/pull/1295/files#r1428294263

The algorithm is designed to generate a password that meets SQL Server complexity requirements (e.g., to avoid the following error message):

The password does not meet SQL Server password policy requirements because it is not complex enough. The password must be at least 8 characters long and contain characters from three of the following four sets: Uppercase letters, Lowercase letters, Base 10 digits, and Symbols.

Microsoft Reviewers: Open in CodeFlow

This method generates a random password that is guaranteed to contain a mix of character types, and does not need to be escaped if used in a connection string.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Dec 19, 2023
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 19, 2023
if (length >= 2)
{
// add a lowercase ASCII letter
Random.Shared.GetItems(PasswordChars.Slice(26, 26), buffer[1..2]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect range?

/// <remarks>If <paramref name="length"/> is greater than or equal to four, the password is guaranteed to contain an uppercase ASCII letter, a lowercase ASCII letter, a digit, and a symbol.</remarks>
internal static string GeneratePassword(int length = 20)
{
return string.Create(length, 0, (buffer, _) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this password generation might be a problematic. Imagine that we set a password length of 6. We would already know that the first character is going to be in the range A-Z, second a-z, third 0-9, and forth one of the specials. Obviously, that becomes less of an issue the longer you get.

/// Returns a random password of length <paramref name="length"/> that does not contain any characters that would be escaped by <see cref="EscapePassword(string)"/>.
/// </summary>
/// <remarks>If <paramref name="length"/> is greater than or equal to four, the password is guaranteed to contain an uppercase ASCII letter, a lowercase ASCII letter, a digit, and a symbol.</remarks>
internal static string GeneratePassword(int length = 20)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How cryptographically secure is this implementation? Writing a good password generator is probably more complicated than writing own cache.
There are quite a few related Qs on the web, e.g., https://stackoverflow.com/questions/54991/generating-random-passwords.

Also, having this implementation static essentially forces it for everyone. Shouldn't it be instead be injectable and replaceable?

@danmoseley
Copy link
Member

danmoseley commented Dec 20, 2023

This is not going to generate a secure password (eg it's using pseudo random generator). If it's not secure, why not simply use "password" or some other fixed value?

@GrabYourPitchforks

@mitchdenny
Copy link
Member

We should keep in mind that our current password generation is also not secure. I raised this internally and it was suggested that we do something like:

byte[] key256 = RandomNumberGenerator.GetBytes(32);
return Convert.ToBase64String(key256);

At long enough lengths that would likely be sufficient for generating a password with enough entropy. We would need to make sure that we observe the other password requirements such as special characters, upper, lowercase etc.

@bgrainger
Copy link
Contributor Author

If it's not secure, why not simply use "password" or some other fixed value?

I assumed (perhaps incorrectly) that this was just intended to generate a pseudorandom password for testing that would stop people hard-coding a fixed default. If the actual intent of this method is to generate a production-ready password that would be stored in Azure Key Vault or similar, then this approach is obviously inadequate.

@bgrainger bgrainger closed this Dec 20, 2023
@mitchdenny
Copy link
Member

Its not for production generation, deployment tools will do that. But if we are going to take a step forward its worth addressing a few of these issues :)

@danmoseley
Copy link
Member

I assumed (perhaps incorrectly) that this was just intended to generate a pseudorandom password for testing that would stop people hard-coding a fixed default. If the actual intent of this method is to generate a production-ready password that would be stored in Azure Key Vault or similar, then this approach is obviously inadequate.

Thanks. One other thing to bear in mind is that we have code scanners for things that look like passwords and tokens. To avoid triggering that we generally use the text "placeholder" which makes it clear it's not real (I don't have full context here.)

@github-actions github-actions bot locked and limited conversation to collaborators Apr 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants