Added TLS security #1537

josecordaz · 2022-02-25T17:35:52Z

Closes: #1499

What changed?

Added parameters to pass in certificate and key. If they are not provided, they will be generated. Very convenient for development mode.

Why?

Add security to all the API interactions.

How did you test it?

Manually

Release notes

Documentation Changes

jpellizzari · 2022-03-04T16:23:42Z

Makefile

@@ -213,6 +213,13 @@ merged.lcov:
 	lcov --add-tracefile coverage/unittest.info --add-tracefile coverage/integrationtest.info -a coverage/lcov.info -o merged.lcov

 ##@ Utilities
+tls-files:
+ifeq (, $(shell which mkcert))
+ $(error "mkcert is not installed, consider following this instructions: https://github.com/FiloSottile/mkcert#installation ")


Does there need to be a dependencies.toml or a README entry to have devs install this tool?

I tried to add a toml entry but this dependency doesn't have the specific binary for x86_64 architectures.

Downloading https://github.com/FiloSottile/mkcert/releases/download/v1.4.3/mkcert-v1.4.3-darwin-x86_64 curl: (22) The requested URL returned error: 404

luizbafilho · 2022-03-04T19:08:51Z

README.md

@@ -72,8 +72,9 @@ To set up a development environment for the CLI
 3. Run `make all` to install dependencies and build binaries and assets
 4. Start a `kind` cluster like so: `KIND_CLUSTER_NAME=<some name> ./tools/kind-with-registry.sh`
 5. Run `./bin/gitops install --config-repo=<repo url>`
-6. Start the in-cluster API replacement job (powered by [http://tilt.dev](tilt.dev)) with `make cluster-dev`
-7. make or make unit-tests to ensure everything built correctly.
+6. Run `make tls-files`


since we are generating tls keys in memory are these new tools and generating localhost keys really necessary?

Fair question. The certificates generated in memory are not valid, but the ones generated here are. I guess this is a matter of preference.

Given that we are providing steps and tools to generate valid certs and having invalid auto-generated in-memory certs is a bit unusual, I have personally never seen any other project doing that, shouldn't we just remove the invalid auto-generate ones?

Since also we set up our dev environment to use the valid certs with tilt that auto-generate option will be mostly unused.

Not too unusual, that is exactly how it is being done in the enterprise project. I personally prefer to enforce the use of valid certificates for development. I'll remove the in-memory generation.

luizbafilho · 2022-03-04T19:10:17Z

cmd/gitops-server/cmd/cmd.go

@@ -74,6 +81,10 @@ func NewCommand() *cobra.Command {
 	cmd.Flags().StringVar(&options.NotificationControllerAddress, "notification-controller-address", "http://notification-controller./", "the address of the notification-controller running in the cluster")
 	cmd.Flags().IntVar(&options.WatcherPort, "watcher-port", 9443, "the port on which the watcher is running")

+	cmd.Flags().StringVar(&options.TLSCert, "tls-cert-file", "", "filename for the TLS certificate, in-memory generated if omitted")
+	cmd.Flags().StringVar(&options.TLSKey, "tls-private-key", "", "filename for the TLS key, in-memory generated if omitted")


to keep consistency with the other flag and with kube-api, we could add the -file suffix here.

luizbafilho · 2022-03-04T19:16:05Z

cmd/gitops-server/cmd/cmd.go

@@ -40,6 +43,9 @@ type Options struct {
 	LoggingEnabled                bool
 	OIDC                          OIDCAuthenticationOptions
 	NotificationControllerAddress string
+	TLSCert                       string


Just to match the flag and avoid confusion we could change TLSCert and TLSKey to TLSCertFile and TLSKeyFile

richardcase · 2022-03-05T10:28:16Z

cmd/gitops-server/cmd/cmd.go

@@ -74,6 +81,10 @@ func NewCommand() *cobra.Command {
 	cmd.Flags().StringVar(&options.NotificationControllerAddress, "notification-controller-address", "http://notification-controller./", "the address of the notification-controller running in the cluster")
 	cmd.Flags().IntVar(&options.WatcherPort, "watcher-port", 9443, "the port on which the watcher is running")

+	cmd.Flags().StringVar(&options.TLSCert, "tls-cert-file", "", "filename for the TLS certificate, in-memory generated if omitted")
+	cmd.Flags().StringVar(&options.TLSKey, "tls-private-key", "", "filename for the TLS key, in-memory generated if omitted")
+	cmd.Flags().BoolVar(&options.NoTLS, "no-tls", false, "do not attempt to read TLS certificates")


I think perhaps --insecure would be a better flag name?

richardcase · 2022-03-05T10:34:49Z

cmd/gitops-server/cmd/cmd.go

+	cmd.Flags().StringVar(&options.TLSCert, "tls-cert-file", "", "filename for the TLS certificate, in-memory generated if omitted")
+	cmd.Flags().StringVar(&options.TLSKey, "tls-private-key", "", "filename for the TLS key, in-memory generated if omitted")
+	cmd.Flags().BoolVar(&options.NoTLS, "no-tls", false, "do not attempt to read TLS certificates")
+


Should we also add a flag so that we can also enable client certificate validation as well?

^^ this is should be there 100%, in production use, I would never turn it off.

Edit: Is this the server side part only? If yes, it's a client thing, but there we should add a flag for it.

yitsushi · 2022-03-07T12:24:37Z

pkg/server/tls/tls.go

+	return tlsConfig, nil
+}
+
+func generateKeyPair(hosts []string) ([]byte, []byte, error) {


Potentially we can reuse an existing rootCA. On my local environment I have one rootCA,added that cert as trusted cert and i just generate keys per "site".

https://gist.github.com/yitsushi/2cd4ec3158c2f334d07f493da7f829e0

so basically the priv, err := part is not generated, it can come from a file.

Actually, I'm deleting this function as it is no longer relevant.

yitsushi · 2022-03-08T11:14:33Z

tools/validate-mkcert.sh

+if ! command -v mkcert &> /dev/null
+then
+    echo "mkcert is not installed, consider following this instructions: https://github.com/FiloSottile/mkcert#installation "
+    exit


exit 1 to indicate error, and make it fail if we use in a pipeline (like any other tasks that has tls-files as a dependency)

This is not supposed to run as part of a pipeline. But I would like to start using this patter as this could end up in a pipeline at some point.

richardcase · 2022-03-09T08:27:23Z

cmd/gitops-server/cmd/cmd.go

+		log.Println("TLS connections disabled")
+		return srv.ListenAndServe()
+	}
+


Given insecure is false here we should probably check that the options.TLSCertFile and options.TLSKeyFile are not empty. If they are empty then we should return an error.

And check that the files exist?

@richardcase You are right we should check that those flags are not empty. On the other hand, if the files do not exist we will get this type of errors already:

…icate, --tls-server-key

…create self-signed certificates.

# Conflicts: # cmd/gitops-server/cmd/cmd.go

…to match with the latest changes

* Added tls/ssl to api for gitops-server cmd * Added new flags to gitops-server cmd: --insecure, --tls-server-certificate, --tls-server-key * Fixed lint issues * Default to false on insecure flag * Added tls support for development. Also added instructions on how to create self-signed certificates. * Generate in-memory TLS keys # Conflicts: # cmd/gitops-server/cmd/cmd.go * Ignore testing tls files * Added make target to generate valid testing tls files when using tilt * Enforce mTLS by default * Use script to validate mkcert instead of doing it in the makefile * Return error status on validate-mkcert.sh * Disable mTLS by default * Error if certificate or key file are empty when insecure flag is false * Removed unused file wego-app.yaml and expose port 9001 on https link to match with the latest changes

There were quite a few conflicts, but many of them were very easy to resolve: anything related to acceptance tests has been deleted, lots of import conflicts, conficts in generated files, and some things that don't have a common ancestor and yet have almost the same content which makes git twitchy. There were a few more significant conflicts I've dealt with manually: * There's 2 independent implementations of https listeners (#1537, #1483). This also means it's been implemented in two places, `gitops run ui` and `gitops-server`. Keep the rate limiting from the main one, but otherwise mostly stick to the v2 one - it's been written to only work in docker. This means no more certificate generation code: v2 expects you to mount your certificates into the pod. TODO this deletes all gitops-server tests because they're all incompatible * The auth system in main has grown a username, which isn't present in v2. Add it to relevant files. * gitops profile upgrade is new. I've not really investigated how it works or if it still works, I've just adapted the API to the kind of changes we've done and checked that the tests pass. * There's been a lot of tilt rewrites in both branches. Since what you need to deploy the app and what it needs has changed in v2, I've mostly ignored main. * v2 and main disagreed on how to embed many svg images - v2 imported the images via filesystem path where they reside, while main imported them through images.ts. I've followed the v2 pattern. * Commit "Get auth working" (fdd15fb) in v2 rewrote how feature flags work, while main continued developing the old API. I've ported the new main code to the v2 API. * UserSettings had conflicts introduced in "Fix proxy port and user settings styling" (84bdc98) vs "WG529 Auth styling and flags improvements (#1624)" (f7098fb), I've tried to merge those. * Both main and v2 have turned a bunch of hard-coded auth secret name strings into constants. They've placed those constants in different places and given them different values. I gave up on trying to make this good and sensible and decided to just make everything compile and pass tests and start, and am looking forward to someone asking themselves "who would do this!?"

There were quite a few conflicts, but many of them were very easy to resolve: anything related to acceptance tests has been deleted, lots of import conflicts, conficts in generated files, and some things that don't have a common ancestor and yet have almost the same content which makes git twitchy. There were a few more significant conflicts I've dealt with manually: * There's 2 independent implementations of https listeners (#1537, #1483). This also means it's been implemented in two places, `gitops run ui` and `gitops-server`. Keep the rate limiting from the main one, but otherwise mostly stick to the v2 one - it's been written to only work in docker. This means no more certificate generation code: v2 expects you to mount your certificates into the pod. * The auth system in main has grown a username, which isn't present in v2. Add it to relevant files. * gitops profile upgrade is new. I've not really investigated how it works or if it still works, I've just adapted the API to the kind of changes we've done and checked that the tests pass. * There's been a lot of tilt rewrites in both branches. Since what you need to deploy the app and what it needs has changed in v2, I've mostly ignored main. * v2 and main disagreed on how to embed many svg images - v2 imported the images via filesystem path where they reside, while main imported them through images.ts. I've followed the v2 pattern. * Commit "Get auth working" (fdd15fb) in v2 rewrote how feature flags work, while main continued developing the old API. I've ported the new main code to the v2 API. * UserSettings had conflicts introduced in "Fix proxy port and user settings styling" (84bdc98) vs "WG529 Auth styling and flags improvements (#1624)" (f7098fb), I've tried to merge those. * Both main and v2 have turned a bunch of hard-coded auth secret name strings into constants. They've placed those constants in different places and given them different values. I gave up on trying to make this good and sensible and decided to just make everything compile and pass tests and start.

There were quite a few conflicts, but many of them were very easy to resolve: anything related to acceptance tests has been deleted, lots of import conflicts, conficts in generated files (which I've just re-generated and ignored), and some things that don't have a common ancestor and yet have almost the same content which makes git twitchy. There were a few more significant conflicts I've dealt with manually: * There's 2 independent implementations of https listeners (#1537, #1483). This also means it's been implemented in two places, `gitops run ui` and `gitops-server`. Keep the rate limiting from the main one, but otherwise mostly stick to the v2 one - it's been written to only work in docker. This means no more certificate generation code: v2 expects you to mount your certificates into the pod. * The auth system in main has grown a username, which isn't present in v2. Add it to relevant files. * gitops profile upgrade is new. I've not really investigated how it works or if it still works, I've just adapted the API to the kind of changes we've done and checked that the tests pass. * There's been a lot of tilt rewrites in both branches. Since what you need to deploy the app and what it needs has changed in v2, I've mostly ignored main. * v2 and main disagreed on how to embed many svg images - v2 imported the images via filesystem path where they reside, while main imported them through images.ts. I've followed the v2 pattern. * Commit "Get auth working" (fdd15fb) in v2 rewrote how feature flags work, while main continued developing the old API. I've ported the new main code to the v2 API. * UserSettings had conflicts introduced in "Fix proxy port and user settings styling" (84bdc98) vs "WG529 Auth styling and flags improvements (#1624)" (f7098fb), I've tried to merge those. * Both main and v2 have turned a bunch of hard-coded auth secret name strings into constants. They've placed those constants in different places and given them different values. I gave up on trying to make this good and sensible and decided to just make everything compile and pass tests and start.

josecordaz changed the base branch from main to v2 February 25, 2022 17:36

josecordaz force-pushed the tls-api branch 2 times, most recently from a07bee2 to b978b04 Compare February 25, 2022 19:01

josecordaz mentioned this pull request Mar 1, 2022

Ensure we use TLS when serving API #1499

Closed

josecordaz force-pushed the tls-api branch from c986bc0 to 85536a8 Compare March 2, 2022 18:20

josecordaz changed the title ~~Tls api~~ Added TLS security Mar 2, 2022

josecordaz marked this pull request as ready for review March 2, 2022 20:45

josecordaz requested review from richardcase and jpellizzari March 3, 2022 20:43

jpellizzari requested a review from luizbafilho March 4, 2022 15:49

jpellizzari approved these changes Mar 4, 2022

View reviewed changes

luizbafilho suggested changes Mar 4, 2022

View reviewed changes

richardcase reviewed Mar 5, 2022

View reviewed changes

yitsushi reviewed Mar 7, 2022

View reviewed changes

josecordaz force-pushed the tls-api branch from 946ab17 to fa6ace8 Compare March 7, 2022 21:22

josecordaz requested review from luizbafilho and richardcase March 7, 2022 21:22

yitsushi reviewed Mar 8, 2022

View reviewed changes

luizbafilho approved these changes Mar 8, 2022

View reviewed changes

yitsushi approved these changes Mar 9, 2022

View reviewed changes

richardcase reviewed Mar 9, 2022

View reviewed changes

josecordaz added 9 commits March 9, 2022 09:52

Added tls/ssl to api for gitops-server cmd

d058be0

Added new flags to gitops-server cmd: --insecure, --tls-server-certif…

558b11c

…icate, --tls-server-key

Fixed lint issues

5bf7b45

Default to false on insecure flag

5a6d61f

Added tls support for development. Also added instructions on how to …

c72da4e

…create self-signed certificates.

Generate in-memory TLS keys

d7636da

# Conflicts: # cmd/gitops-server/cmd/cmd.go

Ignore testing tls files

2a94630

Added make target to generate valid testing tls files when using tilt

49e3a21

Enforce mTLS by default

3bc01fa

josecordaz added 5 commits March 9, 2022 09:54

Use script to validate mkcert instead of doing it in the makefile

1c59f36

Return error status on validate-mkcert.sh

08410c1

Disable mTLS by default

1cb60ca

Error if certificate or key file are empty when insecure flag is false

b322bf0

Removed unused file wego-app.yaml and expose port 9001 on https link …

cccec76

…to match with the latest changes

josecordaz force-pushed the tls-api branch from 352c358 to cccec76 Compare March 9, 2022 18:01

josecordaz requested a review from richardcase March 9, 2022 18:32

josecordaz merged commit 9c8a40a into v2 Mar 10, 2022

josecordaz deleted the tls-api branch March 10, 2022 17:02

ozamosi mentioned this pull request Mar 14, 2022

Merge v2 into main #1707

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added TLS security #1537

Added TLS security #1537

josecordaz commented Feb 25, 2022 •

edited

jpellizzari Mar 4, 2022

josecordaz Mar 7, 2022

luizbafilho Mar 4, 2022

josecordaz Mar 7, 2022

luizbafilho Mar 7, 2022

josecordaz Mar 7, 2022

luizbafilho Mar 4, 2022

luizbafilho Mar 4, 2022

richardcase Mar 5, 2022

richardcase Mar 5, 2022

yitsushi Mar 7, 2022 •

edited

yitsushi Mar 7, 2022

josecordaz Mar 7, 2022 •

edited

yitsushi Mar 8, 2022

josecordaz Mar 8, 2022

richardcase Mar 9, 2022

richardcase Mar 9, 2022

josecordaz Mar 9, 2022

Added TLS security #1537

Added TLS security #1537

Conversation

josecordaz commented Feb 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yitsushi Mar 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josecordaz Mar 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josecordaz commented Feb 25, 2022 •

edited

yitsushi Mar 7, 2022 •

edited

josecordaz Mar 7, 2022 •

edited