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

Use cryptographically generated passwords #2210

Merged
merged 9 commits into from
Feb 14, 2024
Merged

Conversation

sebastienros
Copy link
Member

@sebastienros sebastienros commented Feb 13, 2024

Seeing an attempt to generate random passwords and other discussions about the recommended approach I am providing what I think is a better approach.

This could also be used to generate random ids but it's not part of this PR.

Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Feb 13, 2024
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Would be good to get @mitchdenny to review.

[InlineData(10)]
public void IncludesLowerCase(int length)
{
var password = PasswordGenerator.GeneratePassword(length, 0, 0, 0);
Copy link
Member

Choose a reason for hiding this comment

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

This is not length, this is the # of lower case chars.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see what you are demonstrating. :)

@mitchdenny
Copy link
Member

mitchdenny commented Feb 14, 2024

UPDATE: Ignore this comment - but leaving for transparency :)

It is definitely better than what we have today :)

One thing that I thought of and I don't know if it is really a practical issue for us given the ephemeral nature of the passwords we are generating, is that this code would always seem to be in the form:

[lower case chars set][upper case chars set][digit chars set][special chars set]

This reduces the possible set of characters for each individual character position which reduces the possible combination of passwords. To give a concrete example, here is the dictionary of characters we have by category:

Set Characters Quantity
Lowercase abcdefghjkmnpqrstuvwxyz 23
Uppercase ABCDEFGHJKMNPQRSTUVWXYZ 23
Digits 0123456789 10
Special chars -_#@!./:[]{}+*()~ 17

So, if we call the following code (like we do in SQL Server):

GeneratePassword(6, 6, 2, 2)

The number of combinations is:

23 * 23 * 23 * 23 * 23 * 23 * 23 * 23 * 23 * 23 * 23 * 23 * 10 * 10 * 17 * 17 = 6.33332646085387E+20

Compare this with a GUID which is 1.208925819614629174706176e+24

If we instead allowed the characters to be in any position and the options on the password meant the minimum number of characters of a given set and didn't correlate to password length or where they were found.

@eerhardt
Copy link
Member

If we instead allowed the characters to be in any position

They get shuffled at the end. So the characters can be in any position.

@mitchdenny
Copy link
Member

Ah i missed that ignore everything I said.

@sebastienros sebastienros merged commit 3279939 into main Feb 14, 2024
8 checks passed
@sebastienros sebastienros deleted the sebros/password branch February 14, 2024 22:51
@github-actions github-actions bot locked and limited conversation to collaborators Apr 21, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants