prisma / quaint

SQL Query AST and Visitor for Rust
Apache License 2.0
583 stars 61 forks source link

sslaccept option implies that SSL needs to be enabled #312

Closed dbussink closed 3 years ago

dbussink commented 3 years ago

The sslmode itself on its own does not enable / enforce SSL to be used. Today this means that in cases where no custom CA or client side certificate is provided, no SSL is enabled.

In turn, this means that for servers that provide a valid TLS certificate signed by a default system root need a custom CA to be passed in, normally a path to the system CA store.

This however turns into a lot of custom configuration, since the CA store location is highly operating system dependent.

The sslmode flag only accepts disable or require, since the underlying MySQL driver doesn't seem to support a "prefer TLS" mode which is available in for example libmysqlclient. Therefore any option than require or disable should be rejected here since this leads to unexpected behavior for users as well.

By explicitly handling the sslmode option here and also having sslaccept imply use_ssl, we make this configuration significantly simpler for users.

One example of such a provider is PlanetScale, but also other cloud databases like Amazon Aurora or Azure MySQL can be provisioned with a valid system signed root certificate and would benefit from this improvement.

Addresses https://github.com/prisma/prisma/issues/8697 & improves the experience in general described in https://github.com/prisma/prisma/issues/7292

dbussink commented 3 years ago

I also would want to argue that sslaccept=strict really should be the default here. I know it probably will break folks, but without that flag people don't connect safely when they enable SSL. This would be a potentially breaking change though so it might introduce friction for existing Prisma users that do use SSL.

The sslmode flag only accepts disable or require, since the underlying MySQL driver doesn't seem to support a "prefer TLS" mode which is available in for example libmysqlclient. Therefore any option than require or disable should be rejected here since this leads to unexpected behavior for users as well.

It also looks like https://www.prisma.io/docs/concepts/database-connectors/mysql documents prefer as a possible mode, but in practice that doesn't work. If you don't set sslmode at all today, no SSL is enabled and a server requiring SSL will error out.

janpio commented 3 years ago

Just to clarify: This does nothing about accepting system root certificate stores by default, but adds ssl_mode for MySQL and makes sslaccept to default to use_ssl = true;, correct?

janpio commented 3 years ago

Moved the comment re sslaccept=strict to its own issue: https://github.com/prisma/prisma/issues/8801

dbussink commented 3 years ago

Just to clarify: This does nothing about accepting system root certificate stores by default, but adds ssl_mode for MySQL and makes sslaccept to default to use_ssl = true;, correct?

The underlying MySQL driver already uses system root certificates by default. The problem is that it's impossible today to trigger the use_ssl side effect here without setting any option.

So the only option is to set a CA path to trigger use_ssl = true since the other options are not usable either if you're not using a client side certificate.

dbussink commented 3 years ago
  • Look into how our drivers do things around this topic

I'd like to caveat this one, since this is really all over the place. There are many other drivers that make it harder than needed and require similar configuration option(s). It's the main reason we had to write documentation like https://docs.planetscale.com/reference/secure-connections#ca-root-configuration to explain why it's relevant together with a bunch of standard paths.

This specifically applies to drivers based on libmysqlclient. This is also why I opened https://github.com/mysql/mysql-server/pull/358 with MySQL itself to also improve the user experience there and by proxy for all drivers that use this as the base (like in the Ruby, PHP & Python ecosystem).

On the other hand, there are other languages where this is much better. For example with Rust which is what Prisma uses under the hood:

    let url = "mysql://pqe0bj2i4h9u:pscale_pw_X@uxht6i2xcb2t.us-east-1.psdb.cloud/test";
    let builder = mysql_async::OptsBuilder::from_opts(mysql_async::Opts::from_url(url).unwrap());
    let pool = mysql_async::Pool::new(builder.ssl_opts(mysql_async::SslOpts::default()));

This shows how the default SslOpts provide good defaults that ensure a safe connection that also uses system roots. It also enforces the most strict mode verifying the CA chain and hostname (like HTTPS, although that's more for https://github.com/prisma/prisma/issues/8801 then).

Another good example imho is the Go driver:

db, err := sql.Open("mysql", "pqe0bj2i4h9u:pscale_pw_X@tcp(uxht6i2xcb2t.us-east-1.psdb.cloud)/test?tls=true")

Here tls=true enables TLS/SSL and it also enables system roots by default (which is the case for any TLS usage in Go). It also uses the most strict mode with CA and hostname verification (like HTTPS).

The commonly used Node.js MySQL driver also provides a pretty ok experience here, although I think it could be a bit simpler with the naming of the options:

var conn = mysql.createConnection({
  host: "uxht6i2xcb2t.us-east-1.psdb.cloud",
  user: "pqe0bj2i4h9u",
  password: "pscale_pw_X",
  database: "test",
  ssl: {
    rejectUnauthorized: true
  }
});

The ssl part here enables TLS / SSL and rejectUnauthorized enables the same strict HTTPS like verification mode. It's not needed to provide the CA roots here either, as system roots are here used by default too.

janpio commented 3 years ago

As said before, thanks for the input here. New improvement issue for slightly adapted change is at https://github.com/prisma/prisma/issues/8843

dbussink commented 3 years ago

I have updated this PR to have the most simple possible fix for the issue at hand.

pimeys commented 3 years ago

Ok, so I started now looking into this and I doing some testing. Tested with this branch, and I was able to push a simple schema to my planetscale branch by taking the connection url from the pscale UI and just adding sslaccept=strict to it.

I like it and it goes quite nicely together with how our SQL Server connections work, where you just add trustServerCertificate=true|false and you're all set.

Thank you for the PR. The failing tests are fixed on the main branch already, so I can merge this.

dbussink commented 3 years ago

@pimeys Thanks for getting this merged! :bow:

pimeys commented 3 years ago

It took a bit longer because all engineers are super busy right now. I read all suggestions, and this one feels like the right thing to do.