Azure / azure-sdk-tools

Tools repository leveraged by the Azure SDK team.
MIT License
114 stars 180 forks source link

Feature request: get/set environment file contents #728

Closed drwill-ms closed 1 year ago

drwill-ms commented 4 years ago

I have several environments I may test against (PPE, production instances), and it would be nice to print out the contents of a file to be sure which one I have. I also want to as hand build one based on a teammate's instance.

I'd like the ability to print out the contents of an environment file, to confirm which one I'm using. I may need to permanently change one of the variables in a file. I'd also like to build a file by hand and then encrypt it so it can be read by the test system.

For now, I'm renaming files to switch environments. It would be nice to have them keep descriptive names, and change which one is active somehow.

pakrym commented 4 years ago

cc @weshaggard @heaths

heaths commented 4 years ago

Should we allow passing in an optional description of the environment (or however people would like to use such a property) to "stamp" into the files?

Note also that the logic to encrypt these only works on Windows, and currently is only for the .NET repo. If this expands to other platforms and/or language repos, we may have to think about how to abstract encryption and decryption.

drwill-ms commented 4 years ago

Well, if I have several files on disk, they have to differ by file name, so if I could simply point to the file I wanted to use rather than have it use one with a specific file name, that would be better.

If I can't do that, then the other env contents are enough for me to distinguish.

pakrym commented 4 years ago

Idea! What if we just supported reading non-encoded files? We won't create them in the script but would still read if one exists.

Would be a 4 line change in azure-sdk-for-net.

heaths commented 4 years ago

We won't create them in the script but would still read if one exists.

Could you clarify what you mean? I don't follow. Are you suggesting we don't encrypt them? Given the environment variables we can also produce aren't, that's probably fine since those files are still in .gitignore (*.env, IIRC).

pakrym commented 4 years ago

No, the script would produce an encoded file and print it's content at the time of creation.

We would allow developers to manually replace the file content with an unencoded version if they need to make changes (by overwriting an encoded binary file with json text file of their choosing)

heaths commented 4 years ago

It already prints the unencoded file contents. What's different now? I don't think we should write a bunch of scripts to manage the file system. I appreciate the request, but how often will people use it compared to maintenance costs? Renaming files for different environments seems like a suitable and relatively easy workaround.

weshaggard commented 4 years ago

I'm inclined to agree with @heaths that I'm not sure we should add extra scripts for this unless there becomes a strong reason. If you are working in PS you an use something like https://www.powershellgallery.com/packages/ProtectedData/4.1.3 or https://gist.github.com/atifaziz/10cb04301383972a634d0199e451b096 to dump the file to see the contents.

pakrym commented 4 years ago

What do you all think about https://github.com/Azure/azure-sdk-for-net/pull/13013? Allows to manually author a file if needed.

weshaggard commented 4 years ago

I don't like us having files with plain text secrets on our disk and in our repo even if they are in the gitignore file. I think the risk of leaking those secrets is too high.

pakrym commented 4 years ago

Sounds good

pakrym commented 4 years ago

Here's the snippet to read the env file contents using powershell:

([System.Text.Encoding]::UTF8).GetString([Security.Cryptography.ProtectedData]::Unprotect([IO.File]::ReadAllBytes((Resolve-path "test-resources.json.env")), $null, [Security.Cryptography.DataProtectionScope]::CurrentUser))
kasobol-msft commented 3 years ago

Storage team is using custom configuration file to run tests, see here and here.

We can't easily migrate to use TestEnvironment because we need a human friendly way of manually editing these configs (e.g. in Java it's based on env variables which are horrible to maintain). The reason behind this is that we often use storage accounts from pre-prod or even more unstable environments for local development where there isn't a way of automating access to these accounts. Hence manual edits.

In order for us to fully onboard TestEnvironment we'd need a way to feed it from local file that's easily editable by human. That's the only thing that blocks us from dropping custom configuration infra in storage.

heaths commented 3 years ago

The TestEnvironment class checks both the .env file (if it exists) and environment variables, so you could opt to leave variables out of the .env file and only set environment variables. In fact, according to hhttps://github.com/Azure/azure-sdk-for-net/blob/c9eb735fcebe56c948d9c6804e4b99f53636ebf1/sdk/core/Azure.Core.TestFramework/src/TestEnvironment.cs#L300-L328 environment variables are read first anyway. Based on that implementation, the order goes:

  1. prefixed name from environment (where the prefix is what your derivative class defines)
  2. prefixed name from .env file
  3. non-prefixed name from environment
  4. "AZURE_"-prefixed name from environment
  5. non-prefixed name from .env file

So you could still define some env vars to override the file if you prefer. It's also possible to run the New-TestResources.ps1 script against a sub and environment that is non-public and have those passed into the ARM template, but my experience with this is limited. It may not be worth the hassle if you're already used to managing env vars.

kasobol-msft commented 3 years ago

@heaths we don't use env variables in local .NET development and we don't want to, that'd be a big downgrade in our local dev exp.

The exact request here is to make ".env" files human friendly, i.e. don't assume they're protected (see here). The ability to edit them by a human is a requirement for my team.

As I mentioned above. We can't rely on ARM templates or any of the automation around New-TestResources.ps1 at early stages of feature development. Often the features we work on lack ARM support at that stage or that kind of automation is simply impossible to achieve in environments we want to target.

heaths commented 3 years ago

The point of encrypting the files - and why we only do them on Windows currently thanks to DPAPI - is in case anyone forcibly adds them to the git index and commits+pushes them. We are trying to mitigate leaking of secrets. As Pavel mentioned above, you could DPAPI-decrypt them, edit, and re-encrypt them.

kasobol-msft commented 3 years ago

Then consider adding "local.file.json.env" that's also queried by TestEnv and is explicitly added to .gitignore like we do with https://github.com/Azure/azure-sdk-for-net/blob/c9eb735fcebe56c948d9c6804e4b99f53636ebf1/.gitignore#L128 .

decrypting, editing, re-encryting is a downgrade for us. We'll stick to our test config files until that's resolved.

pakrym commented 3 years ago

All .json.env files are already in the gitignore.

I don't think the problem with accidentally checking the .env file is real, to be honest. It hasn't ever happened and if we make the non-encrypted feature opt-in (New-TestResource -Encrypt $false) I don't think we'll have resources leaked this way. In addition, the credscan that would soon be enabled would catch it real quick.

weshaggard commented 3 years ago

I don't think the problem with accidentally checking the .env file is real, to be honest. It hasn't ever happened

It is definitely real and has happened more then once across our team. Adding to .gitignore does help mitigate the problem somewhat but it only helps with committing the secrets, we also have to be careful about having secrets in unencrypted files on our own disk. Persisted secrets like these are a gold mine for attackers whenever a machine gets compromised.

@kasobol-msft how much of these values are actual configuration vs secrets? Perhaps we can come up with some model that separates them in some way.

pakrym commented 3 years ago

Persisted secrets like these are a gold mine for attackers whenever a machine gets compromised.

How are env vars different in this case?

weshaggard commented 3 years ago

Env vars are more short-lived and go away when the process exits.

pakrym commented 3 years ago

Most people I've seen using env vars for tests set AZURE_CLIENT_SECRET on a machine level.

kasobol-msft commented 3 years ago

@weshaggard see https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/storage/Azure.Storage.Common/tests/Shared/TestConfigurationsTemplate.xml . There's plenty of connection strings.

Btw. In Java all this is based on env variables which is kind of painful to maintain and I was thinking about writing plugin for IntelliJ to manage this better. Maybe VS plugin would be an alternative to pursue, i.e. orchestrate all these low level commands we'd have to do to keep data secure at rest.

weshaggard commented 3 years ago

Setting secrets in machine/system wide environment variables is also not a good idea and I agree is no different then having them unencrypted on the file system. We should try to avoid both of those.

@weshaggard see https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/storage/Azure.Storage.Common/tests/Shared/TestConfigurationsTemplate.xml . There's plenty of connection strings.

That is a lot of boiler plate with potential secrets in them and if you had to maintain every value as an env variable I agree that would be painful. Is there a good way to refactor to scope only the secrets as env variables and then use your template config or test code itself to produce all the connections strings on the fly from a limited number of secrets?

kasobol-msft commented 3 years ago

This size has to be steady but will grow (as we add account level features). We tried minimizing that in Java and ended up with this. .NET config isn't much different. Problem is that many features are configurable at Storage Account level, so either we have sparse config or we have to switch between feature sets constantly (the later would be real pain locally and not achievable in pipelines really).

weshaggard commented 3 years ago

@kasobol-msft I don't have any great solution for your problem but it might be worth setting up a meeting with myself and @benbp who is working on test configuration at a higher level to see what options we might be able to come up with.

kurtzeborn commented 1 year ago

Closing. If we ever really want to do something here, we'd rather put these env vars in KeyVault rather than in a file that's checked in somewhere.

heaths commented 1 year ago

Closing. If we ever really want to do something here, we'd rather put these env vars in KeyVault rather than in a file that's checked in somewhere.

@kurtzeborn the file wouldn't be checked in. That'd be bad. We use - and gitignore - *.env files that are also encrypted via DPAPI for Windows, with no support at all in other platforms (but we have ideas). The idea was to extend this concept to other languages and platforms for the inner dev loop.