Add Password To Redis

[Alirexaa]

close: #3838

Microsoft Reviewers: Open in CodeFlow

[mitchdenny]

I'm comfortable with this change pending @eerhardt 's comments about the API txt files and some incremental improvements to the XML docs since we are in the neighbhorhood.

I'm a little disappointed that about the parameter ordering but I can't think of a way that isn't a binary/source breaking change that isn't also disruptive.

One more iteration and we should be able to stamp an approval on this. Thanks once again for your efforts @Alirexaa.

[radical]

Some of my comments might be non-sensical, or could be addressed in a follow-up PR. I'll let others decide that, since I'm very new to this code:)

[mgravell]

@eerhardt We've discussed this for the client a few times but it has never hit the top of the pile. If it is a blocker, we can prioritize. I would suggest looking at ado-net quoted values as the template, but obvs we'd need to think a little about how to do it without impacting existing use of any new token

[eerhardt]

@eerhardt We've discussed this for the client a few times but it has never hit the top of the pile. If it is a blocker, we can prioritize. I would suggest looking at ado-net quoted values as the template, but obvs we'd need to think a little about how to do it without impacting existing use of any new token

I don't think it is blocking this work. I just wanted to point out that we can't URL encode the connection string here.

[Alirexaa]

What is the status of PR?

Currntly i'm working on #5165, I need playground app there, Besides that, if redis have password, i need to support password there.

@eerhardt @mitchdenny

[mitchdenny]

@Alirexaa see: https://github.com/dotnet/aspire/pull/4642#discussion_r1669073634

[mitchdenny]

I've been talking a closer look at this from a security perspective, and I think we need to make some changes. When I run this locally the args are visble in the dashboard without any obfuscation (unlike environment variables).

I'm looking at the implications for deployment now too.

OK, so deployment doesn't use the args which means that the password isn't required. So we are going to need to fix that in azd before this change would have any impact (it has a local impact obviously but we need to think through to deployment for this one).

[eerhardt]

I've been talking a closer look at this from a security perspective, and I think we need to make some changes. When I run this locally the args are visble in the dashboard without any obfuscation (unlike environment variables).

I'm looking at the implications for deployment now too.

OK, so deployment doesn't use the args which means that the password isn't required. So we are going to need to fix that in azd before this change would have any impact (it has a local impact obviously but we need to think through to deployment for this one).

Good catch.

Reading https://redis.io/docs/latest/operate/oss_and_stack/management/security/#authentication, the other options for enabling a password are:

1. Adding a user and password to the Access Control List
2. Use the requirepass setting in the redis.conf file.

Either approach would require being able to write a file that is mounted into the container. Is that possible during deployment? @ellismg @vhvb1989

Alternatively, we could modify the Access Control List by connecting to the redis container after it is running, and issuing commands to it. I don't believe this is possible during deployment.

[eerhardt]

When I run this locally the args are visble in the dashboard without any obfuscation (unlike environment variables).

@drewnoakes @adamint - is it possible to mark command line arguments as "secret"? So they get obfuscated in the dashboard.

[vhvb1989]

Either approach would require being able to write a file that is mounted into the container. Is that possible during deployment?

That's bindMounts, isn't it? Like the sample from MySql here

The only difference would be that the file that is uploaded is generated, instead of committing it to the repo (or it can be a file in the repo with references to secured-args)

We support bind mount with an alpha feature in azd. During provision, azd creates an storage account and one file-share which is set up as volume in the Azure Container Environment. After provision, azd uploads the local content to the file-share. At the end, azd deploys the Azure Container App using the volume.

The azd's alpha feature is turned on with azd config set alpha.azd.operations on

If you run azd infra synth in a project using container.v0+bindMounts, you will see a new file in the infra folder with the name azd.operations.json, which describes the upload-files operation from a local folder (defined by the AppHost)

[drewnoakes]

@drewnoakes @adamint - is it possible to mark command line arguments as "secret"? So they get obfuscated in the dashboard.

#5065 tracks marking resource properties as secrets, however that has to be specified by the resource service. We could hard-code some known properties as sensitive. If we did that, which would you think should be included? The current list is https://github.com/dotnet/aspire/blob/0f2931a94873d09e5a76f72485000e5388bf08dd/src/Shared/Model/KnownProperties.cs

[mitchdenny]

As much as I would like this in for 8.2, I think we need to hold this PR until we get this resolved. I think hiding the args in the dashboard is fine for local given that if you have access to the dashboard you could uncover the secrets in environment variables anyway.

I'm more concerned about deployment where people will have a reasonable expectation that the if you specify the password that it'll be used when deployed to the cloud.

Frustratingly Redis Stack actually makes use of a REDIS_ARGS environment variable which can use to specify the -requirepass [password] argument.

[eerhardt]

Frustratingly Redis Stack actually makes use of a REDIS_ARGS environment variable which can use to specify the -requirepass [password] argument.

Yeah, I was wondering if it would make sense for us to switch from library/redis to redis/redis-stack-server instead. And then Add WithRedisInsight (dotnet/aspire#5227) would switch the image from redis/redis-stack-server to redis/redis-stack and add the appropriate endpoints.

[drewnoakes]

@drewnoakes @adamint - is it possible to mark command line arguments as "secret"? So they get obfuscated in the dashboard.

#5065 tracks marking resource properties as secrets, however that has to be specified by the resource service. We could hard-code some known properties as sensitive. If we did that, which would you think should be included? The current list is https://github.com/dotnet/aspire/blob/0f2931a94873d09e5a76f72485000e5388bf08dd/src/Shared/Model/KnownProperties.cs

This is in review in https://github.com/dotnet/aspire/pull/5380

[mitchdenny]

Yeah, I was wondering if it would make sense for us to switch from library/redis to redis/redis-stack-server instead. And then Add WithRedisInsight (dotnet/aspire#5227) would switch the image from redis/redis-stack-server to redis/redis-stack and add the appropriate endpoints.

I think this is an option we should seriously consider. I think we would just need to look through where there are any licensing implications for folks deploying that image into production. There might be extra licensing requirements that we wouldn't want to silently swap people into.

[eerhardt]

I've revived this PR with the latest code in main. The intent is to get this change into 9.1. The initial concerns that blocked this change should no longer be a problem. The command line args in the dashboard are hidden/masked by default now.

One complication is that Redis Insight needs to be initialized before we add a database with a password. I'll add a comment to the code to start a conversation about it there.

[mgravell]

Question: is it possible to specify the user for Redis? Or is that unnecessary in this scenario, with Aspire just using the default user? Similarly, there is increasing support for azure etc identity tokens for auth.

[eerhardt]

is it possible to specify the user for Redis? Or is that unnecessary in this scenario, with Aspire just using the default user?

Right now, no. We are just adding password protection for now. If necessary, we can also add support for user in the future.

Similarly, there is increasing support for azure etc identity tokens for auth.

We support Azure Redis and Entra ID already in 9.0. See https://learn.microsoft.com/dotnet/aspire/caching/stackexchange-redis-integration#add-azure-cache-for-redis-client

[eerhardt]

Do we mark this as a breaking change?

I don't think this is a breaking change. We can log it in the changelog, but I'm not sure exactly what it would break. The only thing I can think of is if someone is explicitly cutting the password out of the connection string getting flown to the app (or passing the connection string a custom way - not using our Hosting ConnectionString).

[eerhardt]

Thank you, @Alirexaa for your continued contributions!