Clarify compiler warning and add tool warning for scaffolded connection string

[jeremycook]

I like most non-MS commenters on #10432 was surprised to find the connection string I used to run Scaffold-DbContext got embedded in the generated Context. I'd consider this a security bug if it were my project. It could be mitigated by requiring the user to opt into that behavior or at a minimum outputting a warning at the time the command is ran.

What I propose is that if you want to generate a context that embeds the connection string then you must opt into it. You could warn users to help them transition to the new system with: "Use the -EmbedConnectionString flag to hardcode the connection string into the generated context."

Alternatively, if -EmbedConnectionString is a no-go for backwards compatibility then output a warning when running the command normally about the embedded connection string and security implications, and then mention a flag for opting out of embedding: "The connection string has been hard coded into the generated context. Pass the -ExcludeConnectionString flag to omit it from the generated context."

A third option would be an -ExcludeOnConfiguringMethod flag that would have the same outcome and allow developers to provider their own OnConfiguring method from a partial context class...which could prove useful in a lot of ways and would bypass needing to subclass the generated class to do more interesting things with that.

[ErikEJ]

@jeremycook Just FYI, this is fixed in EF Core Power Tools

[bricelam]

Some additional ideas:

• Strip out any passwords in the generated code; if it breaks, it breaks.
• Warn when running the command (in addition to the #warning we generate in code) and guide people towards the Name= syntax that works with user secrets.

[ajcvickers]

@jeremycook This is an area where:

• People get confused and give up If the generated code does not run without them having to make changes. We try hard to be friendly to people getting started.
• Because this is something that should be changed, we make the generated code result in a compiler warning when built.

We could do more to ensure people are informed and we have discussed some ideas--see comment from @bricelam above. However, we would like to understand why the compiler warning was not enough in your case. Could you shed some light on that?

[jeremycook]

@ajcvickers as I'm sure you are aware there are two issues here: security and developer experience. Here are my thoughts that hopefully help crystallize a solution that can address both.

I understand the desire of your first bullet point and saw that reiterated in #10432, and is something I feel like my suggestions in this issue successfully address in either a breaking or non-breaking way. @bricelam 's suggestion to guide people towards the Name=ConnectionStringName syntax in the warning would have helped me a lot. Instead I found out about that only after deploying, chasing my tail, eventually Googling, discovering #10432, and reading the whole thing. That was a very bad experience. It went like this:

• Development on my local machine worked because my runtime connection string was the same one I used for reverse engineering...so no way to know I goofed while developing. I ignored the warning because I was using integrated security, no username or password, I saw no way around it without manually deleting that line (which goes against the way I intend to use reverse engineering), and everything was working (on my machine).
• Test failed with an error message about not being able to connect to the database server. So I started off with troubleshooting firewall issues and the like instead of looking into the connection string not being set to the correct value. I assumed the connection string was correct because I configured this application like all of my other web apps. I only figured out that it was the connection string after ruling out other issues and adding this piece of code to Startup:

        public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
        {
            var logger = app.ApplicationServices.GetRequiredService<ILogger<Startup>>();
            using var scope = app.ApplicationServices.CreateScope();
            var db = scope.ServiceProvider.GetRequiredService<MyContext>();

            var cs = new Microsoft.Data.SqlClient.SqlConnectionStringBuilder(db.Database.GetDbConnection().ConnectionString);
            logger.LogInformation($"The database server is {cs.DataSource}, database is {cs.InitialCatalog}, and integrated security is {(cs.IntegratedSecurity ? "enabled" : "disabled")}.");

At this point I was quite frustrated (to put it lightly). I knew how to workaround it, and so I created a subclass of the generated context, overrode OnConfiguring so that OnConfiguring of the generated context would not run, ensuring that my connection string from IConfiguration would get used and not the embedded one. I got it all working and then proceeded to Google to see if there is a better way, at which point I discovered Name=ConnectionStringName. I reverse engineered that way which made the warning go away (thank goodness), but having already subclassed and changed all my code to use it I decided I'm not going back to using the generated context directly.

---

As far as the compiler warning goes, I did see it and decided I wasn't too worried about the connection string being used because I was using integrated security. Had I been using a username or password I would have dug deeper when I inspected the code where the warning was.

But that is me, whereas a lot of developers I have worked with ignore warnings or might only occasionally take note. They might also just silence the warnings in Visual Studio. 7 years ago I worked on a project with 1000s of warnings. The lead developer didn't perceive an issue. So I don't think the in-code warning by itself is enough to protect many average developers from themselves. It would keep me safe because I pay attention to warnings and fix them or suppress them in code when appropriate, but I know enough devs who just ignore them like an inbox with 10000 unread messages.

Sorry to be long winded. I hope that my words and the others who have expressed there concerns in #10432 hold enough weight to move the peg on this one. I think you guys can make it easy to use for one-offs and big projects, experienced and inexperienced folks, and make it safe to use by default.

[jeremycook]

@ErikEJ I considered your tool but it wasn't obvious to me how I would do it without installing the Visual Studio extension. My desire is to keep the ability to update the code based on the database as simple and low friction as possible. Scaffold-DbContext is very close to what I need, and I have accepted the trade-offs around limited customization. My command looks like this:

Scaffold-DbContext Name=ThingManager Microsoft.EntityFrameworkCore.SqlServer -UseDatabaseNames -DataAnnotations -Force -OutputDir Data -Project ThingManager -StartupProject Website

The project has a Readme.md file that contains that command. Because the EF Core tooling is distributed through a Nuget package anyone with Visual Studio can do what they need to without worrying about external dependencies.

The Data folder only contains generated classes. I have a Partials folder next to it where I do some customizing. I use MVC extensibility points to customize the display names of properties and classes whereas when doing code-first development my preference has been to use DisplayAttribute and friends. I also don't _dbContext.Add(model) or _dbContext.Update(model) in my controllers. I instantiate or find an entity and then map properties from the input model to the entity. This is for a mostly CRUD app.

[smitpatel]

If connection string is properly configuring using IConfiguration then issue you described shouldn't occur. See #6686

[jeremycook]

@smitpatel I assumed that behavior when I read the warning and the code that it was warning about. And sure, in an ideal world I wouldn't screw up. Occasionally I do and in this case what EF core reverse engineering generated obfuscated the real issue, which was that it was using the scaffolded connection string while I thought it was using the one from IConfiguration. If OnConfiguring had not been scaffolded at all I would have gotten a much more helpful error message about connection string being null both on my development machine and the test machine (though it wouldn't have made it that far because it would have presented itself when developing). I had a paragraph about this as I was typing up my response to @ajcvickers but it seemed self-evident in the other things I was saying and my comment was getting long enough as it was. So I cut it out.

[smitpatel]

If you scaffold a code in dev environment which puts dev connection string in the file, and then publish code in production with proper configuration set for a web app then even if the connection string is incorrect in dbcontext file, it works since it is already configured. If you ran into issue then please provide a full repro so we can investigate.

[jeremycook]

If that were the issue then that would be this issue, and I would provide a repro. The issue is that a connection string is scaffolded which can introduce security, design-time and run-time challenges. Let developers leave the connection string out of the generated Context and those challenges evaporate.

[ajcvickers]

@jeremycook I think you have a valid point that the warning only mentions security. It doesn't warn you that if a connection string is not configured by the app, or not found when the app runs, then the scaffolded connection string will used.

To @smitpatel's point, this is an example of what we scaffold:

if (!optionsBuilder.IsConfigured)
{
#warning To protect potentially sensitive information in your connection string, you should move it out of source code. See http://go.microsoft.com/fwlink/?LinkId=723263 for guidance on storing connection strings.
    optionsBuilder.UseSqlServer("Server=(local);Database=Test;ConnectRetryCount=0;User ID=sa;Password=PassWord!", x => x.UseNetTopologySuite());
}

Notice that this connection string is only used if the provider is not already configured. The provider is only "configured" when the connection string has been configured. (Except in some corner cases that don't matter here.)

So this means if configuration of the connection string in the app has been done but is not working, then this can be masked because the scaffolded connection will then be used.

[ajcvickers]

Team meeting notes:

• We will update the warning message to indicate that this is not just about security.
• In addition to the compiler warning, we will spit out a warning from the tool.
• We will direct people more directly to the name= syntax.
• @bricelam will also look for other cleanup/messaging changes we could do here.

[jeremycook]

Thank you all for taking the time to consider this and for your response.

[darkguy2008]

@ajcvickers why not just add a flag? something like --no-connection-string would be really appreciated. Experienced devs shouldn't have to deal with this nonsense, just stamp a flag and get over it. I treat warnings as errors, a build should be clean and not even giving the users a choice whether to generate it or not is bad development UX.

[bricelam]

@darkguy2008 Are you aware of the option to use dotnet ef dbcontext scaffold Name=ConnStrFromConfig? This won't generate a warning and works well with user secrets.

[bricelam]

You can also use your own template (see bricelam/EFCore.TextTemplating) and delete the line that generates the connection string.

[darkguy2008]

@bricelam thanks for your answer! To be honest I wasn't aware of that parameter, so thanks for that. Although the user experience is still a bit weird:

So my DbContext is scaffolded by using a connectionstring in a file that's not (!) appsettings.json as I've set up my project:

string configFile = Path.Combine(Directory.GetCurrentDirectory(), "config.json");
if (!File.Exists(configFile))
    configFile = Path.Combine(new DirectoryInfo(Directory.GetCurrentDirectory()).Parent.FullName, "config.json");

var env = Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT");
var configuration = new ConfigurationBuilder()
.AddJsonFile(configFile, optional: true, reloadOnChange: true)
.AddJsonFile("appsettings.json", optional: true, reloadOnChange: true)
.Build();

return new WebHostBuilder()
.UseKestrel()
.UseConfiguration(configuration)
.UseUrls(new string[] { configuration["URL"] })
.UseStartup<Startup>();

The good news (and kudos) is that it seems to work as the tables are regenerated correctly, so it really grabbed the connection string from my config.json file. Now in the DbContext I get this:

protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
{
    if (!optionsBuilder.IsConfigured)
    {
        optionsBuilder.UseSqlServer("Name=SQLServer");
    }
}

Which is interesting to say the least, but the good news is that it works.

I would have expected something different, like, a flag to just skip the whole OnConfiguring generation. I really didn't expect that Name=ConStrFomConfigFile flag to have this result, so hopefully this will also clear doubts for future bypassers.

I appreciate the work you've done with the T4 templates, however I've already had my fair share of T4 with another project and they're as useful as nasty, so I'd rather steer away from them :) just my individual experience, but cool project nonetheless 👍

[LordBenjamin]

Appreciate the suggestions @bricelam, but it seems like these all involve extra work on the part of the implementing developer (setting up T4 templates is an extra step and a different thing to learn by itself, depending on config.json only makes sense when you're working with an ASP.NET Core application).

I've been reading through a few related issues and I've seen heavy resistance from the EF Core team towards adding a --do-not-embed-connection-string or similar flag. But I really haven't seen any clear justification for why this would be a problem?

@ajcvickers mentioned:

People get confused and give up If the generated code does not run without them having to make changes. We try hard to be friendly to people getting started.

This surely doesn't apply if it's an opt-out behaviour?

I appreciate and applaud the desire to keep the defaults simple, but why does this prevent having the option to override the defaults (without resorting to T4 / regex / custom designer code / etc, all of which will break as soon as the shape of the warning or surrounding code changes)?

For now, I am suppressing the warning using:

using Microsoft.EntityFrameworkCore.Design;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Scaffolding;
using Microsoft.EntityFrameworkCore.Scaffolding.Internal;
using Microsoft.Extensions.DependencyInjection;

namespace Auditing.EntityFramework.Entities.Design {
    public class DesignTimeServices : IDesignTimeServices {
        public void ConfigureDesignTimeServices(IServiceCollection serviceCollection) {
            serviceCollection.AddSingleton<ICSharpDbContextGenerator, DbContextGenerator>();
        }
    }

#pragma warning disable EF1001 // Internal EF Core API usage.
    public class DbContextGenerator : CSharpDbContextGenerator {

        public DbContextGenerator(
            IProviderConfigurationCodeGenerator providerConfigurationCodeGenerator,
            IAnnotationCodeGenerator annotationCodeGenerator,
            ICSharpHelper cSharpHelper)
            : base(providerConfigurationCodeGenerator, annotationCodeGenerator, cSharpHelper) {
        }

        public override string WriteCode(
            IModel model,
            string contextName,
            string connectionString,
            string contextNamespace,
            string modelNamespace,
            bool useDataAnnotations,
            bool suppressConnectionStringWarning) {

            return base.WriteCode(
                model,
                contextName,
                connectionString,
                contextNamespace,
                modelNamespace,
                useDataAnnotations,
                suppressConnectionStringWarning: true); // Override default behaviour
        }
    }
}
#pragma warning restore EF1001 // Internal EF Core API usage.

However, as EF1001 is warning me, this may break with new EF Core releases. Also, as @jeremycook notes (and indeed the emitted #warning also notes, the original connection string is still embedded, which is a potential security issue and violates the principle of least-surprise in scenarios where the connection string is accidentally omitted from, for example, config.json.

It's proving difficult to reproduce a workflow that was trivial to implement with LINQ-to-SQL / sqlmetal.exe, which is preventing us from moving applications over to .NET Core. Is DB-first development discouraged by EF Core?

[ErikEJ]

Fwiw, I will start work on a .NET Core based command line version of the EF Core Power Tools reverse engineer component.

[lajones]

See also this docs issue to cover Brice's comment above that we should point people to the Name= syntax more.

[darkguy2008]

This is great, thanks for the work involved! This allows me to remove a dummy project from my solution and less files in my repo :)