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

[installer, gitpod-db] Introduce database.ssl.ca #15320

Merged
merged 1 commit into from
Dec 15, 2022
Merged

Conversation

geropl
Copy link
Member

@geropl geropl commented Dec 13, 2022

Description

This allows Gitpod to directly created SSL encrypted connections to MySQL databases.

For testing 👇 there are two environments:

  • preview on this branch (w/o SSL)
  • the AWS release test instance (deployed from this branch with the deps: external flag) with external RDS with SSL.
    • It uses the default jobs config, with two changes:
      • a secret named database-ssl and an attribute ca.crt that contains the cert (chain) to validate the RDS certificate
      • the installer config slightly adjust to:
       database:
         inCluster: false
         external:
             ...
         ssl:
           customCa:
             kind: secret
             name: database-ssl
      

Related Issue(s)

Fixes #12012

How to test

*: there is an odd content-init error I get on the AWS realease test env. @nandajavarma any idea where that might come from? 🤔

Release Notes

Allow specifying CA certificate to configure SSL secured database connections

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-gpl-12012-ssl.14 because the annotations in the pull request description changed
(with .werft/ from main)

@geropl
Copy link
Member Author

geropl commented Dec 13, 2022

@mrzarquon @mrsimonemms Could you help me test this change? I think the quickest would be if you could point me to an AWS installation and show me how to configure it with the certs from here &deploy it...? 🤔

Update: Ok, got it working thanks to @mrsimonemms and @nandajavarma 🙏 !
See the description for test instructions. ☝️

@geropl geropl force-pushed the gpl/12012-ssl branch 5 times, most recently from cf604cd to 56a3c41 Compare December 14, 2022 16:07
@geropl geropl marked this pull request as ready for review December 14, 2022 17:01
@geropl geropl requested review from a team December 14, 2022 17:01
@github-actions github-actions bot added team: SID team: webapp Issue belongs to the WebApp team labels Dec 14, 2022
@mrsimonemms
Copy link
Contributor

@geropl looks good, but it could do with a cluster validation check to ensure the secret is uploaded and valid.

It'll be something like:

if cfg.Config.Database.SSL != nil && cfg.Config.Database.SSL.CustomCA != nil {
	secretName := cfg.Config.Database.SSL.CustomCA.Name
	res = append(res, cluster.CheckSecret(secretName, cluster.CheckSecretRequiredData("<some key1>", "<some key2>")))
}

The <some key1> <some key2> are the keys you want to ensure are present in the secret.

Copy link
Contributor

@mrsimonemms mrsimonemms left a comment

Choose a reason for hiding this comment

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

One question, but happy with the change

Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

LGTM % modulo dropping custom from the references. All options are custom, there shouldn't be a need to duplicate that in the name.

/hold

Password string
Host string
Database string
CustomCACert string
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CustomCACert string
CACert string

Copy link
Member

Choose a reason for hiding this comment

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

Just feels we don't need to push the "custom" part into this

Password: os.Getenv("DB_PASSWORD"),
Host: net.JoinHostPort(os.Getenv("DB_HOST"), os.Getenv("DB_PORT")),
Database: "gitpod",
CustomCACert: os.Getenv("DB_CUSTOM_CA_CERT"),
Copy link
Member

Choose a reason for hiding this comment

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

Likewise here, I'd drop the custom.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good point. So far I have been torn we because I'd have to partially retest. But I think you're right. 💯

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. Done, and re-tested: works.

@geropl
Copy link
Member Author

geropl commented Dec 15, 2022

/unhold

ref

@roboquat roboquat merged commit 8a03b3a into main Dec 15, 2022
@roboquat roboquat deleted the gpl/12012-ssl branch December 15, 2022 15:29
@roboquat roboquat added the deployed: webapp Meta team change is running in production label Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production release-note size/L team: SID team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support SSL configurations of mysql connections
4 participants