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

Prevent copy/paste of password and warn #44761

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

Rick-Anderson
Copy link
Contributor

@Rick-Anderson Rick-Anderson commented Nov 8, 2024

This falls under the company wide SFI (Secure Future Initiative)
Warn about password risk. Make it more difficult to copy/paste a password in config files and command lines.

@marcpopMSFT @sayedihashimi please recommend a reviewer

Contributes to dotnet/AspNetCore.Docs#33861

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-WebSDK untriaged Request triage from a team member labels Nov 8, 2024
@sayedihashimi
Copy link
Member

sayedihashimi commented Nov 9, 2024

I'm not sure who is best to review, but I agree we should see if we can improve this from an SFI perspective. @bradygaster is deeply involved in that so he may know a good reviewer.

Also adding @vijayrkn.

@KalleOlaviNiemitalo
Copy link

This uses all of <Deploy-/p:Password>, </$Credential>, and </$;Credential;> in the same file. Is there some difference in semantics? All of these syntaxes look contorted.

@baronfel
Copy link
Member

baronfel commented Nov 9, 2024

I'd strongly consider changing the doc to some mechanism that does not ingest secrets of any kind into MSBuild via Properties - once something is an MSBuild property it is visible to all Task and MSBuild logic that runs in that same build, so unless you are strictly auditing the build logic you use (including build logic from NuGet packages, etc) you're exposing yourself.

@KalleOlaviNiemitalo

This comment has been minimized.

@baronfel
Copy link
Member

baronfel commented Nov 9, 2024

Yes, good spot @KalleOlaviNiemitalo. Edited for clarity.

@KalleOlaviNiemitalo
Copy link

Could MSBuild detect Common annotated security keys and exclude them from logs?

@KalleOlaviNiemitalo
Copy link

Other mitigations for production environments:

  • Use MSBuild to create artifacts, but deploy them as a separate non-MSBuild step that has fewer dependencies and is easier to audit.
  • Short-lived deployment keys. Needs support at the server, and a separate root of trust whose secrets are not exposed to the project.

@Rick-Anderson
Copy link
Contributor Author

Rick-Anderson commented Nov 10, 2024

This uses all of <Deploy-/p:Password>, </$Credential>, and </$;Credential;> in the same file. Is there some difference in semantics? All of these syntaxes look contorted.

<Deploy-/p:Password> is used on the command line and /p is the command switch so should throw an error if copy/pasted.
</$Credential> is for the app pw, </$;Credential;> is for the connection string. I didn't want to use the same stub for both pw's, I don't want to encourage using the same pw for deployment and the connection string. I can combine those if you like.

EDIT

Changed </$;Credential;> to </$DB_Credential>

To make it harder to global copy/paste and use the same PW for two different services.

Comment on lines +22 to +26
For production deployments:

* Use MSBuild to create artifacts, but without deployment, so no credentials are required. Deploy apps as a separate non-MSBuild step that has fewer dependencies and is easier to audit.
* Use deployment keys with short expiration times. A server in a separate root of trust is used to manage the deployment keys. Secrets aren't exposed to the project, ensuring that even if the project is compromised, the root of trust remains secure.

Copy link
Contributor Author

@Rick-Anderson Rick-Anderson Nov 10, 2024

Choose a reason for hiding this comment

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

@KalleOlaviNiemitalo please review and use the suggestion feature to submit an improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-WebSDK untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants