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

Why is hosting environment default value "production"? #863

Closed
hlubovac opened this Issue Oct 12, 2016 · 42 comments

Comments

Projects
None yet
5 participants
@hlubovac

hlubovac commented Oct 12, 2016

RE:

public string EnvironmentName { get; set; } = Hosting.EnvironmentName.Production;

I'm curious if there is a reason for this. I tested launching the app without that setting in config, and - since the default is "production" - the danger is that a developer, intermittently, while having network access to production environment, may corrupt production data. IMO, there should not be default at all; if it has to be for some reason, then maybe "development".

Although this whole business of assuming certain business configuration is little unclean to me. We may have more than just the 3 environments, or we may refer to them using other names. Existing environment-related extension methods (IsDevelopment, etc) are just making this confusing. If you ask me, thanks for providing application-path information, but strongly-typing environment types just doesn't belong in this framework. Sorry if I sound uninformed; I based this comment by what's visible to me.

Thanks.

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 13, 2016

I guess I discovered a way to change that behavior, thus making inadvertent mistakes like I mentioned more obvious. So, sharing with those sharing the same concern:

var defaults = new Dictionary<string, string> { { WebHostDefaults.EnvironmentKey, "Development" } }; var configBuilder = new ConfigurationBuilder().AddInMemoryCollection(defaults) ...

hlubovac commented Oct 13, 2016

I guess I discovered a way to change that behavior, thus making inadvertent mistakes like I mentioned more obvious. So, sharing with those sharing the same concern:

var defaults = new Dictionary<string, string> { { WebHostDefaults.EnvironmentKey, "Development" } }; var configBuilder = new ConfigurationBuilder().AddInMemoryCollection(defaults) ...

@natemcmaster

This comment has been minimized.

Show comment
Hide comment
@natemcmaster

natemcmaster Oct 13, 2016

Member

Here's more context: #272

Member

natemcmaster commented Oct 13, 2016

Here's more context: #272

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 13, 2016

Thanks - although, I didn't catch an explanation to why exactly it was changed to default to "production".

[In my app] I just wrote code to change the default to string.empty (can't be null, as that gets ignored internally; referring to the code snippet in my previous comment) and to raise an exception if the environment hasn't been specifically set either via env-var or main-args. I have something against black boxes silently falling back to some unobvious behavior: I'd rather have them insist on being explicitly told how to [behave]. But, that's just me :)

Thank you.

hlubovac commented Oct 13, 2016

Thanks - although, I didn't catch an explanation to why exactly it was changed to default to "production".

[In my app] I just wrote code to change the default to string.empty (can't be null, as that gets ignored internally; referring to the code snippet in my previous comment) and to raise an exception if the environment hasn't been specifically set either via env-var or main-args. I have something against black boxes silently falling back to some unobvious behavior: I'd rather have them insist on being explicitly told how to [behave]. But, that's just me :)

Thank you.

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 13, 2016

Not that I really want to beat this to death, but whatever happened to writing testable code and then making it difficult/impossible to swap the IHostingEnvironment instance here:

_hostingEnvironment = new HostingEnvironment();

Similar would go for var services = new ServiceCollection(); within the same class.

I'm getting discouraged to use the WebHostBuilder class a little.
:)

hlubovac commented Oct 13, 2016

Not that I really want to beat this to death, but whatever happened to writing testable code and then making it difficult/impossible to swap the IHostingEnvironment instance here:

_hostingEnvironment = new HostingEnvironment();

Similar would go for var services = new ServiceCollection(); within the same class.

I'm getting discouraged to use the WebHostBuilder class a little.
:)

@blowdart

This comment has been minimized.

Show comment
Hide comment
@blowdart

blowdart Oct 13, 2016

Member

It was changed to production to be secure by default. This won't change.

Member

blowdart commented Oct 13, 2016

It was changed to production to be secure by default. This won't change.

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 13, 2016

I don't quite get that explanation still. What and how is getting "secure" by that property not being NULL by default?

hlubovac commented Oct 13, 2016

I don't quite get that explanation still. What and how is getting "secure" by that property not being NULL by default?

@muratg muratg added this to the Discussions milestone Oct 13, 2016

@blowdart

This comment has been minimized.

Show comment
Hide comment
@blowdart

blowdart Oct 13, 2016

Member

I am a little confused, which property? Or are you meaning, if the env variable is not there when you say property?

Member

blowdart commented Oct 13, 2016

I am a little confused, which property? Or are you meaning, if the env variable is not there when you say property?

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 13, 2016

I used the word "property" to refer to the same thing you defined as "it" in your second to the last comment, sir.

To be clearer, that would be Microsoft.AspNetCore.Hosting.Internal.HostingEnvironment.EnvironmentName, which is pretty much the topic of this discussion (thank you @muratg).

hlubovac commented Oct 13, 2016

I used the word "property" to refer to the same thing you defined as "it" in your second to the last comment, sir.

To be clearer, that would be Microsoft.AspNetCore.Hosting.Internal.HostingEnvironment.EnvironmentName, which is pretty much the topic of this discussion (thank you @muratg).

@blowdart

This comment has been minimized.

Show comment
Hide comment
@blowdart

blowdart Oct 13, 2016

Member

So, the reason we went with production by default is because both our templates, and in general other people's code add detailed error messages, debug logging, and other things that can cause information disclosure vulnerabilities (at best), or things like auth bypass at worst, depending on what you wrapped in an env.IsDevelopment() check.

By defaulting to production we remove that risk.

In addition launching from inside VS and VS Code sets the environment as development, and best practice generally acknowledges developers should not have every day access to production assets, the risk of defaulting to development is far greater than defaulting to release/production.

Member

blowdart commented Oct 13, 2016

So, the reason we went with production by default is because both our templates, and in general other people's code add detailed error messages, debug logging, and other things that can cause information disclosure vulnerabilities (at best), or things like auth bypass at worst, depending on what you wrapped in an env.IsDevelopment() check.

By defaulting to production we remove that risk.

In addition launching from inside VS and VS Code sets the environment as development, and best practice generally acknowledges developers should not have every day access to production assets, the risk of defaulting to development is far greater than defaulting to release/production.

@muratg

This comment has been minimized.

Show comment
Hide comment
@muratg

muratg Oct 13, 2016

Member

@hlubovac, to clarify @blowdart's point - assume the default is "Development" and you debugging reasons you log/print your connection string. Now this is fine, but if you deploy to your production environment, and forget to set the environment to "Production", you'll start leaking your app's secrets inadvertently.

Re your concern about possibly corrupting your production data -- I VERY strongly recommend against having production secrets on your development boxes. Regardless of the default this is always a big risk!

Member

muratg commented Oct 13, 2016

@hlubovac, to clarify @blowdart's point - assume the default is "Development" and you debugging reasons you log/print your connection string. Now this is fine, but if you deploy to your production environment, and forget to set the environment to "Production", you'll start leaking your app's secrets inadvertently.

Re your concern about possibly corrupting your production data -- I VERY strongly recommend against having production secrets on your development boxes. Regardless of the default this is always a big risk!

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 13, 2016

I still think that it shouldn't the business of this "platform" (as you call it) or a starter-project template to even care which logic executes and when/where. We didn't have the notion of business-environments for 15 years, and nobody asked .NET team to enforce it in any way. By "solving" the problem of having the project template potentially "suggest" that, e.g. showing a developer-error-page isn't appropriate in all environments, the platform introduces even a bigger one: mistakenly executing code against the production resources (like I described above), and - no, that doesn't even necessarily has to mean that I'd have production-related secrets embedded into the source code either (I'll describe how in next paragraph). To expand on the enforced usage, now, that the 3 environments are "strong-typely" bound to launching http service (!?), people will be bound to actually use it, as it's very difficult to not be tempted, since it's impossible to exclude it - because Hosting assembly is somehow a dependency of Kestel (shouldn't it be the other way around, or we're still having problems naming "things" properly). Like I said, maybe the place I work at has been referring to what-you-call-production as "release" (or something else altogether) and, now that IsProduction is a part of the framework - unless I favor clutter - I might decide to force myself to use it, and - while I'm not realizing the consequences of commenting-out/removing that setting from launchSettings.json file, I might get myself fired. Finally, that "bigger problem introduced" relates to your template not being able to enforce that "don't embed production secrets in code", even when that's relevant.

And, here's the setup that I know of where no production secrets are embedded in the code, but it's still possible to do damage to production data (assuming developer's workstation has network access to it:

There's a company out there (and I'm sure it's only one of a kind) that likes to centralize database-connection-strings and host their delivery to applications behind a service. So, apps, at their startup, go "hey, conn-string service, gimme conn-string for [insert-environment-here]; I'm app [insert app id], here's my client cert as a proof, too" - and the service goes, "here you are". And then, I'm suddenly deleting customer's data by mistake (because I didn't realize I send the word "production" embedded into that get-conn-string request, if that's not clear by now).

I think your template should be just it. I think you should strive to convert your framework to a library and let me be in charge of what executes and when, as opposed to being called back. Let me use that dependency-injection and inject logging into Kestel if I want to (for example!), and not have you include it there anyway.

Sorry, this wasn't supposed to sound a like a rant. I'm trying to influence you to improve things and avoid creating complexity and confusion unnecessarily. :)

Thanks.

hlubovac commented Oct 13, 2016

I still think that it shouldn't the business of this "platform" (as you call it) or a starter-project template to even care which logic executes and when/where. We didn't have the notion of business-environments for 15 years, and nobody asked .NET team to enforce it in any way. By "solving" the problem of having the project template potentially "suggest" that, e.g. showing a developer-error-page isn't appropriate in all environments, the platform introduces even a bigger one: mistakenly executing code against the production resources (like I described above), and - no, that doesn't even necessarily has to mean that I'd have production-related secrets embedded into the source code either (I'll describe how in next paragraph). To expand on the enforced usage, now, that the 3 environments are "strong-typely" bound to launching http service (!?), people will be bound to actually use it, as it's very difficult to not be tempted, since it's impossible to exclude it - because Hosting assembly is somehow a dependency of Kestel (shouldn't it be the other way around, or we're still having problems naming "things" properly). Like I said, maybe the place I work at has been referring to what-you-call-production as "release" (or something else altogether) and, now that IsProduction is a part of the framework - unless I favor clutter - I might decide to force myself to use it, and - while I'm not realizing the consequences of commenting-out/removing that setting from launchSettings.json file, I might get myself fired. Finally, that "bigger problem introduced" relates to your template not being able to enforce that "don't embed production secrets in code", even when that's relevant.

And, here's the setup that I know of where no production secrets are embedded in the code, but it's still possible to do damage to production data (assuming developer's workstation has network access to it:

There's a company out there (and I'm sure it's only one of a kind) that likes to centralize database-connection-strings and host their delivery to applications behind a service. So, apps, at their startup, go "hey, conn-string service, gimme conn-string for [insert-environment-here]; I'm app [insert app id], here's my client cert as a proof, too" - and the service goes, "here you are". And then, I'm suddenly deleting customer's data by mistake (because I didn't realize I send the word "production" embedded into that get-conn-string request, if that's not clear by now).

I think your template should be just it. I think you should strive to convert your framework to a library and let me be in charge of what executes and when, as opposed to being called back. Let me use that dependency-injection and inject logging into Kestel if I want to (for example!), and not have you include it there anyway.

Sorry, this wasn't supposed to sound a like a rant. I'm trying to influence you to improve things and avoid creating complexity and confusion unnecessarily. :)

Thanks.

@blowdart

This comment has been minimized.

Show comment
Hide comment
@blowdart

blowdart Oct 13, 2016

Member

VS has had the concept of different environments, via config transform since 2008.

Member

blowdart commented Oct 13, 2016

VS has had the concept of different environments, via config transform since 2008.

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 13, 2016

I have yet to meet a serious development team who's using VS's "publish" feature to deploy to production (or, anywhere, for that matter).

hlubovac commented Oct 13, 2016

I have yet to meet a serious development team who's using VS's "publish" feature to deploy to production (or, anywhere, for that matter).

@blowdart

This comment has been minimized.

Show comment
Hide comment
@blowdart

blowdart Oct 13, 2016

Member

Indeed, you'd use CI instead. And that would hold the production secrets, rather than them being on a developers machine.

Member

blowdart commented Oct 13, 2016

Indeed, you'd use CI instead. And that would hold the production secrets, rather than them being on a developers machine.

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 13, 2016

In the scenario that I described, there are no secrets on the developer's machine. All secrets are behind the service that delivers secrets driven by the value of "EnvironmentName". Perhaps it's a WTF not to host a separate secret-keeper service per business environment, but it's hypothetically how the described problem might occur - and how your template can't do a thing about even warning about it.

Notice how I'm referring to the "environment" in subject as "business environment"? Because that's really what it is. "Environment" by itself hints relation to the operating system to me. A platform/library of pretty much any kind shouldn't force how I organize my "business".

Like I said, I have a work-around to what I consider a problem here. If you're curious, here it is:

`

        private readonly string[] ENVIRONMENTS = { "developer", "development", "test", "staging", "production" };

`

`

        var defaults = new Dictionary<string, string> { { WebHostDefaults.EnvironmentKey, string.Empty } };

        var configBuilder = new ConfigurationBuilder()
            .AddInMemoryCollection(defaults)
            .AddEnvironmentVariables("ASPNETCORE_"); // expected to provide a value for environment variable named "ASPNETCORE_ENVIRONMENT"

        var configuration = configBuilder.Build();

        string configDirectory = Directory.GetCurrentDirectory();

        // Mimicking the underlying framework to convert the IConfiguration 
        // object to a strongly-typed container of the environment, in order to 
        // ensure that that value has been explicitly set:
        var options = new WebHostOptions(configuration);

        if (options.Environment.Length == 0)
            throw new Exception("Environment name not provided. Please define environment variable \"ASPNETCORE_ENVIRONMENT\" for the system or the executing user.");
        if (!this.ENVIRONMENTS.Contains(options.Environment.ToLower()))
            throw new Exception("Environment not supported: \"" + options.Environment + "\".");

`

Of course, it's also accompanied with about 25 lines of comments, with links to relevant source code here. Perhaps, over time (I literally just started experimenting with dotnetcore 2 days ago), I'll figure out a more elegant solution to guarantee an explicitly defined business-environment name, but that's what I do now to prevent "Production" to sneak in on me.

hlubovac commented Oct 13, 2016

In the scenario that I described, there are no secrets on the developer's machine. All secrets are behind the service that delivers secrets driven by the value of "EnvironmentName". Perhaps it's a WTF not to host a separate secret-keeper service per business environment, but it's hypothetically how the described problem might occur - and how your template can't do a thing about even warning about it.

Notice how I'm referring to the "environment" in subject as "business environment"? Because that's really what it is. "Environment" by itself hints relation to the operating system to me. A platform/library of pretty much any kind shouldn't force how I organize my "business".

Like I said, I have a work-around to what I consider a problem here. If you're curious, here it is:

`

        private readonly string[] ENVIRONMENTS = { "developer", "development", "test", "staging", "production" };

`

`

        var defaults = new Dictionary<string, string> { { WebHostDefaults.EnvironmentKey, string.Empty } };

        var configBuilder = new ConfigurationBuilder()
            .AddInMemoryCollection(defaults)
            .AddEnvironmentVariables("ASPNETCORE_"); // expected to provide a value for environment variable named "ASPNETCORE_ENVIRONMENT"

        var configuration = configBuilder.Build();

        string configDirectory = Directory.GetCurrentDirectory();

        // Mimicking the underlying framework to convert the IConfiguration 
        // object to a strongly-typed container of the environment, in order to 
        // ensure that that value has been explicitly set:
        var options = new WebHostOptions(configuration);

        if (options.Environment.Length == 0)
            throw new Exception("Environment name not provided. Please define environment variable \"ASPNETCORE_ENVIRONMENT\" for the system or the executing user.");
        if (!this.ENVIRONMENTS.Contains(options.Environment.ToLower()))
            throw new Exception("Environment not supported: \"" + options.Environment + "\".");

`

Of course, it's also accompanied with about 25 lines of comments, with links to relevant source code here. Perhaps, over time (I literally just started experimenting with dotnetcore 2 days ago), I'll figure out a more elegant solution to guarantee an explicitly defined business-environment name, but that's what I do now to prevent "Production" to sneak in on me.

@muratg

This comment has been minimized.

Show comment
Hide comment
@muratg

muratg Oct 13, 2016

Member

@hlubovac In the scenario you described, you still have one production secret on the development machine: the secret to get other secrets from "secret-keeper" service 😄 That's is NOT secure by any means, and I recommend keeping development/test secrets and production secrets seperate with two different accounts/keys (whatever) on the "secret-keeper" service.

Back to the feature -- like you said you have a workaround for your own workload. You're not "forced" to use our way of approaching the environment names, and what's the default. I believe we're just guiding folks to the right path (i.e. not expose their secrets.)

We won't be removing this functionality or the extension methods, mainly because it's a breaking change, but also they are actually being used safely by internal and external customers.

Member

muratg commented Oct 13, 2016

@hlubovac In the scenario you described, you still have one production secret on the development machine: the secret to get other secrets from "secret-keeper" service 😄 That's is NOT secure by any means, and I recommend keeping development/test secrets and production secrets seperate with two different accounts/keys (whatever) on the "secret-keeper" service.

Back to the feature -- like you said you have a workaround for your own workload. You're not "forced" to use our way of approaching the environment names, and what's the default. I believe we're just guiding folks to the right path (i.e. not expose their secrets.)

We won't be removing this functionality or the extension methods, mainly because it's a breaking change, but also they are actually being used safely by internal and external customers.

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 13, 2016

Thank you for your time.

hlubovac commented Oct 13, 2016

Thank you for your time.

@muratg

This comment has been minimized.

Show comment
Hide comment
@muratg

muratg Oct 13, 2016

Member

Re hosting being a Kestrel dependency, this is by design. Kestrel is only one of the web servers. (The other one we own is WebListener, which also depends on hosting.) Anyone can take a dependency on Hosting (among other things) and write their web servers. Let me know if this doesn't clarify the dependency direction for you.

Member

muratg commented Oct 13, 2016

Re hosting being a Kestrel dependency, this is by design. Kestrel is only one of the web servers. (The other one we own is WebListener, which also depends on hosting.) Anyone can take a dependency on Hosting (among other things) and write their web servers. Let me know if this doesn't clarify the dependency direction for you.

@cwe1ss

This comment has been minimized.

Show comment
Hide comment
@cwe1ss

cwe1ss Oct 13, 2016

Contributor

I had the same question a few months ago - it contains some more background: #720

We are also using certificates for authenticating with Azure Key Vault to get secrets. our developers should never have production certs on their machines, but we also think this default behavior is way too dangerous - that's why we decided not to use the built-in environment names - we use DEV/PROD instead. This way, using "Production" on a dev machine just throws on startup.

Contributor

cwe1ss commented Oct 13, 2016

I had the same question a few months ago - it contains some more background: #720

We are also using certificates for authenticating with Azure Key Vault to get secrets. our developers should never have production certs on their machines, but we also think this default behavior is way too dangerous - that's why we decided not to use the built-in environment names - we use DEV/PROD instead. This way, using "Production" on a dev machine just throws on startup.

@muratg

This comment has been minimized.

Show comment
Hide comment
@muratg

muratg Oct 13, 2016

Member

@cwe1ss, if your production secrets are not on the dev or test machines, why would this be dangerous?

Member

muratg commented Oct 13, 2016

@cwe1ss, if your production secrets are not on the dev or test machines, why would this be dangerous?

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 13, 2016

@muratg, I think I understand the dependency tree. That particular example is a little confusing because we usually "host a [kestrel] service", as opposed to "[kestrel-]serve a host".

I agree with you that business-environment secrets shouldn't cross business-environment boundaries. The example I gave you was hypothetical, featuring a much bigger problem, introduced by the solution to forgetting to wrap app.UseDeveloperExceptionsPage(); within a web.config setting, like we've done so far. It was so much easier to not include that snippet into the template, and instead advertise it via some blog, like everything else is done.

I haven't started researching your solution/suggestions to handling secrets in config files in general, so I can't relate to that at the moment.

hlubovac commented Oct 13, 2016

@muratg, I think I understand the dependency tree. That particular example is a little confusing because we usually "host a [kestrel] service", as opposed to "[kestrel-]serve a host".

I agree with you that business-environment secrets shouldn't cross business-environment boundaries. The example I gave you was hypothetical, featuring a much bigger problem, introduced by the solution to forgetting to wrap app.UseDeveloperExceptionsPage(); within a web.config setting, like we've done so far. It was so much easier to not include that snippet into the template, and instead advertise it via some blog, like everything else is done.

I haven't started researching your solution/suggestions to handling secrets in config files in general, so I can't relate to that at the moment.

@cwe1ss

This comment has been minimized.

Show comment
Hide comment
@cwe1ss

cwe1ss Oct 13, 2016

Contributor

Because it's not 100% fool-proof. Our system will probably run for 10-15 years and it will contain ~100 services, so there might be this one Friday afternoon where I have to use the production certificate to debug a nasty bug and then I forget to delete the certificate. I don't want this to be the day where I also messed up my production DB because of that. 😄

But yes, we are taking a few more precautions to make sure this can never happen. E.g. Right now, we still have all environment configurations checked in to the same repo - we'll move them into their own repo, ...

Contributor

cwe1ss commented Oct 13, 2016

Because it's not 100% fool-proof. Our system will probably run for 10-15 years and it will contain ~100 services, so there might be this one Friday afternoon where I have to use the production certificate to debug a nasty bug and then I forget to delete the certificate. I don't want this to be the day where I also messed up my production DB because of that. 😄

But yes, we are taking a few more precautions to make sure this can never happen. E.g. Right now, we still have all environment configurations checked in to the same repo - we'll move them into their own repo, ...

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 13, 2016

@muratg , if for anything else, then because it's confusing. Configuration values should be defined by the entity in charge. In this context, the entity in charge is the application and not the "platform". I don't understand what in the world the "platform" needs to know the environment name for!? How's that relevant to launching a TCP listener, please? Why do you think the platform should define the business process/workflow of the "entity in charge"? I don't understand why you're still defending this.

Sure, I get it: mistake has been made, too many apps out there that depend on this; you can't remove it; fine.

... Although this is perfect for System.ObsoleteAttribute, used many times before to announce a feature planned to go away in near future.

hlubovac commented Oct 13, 2016

@muratg , if for anything else, then because it's confusing. Configuration values should be defined by the entity in charge. In this context, the entity in charge is the application and not the "platform". I don't understand what in the world the "platform" needs to know the environment name for!? How's that relevant to launching a TCP listener, please? Why do you think the platform should define the business process/workflow of the "entity in charge"? I don't understand why you're still defending this.

Sure, I get it: mistake has been made, too many apps out there that depend on this; you can't remove it; fine.

... Although this is perfect for System.ObsoleteAttribute, used many times before to announce a feature planned to go away in near future.

@cwe1ss

This comment has been minimized.

Show comment
Hide comment
@cwe1ss

cwe1ss Oct 13, 2016

Contributor

@hlubovac I think you wanted to mention @muratg , right?

Contributor

cwe1ss commented Oct 13, 2016

@hlubovac I think you wanted to mention @muratg , right?

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 13, 2016

yes, sorry; corrected

hlubovac commented Oct 13, 2016

yes, sorry; corrected

@muratg

This comment has been minimized.

Show comment
Hide comment
@muratg

muratg Oct 13, 2016

Member

@hlubovac To be clear, I don't recommend using configuration files for production secrets -- use some other config mechanism. (If you're using Azure, it can be KeyVault or you can set up env vars on prod via Azure Portal). For development secrets, again, I don't recommend keeping them in config files (because of the risk of accidentally checking them in). Instead, use user-secrets tool.

Re your second point, everything is owned by application, platform does nothing special. We have a few extension points as part of the framework, and the templates use these as guidance.

Also, I don't think our guidance is a mistake - templates are mainly to guide new apps. Folks who have existing conventions have all means of applying those conventions in ASP.NET Core as well. If we completely remove these and put the guidance on a blog, I'd bet that a lot of folks would have bigger issues, like exposing their secrets due to logging/diagnostics, or checking them in as config files in their repos.

Perhaps we should write some documentation or a blog post about the design decisions on this to more clearly articulate what we tried to accomplish. Do you think this would be useful?

Again, (unless I'm missing a point) I don't see any real danger/risk AS LONG AS dev boxes do not contain any production secrets. If that's the case, you have much bigger problems IMO.

Member

muratg commented Oct 13, 2016

@hlubovac To be clear, I don't recommend using configuration files for production secrets -- use some other config mechanism. (If you're using Azure, it can be KeyVault or you can set up env vars on prod via Azure Portal). For development secrets, again, I don't recommend keeping them in config files (because of the risk of accidentally checking them in). Instead, use user-secrets tool.

Re your second point, everything is owned by application, platform does nothing special. We have a few extension points as part of the framework, and the templates use these as guidance.

Also, I don't think our guidance is a mistake - templates are mainly to guide new apps. Folks who have existing conventions have all means of applying those conventions in ASP.NET Core as well. If we completely remove these and put the guidance on a blog, I'd bet that a lot of folks would have bigger issues, like exposing their secrets due to logging/diagnostics, or checking them in as config files in their repos.

Perhaps we should write some documentation or a blog post about the design decisions on this to more clearly articulate what we tried to accomplish. Do you think this would be useful?

Again, (unless I'm missing a point) I don't see any real danger/risk AS LONG AS dev boxes do not contain any production secrets. If that's the case, you have much bigger problems IMO.

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 13, 2016

I simply have no incentive to argue. Your reasoning is illogical to me. You don't seem to recognize that design to be anti-pattern, and then discussing further is pointless. I am sorry that I have to be so blunt.

Thank you.

hlubovac commented Oct 13, 2016

I simply have no incentive to argue. Your reasoning is illogical to me. You don't seem to recognize that design to be anti-pattern, and then discussing further is pointless. I am sorry that I have to be so blunt.

Thank you.

@cwe1ss

This comment has been minimized.

Show comment
Hide comment
@cwe1ss

cwe1ss Oct 14, 2016

Contributor

Secrets are just one side of this issue. There are many more environment specific settings which are not security-relevant - those that usually used to be in the AppSettings section of web.config (e.g. URIs, cache settings, ...). Each one of these could lead to unintended side effects if applied in the wrong environment.

Of course, it would be best to keep all of these things separated as well, but right now, you are actively promoting this pattern in your templates - maybe this is something that should be discussed?!

Also I think it is problematic/unexpected that the behavior of Visual Studio (which looks at launchSettings.json and therefore defaults to Development) and raw dotnet run usage (which doesn't look at this file and therefore defaults to Production) is different.

I understand your reasons behind this and I understand that this can no longer be changed. I just wanted to provide more examples of IMO valid scenarios that might now be troublesome. Maybe we find some ways to improve this a bit.

Contributor

cwe1ss commented Oct 14, 2016

Secrets are just one side of this issue. There are many more environment specific settings which are not security-relevant - those that usually used to be in the AppSettings section of web.config (e.g. URIs, cache settings, ...). Each one of these could lead to unintended side effects if applied in the wrong environment.

Of course, it would be best to keep all of these things separated as well, but right now, you are actively promoting this pattern in your templates - maybe this is something that should be discussed?!

Also I think it is problematic/unexpected that the behavior of Visual Studio (which looks at launchSettings.json and therefore defaults to Development) and raw dotnet run usage (which doesn't look at this file and therefore defaults to Production) is different.

I understand your reasons behind this and I understand that this can no longer be changed. I just wanted to provide more examples of IMO valid scenarios that might now be troublesome. Maybe we find some ways to improve this a bit.

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 14, 2016

@muratg, @blowdart - unrelated to the topic, but since you mentioned managing secrets, could you pass it to the proper group that "Installing the Secret Manager tool" section of https://docs.asp.net/en/latest/security/app-secrets.html isn't quite complete (syntax isn't obvious/clear; no link to external document talking about defining "tools"; no example; following the pattern provided by the template doesn't work).

Maybe also hire a linguist to proof read your documentation and use adult language. "When any of the tools are defined in the project.json file, you must be in the same directory in order to use the tooling commands." What do you mean, I have to be in the same directory? Did you mean that I need to execute tooling commands within the same directory-context where the project.json file, holding the userSecretsId value, is located? By the way, relevant to aspnet/UserSecrets#62, I truly hope that you didn't forget to add code to that tool to accept the userSecretsId as an argument, as an alternative to keeping it in project.json file - which, by the way - makes being in the same directory totally irrelevant. In other words, userSecretsId is best abstracted away from the "platform" (e.g. located in a separate file) and provided to it by application code for both usage (I verified this) and management (I can't verify this until I figure out the syntax for "installing" the tool).

@cwe1ss, we need to be super careful not to use the business-environment name to drive obtaining our userSecretsId, cause - those that do occasionally debug production systems from their dev workstations (regardless of whether that's good or bad practice) will be tempted to create a production-related userSecret store, and there you go... (@muratg, @blowdart, are you listening to this). But, my stacktrace won't get displayed to the end-user, cause that's more important (like I'm going to use environment name to drive enabling/disabling application's features anyway; instead, the proper way is to use feature-dedicated settings, of course, and that is what you should promote, @muratg and @blowdart, and, of course, stay away from defining my business process). :)

Thanks.

hlubovac commented Oct 14, 2016

@muratg, @blowdart - unrelated to the topic, but since you mentioned managing secrets, could you pass it to the proper group that "Installing the Secret Manager tool" section of https://docs.asp.net/en/latest/security/app-secrets.html isn't quite complete (syntax isn't obvious/clear; no link to external document talking about defining "tools"; no example; following the pattern provided by the template doesn't work).

Maybe also hire a linguist to proof read your documentation and use adult language. "When any of the tools are defined in the project.json file, you must be in the same directory in order to use the tooling commands." What do you mean, I have to be in the same directory? Did you mean that I need to execute tooling commands within the same directory-context where the project.json file, holding the userSecretsId value, is located? By the way, relevant to aspnet/UserSecrets#62, I truly hope that you didn't forget to add code to that tool to accept the userSecretsId as an argument, as an alternative to keeping it in project.json file - which, by the way - makes being in the same directory totally irrelevant. In other words, userSecretsId is best abstracted away from the "platform" (e.g. located in a separate file) and provided to it by application code for both usage (I verified this) and management (I can't verify this until I figure out the syntax for "installing" the tool).

@cwe1ss, we need to be super careful not to use the business-environment name to drive obtaining our userSecretsId, cause - those that do occasionally debug production systems from their dev workstations (regardless of whether that's good or bad practice) will be tempted to create a production-related userSecret store, and there you go... (@muratg, @blowdart, are you listening to this). But, my stacktrace won't get displayed to the end-user, cause that's more important (like I'm going to use environment name to drive enabling/disabling application's features anyway; instead, the proper way is to use feature-dedicated settings, of course, and that is what you should promote, @muratg and @blowdart, and, of course, stay away from defining my business process). :)

Thanks.

@blowdart

This comment has been minimized.

Show comment
Hide comment
@blowdart
Member

blowdart commented Oct 14, 2016

That'll be @Rick-Anderson

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 14, 2016

Whatever did you mean... I'm not MS employee, don't know your organization (except that you only have 3 business environments: development, staging and production; it's a shame that you don't have one called "test", but now that makes all the sense), so I was asking you to pass that message through maybe an official internal request, as opposed to the optionally viewed public medium such as GitHub (you shouldn't assume my understanding of your internal rules, such as, maybe that you possibly treat GitHub as mandatory issue-tracker).

hlubovac commented Oct 14, 2016

Whatever did you mean... I'm not MS employee, don't know your organization (except that you only have 3 business environments: development, staging and production; it's a shame that you don't have one called "test", but now that makes all the sense), so I was asking you to pass that message through maybe an official internal request, as opposed to the optionally viewed public medium such as GitHub (you shouldn't assume my understanding of your internal rules, such as, maybe that you possibly treat GitHub as mandatory issue-tracker).

@blowdart

This comment has been minimized.

Show comment
Hide comment
@blowdart

blowdart Oct 14, 2016

Member

I tagged Rick because he owns documentation. Github is our official tracking mechanism. Even the docs are here, in https://github.com/aspnet/Docs. Rather than duplicate your comment there, because you care about responses, it's far easier to tag him here and let him run with it. As I did.

Member

blowdart commented Oct 14, 2016

I tagged Rick because he owns documentation. Github is our official tracking mechanism. Even the docs are here, in https://github.com/aspnet/Docs. Rather than duplicate your comment there, because you care about responses, it's far easier to tag him here and let him run with it. As I did.

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 14, 2016

Why bother the guy to feel obligated to read through all this thread, 100% irrelevant to what he needs to organize to get done? I think it would have been much easier if you fired up your internal-request tool, pasted the paragraph that I wrote, addressed it to him, and told me "it's done" - but whatever...

hlubovac commented Oct 14, 2016

Why bother the guy to feel obligated to read through all this thread, 100% irrelevant to what he needs to organize to get done? I think it would have been much easier if you fired up your internal-request tool, pasted the paragraph that I wrote, addressed it to him, and told me "it's done" - but whatever...

@blowdart

This comment has been minimized.

Show comment
Hide comment
@blowdart

blowdart Oct 14, 2016

Member

You're welcome to your opinion, however this is our workflow.

As this as spun way out of control topic wise, and as the original question has been answered I'm closing this issue.

Member

blowdart commented Oct 14, 2016

You're welcome to your opinion, however this is our workflow.

As this as spun way out of control topic wise, and as the original question has been answered I'm closing this issue.

@blowdart blowdart closed this Oct 14, 2016

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 14, 2016

Touché, sir (defining business-environment names).

hlubovac commented Oct 14, 2016

Touché, sir (defining business-environment names).

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 14, 2016

That's the way to get business done: sweep under the rug. Great job, @blowdart! Let's give you a raise.

hlubovac commented Oct 14, 2016

That's the way to get business done: sweep under the rug. Great job, @blowdart! Let's give you a raise.

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 14, 2016

Thanks for thumbs down. I will not undo [by deleting the comment] what I said. I did that to show frustration related to @blowdart's closing the issue without asking interested parties if they agree with that plan (I clearly wouldn't, as I still find the current implementation in issue anti-pattern, in several locations, as a matter of fact). Excuses such as "too many users would be affected" aren't really acceptable (so, what; how many other breaking changes have been published as part of these previews so far; this one would be for the better good, I'd say).

hlubovac commented Oct 14, 2016

Thanks for thumbs down. I will not undo [by deleting the comment] what I said. I did that to show frustration related to @blowdart's closing the issue without asking interested parties if they agree with that plan (I clearly wouldn't, as I still find the current implementation in issue anti-pattern, in several locations, as a matter of fact). Excuses such as "too many users would be affected" aren't really acceptable (so, what; how many other breaking changes have been published as part of these previews so far; this one would be for the better good, I'd say).

@cwe1ss

This comment has been minimized.

Show comment
Hide comment
@cwe1ss

cwe1ss Oct 14, 2016

Contributor

@hlubovac your communication style in this issue has also been an anti-pattern. 😄 maybe a nicer tone would have brought you further...

@blowdart should I create an issue in the template repo to discuss if using appsettings.Environment.json should be removed or do you think it's not worth it?

Contributor

cwe1ss commented Oct 14, 2016

@hlubovac your communication style in this issue has also been an anti-pattern. 😄 maybe a nicer tone would have brought you further...

@blowdart should I create an issue in the template repo to discuss if using appsettings.Environment.json should be removed or do you think it's not worth it?

@blowdart

This comment has been minimized.

Show comment
Hide comment
@blowdart

blowdart Oct 14, 2016

Member

@cwe1ss Sure, that's a different issue than this one.

Member

blowdart commented Oct 14, 2016

@cwe1ss Sure, that's a different issue than this one.

@hlubovac

This comment has been minimized.

Show comment
Hide comment
@hlubovac

hlubovac Oct 14, 2016

@cwe1ss, you are absolutely right. My tone has been cruel. I'll disagree that a nicer tone would have made a difference, though: Mr. @blowdart had made it very clear at the very beginning that "this would not change". I'm actually surprised this issue hadn't been closed right after that message. Point taken, though, and thank you. Also, thank you for agreeing with me on the problem in issue.

hlubovac commented Oct 14, 2016

@cwe1ss, you are absolutely right. My tone has been cruel. I'll disagree that a nicer tone would have made a difference, though: Mr. @blowdart had made it very clear at the very beginning that "this would not change". I'm actually surprised this issue hadn't been closed right after that message. Point taken, though, and thank you. Also, thank you for agreeing with me on the problem in issue.

@muratg

This comment has been minimized.

Show comment
Hide comment
@muratg

muratg Oct 14, 2016

Member

@cwe1ss Yes, please do. I was planning to do it sometime today. [Repo: https://github.com/aspnet/templates]

Member

muratg commented Oct 14, 2016

@cwe1ss Yes, please do. I was planning to do it sometime today. [Repo: https://github.com/aspnet/templates]

@cwe1ss

This comment has been minimized.

Show comment
Hide comment
@cwe1ss

cwe1ss Oct 14, 2016

Contributor

done! I hope the description is ok. -> aspnet/Templates#678

Contributor

cwe1ss commented Oct 14, 2016

done! I hope the description is ok. -> aspnet/Templates#678

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment