aws / aws-aspnet-cognito-identity-provider

ASP.NET Core Identity Provider for Amazon Cognito
https://aws.amazon.com/developer/language/net/
Apache License 2.0
213 stars 89 forks source link

Security: Defaults require secrets be published in unencrypted configuration files. #196

Closed duaneking closed 3 years ago

duaneking commented 3 years ago

Description

Secrets can be leaked if checked into source control. Most companies use some form of CI/CD to deploy. As a result, storing secrets unencrypted at rest in the way the readme suggests is an insecure antipattern by default. The readme explicitly states that these secretes should be stored at rest unencrypted. This is a security antipattern.

Reproduction Steps

See readme's explicit instructions to use config values for cognito instances in unencrypted at rest config files.

Logs

See

Environment

All.

Resolution


This is a :bug: bug-report

ashishdhingra commented 3 years ago

Hi @duaneking,

Good morning.

I agree that storing secrets as defaults in appsettings.json should not be checked-in to source control. This is true for any application (for example, using Entity Framework connection string for accessing SQL Server). For AWS environments, you could use AWS Secrets Manager or AWS Systems Manager.

Following articles might be helpful for your scenario:

Hope this helps.

Thanks, Ashish

duaneking commented 3 years ago
  1. Not everybody wants to use AWS secrets manager due to its known issues, but the people who do would still need a working example in the sample app that does not exist today to guide them. Also that page lists no support for .Net and although you can see it in one image, its not called out as supported and the way its not presented as the others do makes customers feel like its not ready for prime time in C#, for good or bad.

  2. Not everybody wants to use AWS Systems manager due to its known issues and its hidden costs, but the people who do would still need a working example in the sample app that does not exist today to guide them.

  3. That blog post does not give examples or provide a lot of direction; In fact if you actually read the comments by users on that page by clicking the button that hides them by default instead of taking ownership and being customer focused by making them public by default, you will find a lot of frustrated and angry users complaining about how useless and terrible that post and its samples actually are. I can only assume you are unaware of this, so I mention it in an attempt to be helpful.

ashishdhingra commented 3 years ago
  1. Not everybody wants to use AWS secrets manager due to its known issues, but the people who do would still need a working example in the sample app that does not exist today to guide them. Also that page lists no support for .Net and although you can see it in one image, its not called out as supported and the way its not presented as the others do makes customers feel like its not ready for prime time in C#, for good or bad.
  2. Not everybody wants to use AWS Systems manager due to its known issues and its hidden costs, but the people who do would still need a working example in the sample app that does not exist today to guide them.
  3. That blog post does not give examples or provide a lot of direction; In fact if you actually read the comments by users on that page by clicking the button that hides them by default instead of taking ownership and being customer focused by making them public by default, you will find a lot of frustrated and angry users complaining about how useless and terrible that post and its samples actually are. I can only assume you are unaware of this, so I mention it in an attempt to be helpful.

Hi @duaneking,

Kindly advise what is required here. The Readme specifies the guidance on how to use the ASP.NET Cognito Identity Provider package:

"AWS": {
    "Region": "<your region id goes here>",
    "UserPoolClientId": "<your user pool client id goes here>",
    "UserPoolClientSecret": "<your user pool client secret goes here>",
    "UserPoolId": "<your user pool id goes here>"
}

Thanks, Ashish

duaneking commented 3 years ago

You do not understand.

These types inject interfaces into Identity code but still require downcasting to non-standard types to actually use. Think about how terrible this is, and what that represents as a level of quality for the code and the people who have to maintain code that uses it.

What I'm advocating for is that the team take ownership, and be customer focused, and provide better examples, and then explicitly call out the trade offs they have chosen as well as the reasons for them.

I'm directly calling out the poor docs. Yes, you and I may understand that what it mans there is to search for and figure out what the constructor accepts and then through trial and error instantiate it ourselves, but maybe a person who wants to use it has less experience and needs a direct example of the constructer they need to use? Maybe they don't understand the complex object model of interfaces involved and are looking at the docs for a reason?

Because the absence of this is what is known as a code smell.

normj commented 3 years ago

I have made some updates to the README file with better guidance of how to configure the user pool and distinguish what you would do for development prototyping versus a production user pool.

github-actions[bot] commented 3 years ago

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.