Skip to content
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

Support SSL server certificate parameter for MySQL #15172

Merged
merged 1 commit into from
Mar 24, 2021

Conversation

jeff303
Copy link
Contributor

@jeff303 jeff303 commented Mar 15, 2021

Adding ssl-cert config field to MySQL DB details map, to hold the server cert chain in PEM format (similar to what is done in MongoDB driver)

Updating MySQL driver init to map :ssl-cert into :serverSslCert for the JDBC url, when ssl is in use and cert is provided (the MariaDB driver we are using accepts PEM format certificates inline directly for the param value, so no need to shepherd into a temp file)

Update CircleCI config with new be-tests-mysql-ssl-latest-ee job, modeled similarly to the be-tests-oracle-ssl-ee one

  • use extra-env to set all the MySQL DB related vars (for an RDS instance, currently)
  • adding the rds-combined-ca-bundle.pem certificate to resources/certificates
  • loading that cert bundle from resources directory via env var

@jeff303
Copy link
Contributor Author

jeff303 commented Mar 15, 2021

Fixes #1350.

@jeff303 jeff303 linked an issue Mar 15, 2021 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Mar 15, 2021

Codecov Report

Merging #15172 (7f83eb8) into master (3099c35) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15172      +/-   ##
==========================================
- Coverage   84.42%   84.40%   -0.02%     
==========================================
  Files         390      390              
  Lines       30903    30903              
  Branches     2213     2214       +1     
==========================================
- Hits        26089    26084       -5     
- Misses       2601     2605       +4     
- Partials     2213     2214       +1     
Impacted Files Coverage Δ
src/metabase/task/sync_databases.clj 74.10% <0.00%> (-3.60%) ⬇️
src/metabase/models/query.clj 85.41% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b2c388...7f83eb8. Read the comment docs.

@jeff303 jeff303 force-pushed the support-mysql-pem-certificate branch 2 times, most recently from 113af45 to dc4e90d Compare March 16, 2021 19:58
@paoliniluis
Copy link
Contributor

It would be a good idea to include the Azure cert chain as well (https://cacerts.digicert.com/DigiCertGlobalRootG2.crt.pem) so users can use postgres and mysql in both public clouds without issues (till we have a self upload option and we can eliminate certificate bundling altogether)

@paoliniluis
Copy link
Contributor

One more thing: if you bundle the scripts in the release jar, they won't be needed in the container right? (so this one can be reverted e2147a2)

@jeff303
Copy link
Contributor Author

jeff303 commented Mar 17, 2021

It would be a good idea to include the Azure cert chain as well (https://cacerts.digicert.com/DigiCertGlobalRootG2.crt.pem) so users can use postgres and mysql in both public clouds without issues (till we have a self upload option and we can eliminate certificate bundling altogether)

One more thing: if you bundle the scripts in the release jar, they won't be needed in the container right? (so this one can be reverted e2147a2)

I think creating a Metabase-wide trust store and automatically including certain CAs into it is out of scope for this issue/change. For one thing, it's complicated (we would either need to copy the JDK-wide cacerts file, add some stuff to it, then bring it back), and we would either have to override JDK level params (which might have unintended side effects on other stuff), or sneakily create the trust store "behind the scenes" and set it for MySQL, but only in certain conditions (ex: not if the user has their own custom certificate, like for an on-prem database). That trickiness will become too difficult to manage, IMHO (we would have to present some type of workflow/wizard, first saying "Are you using RDS?", etc. to show/hide various other configs).

I think bundling them with the Docker image is good, since it will automatically handle a lot of this for people running the Docker image (which includes a lot of deployment scenarios).

@jeff303
Copy link
Contributor Author

jeff303 commented Mar 18, 2021

To close the loop on a few things, as this is finally ready for review.

  1. The first problem with the CI tests was that I was using the > operator in the CircleCI YAML to try to prepend the extra environment options. This includes a newline at the end of the string, and hence prepending them wasn't actually working at all. Fixed by switching to >- which chomps the final newline.
  2. Snuck in a fix for a ridiculously obscure problem that was making the Oracle SSL job fail
  3. I created a new MySQL RDS instance (tiny) for the express purpose of testing SSL connectivity. It lives in us-east-2, and is called mysql-ci-test-db. The host name, and password for the DB user (whose name is metabase) are configured via the CircleCI job environment variables admin page, and the open source CircleCI config here is just referencing them when it sets the relevant MB_MYSQL_TEST_* variables for lein test

.circleci/config.yml Outdated Show resolved Hide resolved
Copy link
Member

@camsaul camsaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems really crazy to do a whole separate job just to test connecting with SSL. I'd just pass the existing env vars to an existing test job and then write a new test that will look for the extra env vars and see whether we can connect with them if those env vars are present

@jeff303 jeff303 force-pushed the support-mysql-pem-certificate branch from fa00968 to d74af16 Compare March 19, 2021 16:03
@jeff303 jeff303 requested a review from camsaul March 19, 2021 18:53
Adding ssl-cert config field to MySQL DB details map, to hold the server cert chain in PEM format (similar to what is done in MongoDB driver)

Updating MySQL driver init to map :ssl-cert into :serverSslCert for the JDBC url, when ssl is in use and cert is provided (the MariaDB driver we are using accepts PEM format certificates inline directly for the param value, so no need to shepherd into a temp file)

Adding new test to mysql_test.clj to run a single test while connecting via SSL with PEM cert

Update CircleCI config:
 - use extra-env to set all the MySQL SSL instance DB related vars (for an RDS instance, currently)
 - adding the rds-combined-ca-bundle.pem certificate to resources/certificates
 - loading that cert bundle from resources directory via env var

Adding to/fixing assertion in connection-spec-test for :ssl
@jeff303 jeff303 force-pushed the support-mysql-pem-certificate branch from 81525bd to 7f83eb8 Compare March 19, 2021 20:03
@jeff303 jeff303 mentioned this pull request Mar 19, 2021
Copy link
Member

@camsaul camsaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool works for me

@jeff303 jeff303 merged commit 54f6a5e into master Mar 24, 2021
@jeff303 jeff303 deleted the support-mysql-pem-certificate branch March 24, 2021 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support PEM file to connect to MySQL DB with SSL certificate
3 participants