⚠️ Make catalog server serve catalog contents over HTTPS #243

everettraven · 2024-04-17T20:50:27Z

Description:

Updates the internal server.Server to have options for serving contents over HTTPS
Sets serving catalog contents over HTTPS, only configuration is the external https address, cert and key file
Adds cert-manager as a dependency and adds cert manager resources to the config to create the necessary self-signed certificates
Updates .goreleaser.yml to include cert-manager installation instructions since it is now a dependency again

adds cert-manager as a dependency again to create self-signed certs for the catalog server Signed-off-by: everettraven <everettraven@gmail.com>

Signed-off-by: everettraven <everettraven@gmail.com>

codecov · 2024-04-17T21:11:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.84%. Comparing base (0d07c30) to head (495903c).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #243   +/-   ##
=======================================
  Coverage   48.84%   48.84%           
=======================================
  Files           8        8           
  Lines         434      434           
=======================================
  Hits          212      212           
  Misses        201      201           
  Partials       21       21

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

grokspawn · 2024-04-24T12:23:56Z

test/e2e/unpack_test.go

@@ -93,7 +93,7 @@ var _ = Describe("Catalog Unpacking", func() {
 			// the ProxyGet() call below needs an explicit port value, so if
 			// value from url.Port() is empty, we assume port 80.


Suggested change

// value from url.Port() is empty, we assume port 80.

// value from url.Port() is empty, we assume port 443.

grokspawn · 2024-04-24T12:26:20Z

Looks like docs/fetching-catalog-contents.md, hack/scripts/demo-script.sh, README.md need updates for their port-forward instructions.

grokspawn · 2024-04-24T12:38:33Z

cmd/manager/main.go

@@ -77,9 +77,11 @@ func main() {
 		catalogdVersion      bool
 		systemNamespace      string
 		catalogServerAddr    string
-		httpExternalAddr     string
+		httpsExternalAddr    string


possible I'm getting wrapped around the axle of nomenclature, but the description says that this defaults to https, and given the name it seems that the primary focus is https and I'm not clear on how they would disable it. All the relevant flags have actionable default values.

I'm not certain that we need to have an http escape hatch here, but if there isn't one can you update the PR description to say that we're moving exclusively to secured connections?

Description updated

joelanford · 2024-04-24T14:12:34Z

internal/third_party/server/server.go

+		if s.ServeTLS {
+			return s.Server.ServeTLS(s.Listener, s.CertFile, s.KeyFile)
+		}


I finally got the upstream PR to make the controller-runtime HTTP server runnable exported, so I think we should plan to drop this package when there is a release containing it

With that in mind, I would suggest setting up TLS via an explicit s.Listener. That would also eliminate the slight weirdness with the server struct having CertFile and KeyFile fields that are inert when ServeTLS: false.

Do you mean always creating a listener in main.go and passing that into the Server creation, conditionally enabling TLS if the cert/key exist?

@trgeiger that is my understanding. I would also revert the changes that I made to the server.Server. I read the wrong section of the net/http package and realize now that the changes I made were unnecessary and passing in the listener is the "right" way to go here

Thanks, that clears that up for me

joelanford · 2024-04-24T14:15:59Z

config/default/kustomization.yaml

@@ -14,5 +14,5 @@ kind: Kustomization
 resources:
 - ../crd
 - ../rbac
+- ../certmanager


This will break downstream for Red Hat because they use a different certificate manager on OCP clusters.

I'd suggest setting things up similar to what we did in rukpak: https://github.com/operator-framework/rukpak/tree/main/manifests

tmshort · 2024-04-24T14:21:56Z

cmd/manager/main.go

@@ -89,10 +91,12 @@ func main() {
 			"Enabling this will ensure there is only one active controller manager.")
 	flag.StringVar(&systemNamespace, "system-namespace", "", "The namespace catalogd uses for internal state, configuration, and workloads")
 	flag.StringVar(&catalogServerAddr, "catalogs-server-addr", ":8083", "The address where the unpacked catalogs' content will be accessible")
-	flag.StringVar(&httpExternalAddr, "http-external-address", "http://catalogd-catalogserver.catalogd-system.svc", "The external address at which the http server is reachable.")
+	flag.StringVar(&httpsExternalAddr, "https-external-address", "https://catalogd-catalogserver.catalogd-system.svc", "The external address at which the http server is reachable.")


We are effectively removing a flag from this executable. Is this considered a breaking change? Should the http-external-address flag be kept but... (unsure what to do with it right now).

That being said, and in addition to @grokspawn's comment. Would it be possible for this to be set to an http:// address? Is that supported? Can we run either?

joelanford · 2024-04-24T14:21:59Z

cmd/manager/main.go

+	flag.StringVar(&certfile, "tls-cert", "/var/certs/tls.crt", "The certificate file used for serving catalog contents over HTTPS")
+	flag.StringVar(&keyfile, "tls-key", "/var/certs/tls.key", "The key file used for serving catalog contents over HTTPS")


These defaults could be annoying for running the binary locally outside of a container.

WDYT about:

Making these default to empty strings

If they are empty, run the server without TLS

If only one is set, fail

If both are set, load cert/key from the specified locations, and run with TLS

Also, do we need to think about what happens during a key rotation? We might need a file watcher to detect when the keys change.

What happens if an http:// address is specified?

Good point. I can't think of a good reason for this flag to carry a full URL with a scheme. Let's just make it --external-address=<ip>:<port>?

everettraven · 2024-04-24T15:00:02Z

Moving this to a draft until I have cycles to address review comments and make changes

everettraven · 2024-05-07T14:02:11Z

Closing in favor of @trgeiger 's PR (#263)

make catalog server serve catalog contents over HTTPS

8e771af

adds cert-manager as a dependency again to create self-signed certs for the catalog server Signed-off-by: everettraven <everettraven@gmail.com>

everettraven requested a review from a team as a code owner April 17, 2024 20:50

fix e2e

495903c

Signed-off-by: everettraven <everettraven@gmail.com>

grokspawn reviewed Apr 24, 2024

View reviewed changes

joelanford reviewed Apr 24, 2024

View reviewed changes

tmshort reviewed Apr 24, 2024

View reviewed changes

joelanford reviewed Apr 24, 2024

View reviewed changes

everettraven marked this pull request as draft April 24, 2024 15:00

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 24, 2024

everettraven mentioned this pull request May 2, 2024

[epic] Use encryption (HTTPS) for catalogd webserver responses #242

Open

3 tasks

everettraven closed this May 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Make catalog server serve catalog contents over HTTPS #243

⚠️ Make catalog server serve catalog contents over HTTPS #243

everettraven commented Apr 17, 2024 •

edited

codecov bot commented Apr 17, 2024 •

edited

grokspawn Apr 24, 2024

grokspawn commented Apr 24, 2024

grokspawn Apr 24, 2024 •

edited

everettraven Apr 24, 2024

joelanford Apr 24, 2024 •

edited

trgeiger May 2, 2024 •

edited

everettraven May 2, 2024

trgeiger May 2, 2024

joelanford Apr 24, 2024

tmshort Apr 24, 2024 •

edited

tmshort Apr 24, 2024

joelanford Apr 24, 2024

joelanford Apr 24, 2024

tmshort Apr 24, 2024

joelanford Apr 24, 2024

everettraven commented Apr 24, 2024

everettraven commented May 7, 2024

		@@ -93,7 +93,7 @@ var _ = Describe("Catalog Unpacking", func() {
		// the ProxyGet() call below needs an explicit port value, so if
		// value from url.Port() is empty, we assume port 80.

		flag.StringVar(&certfile, "tls-cert", "/var/certs/tls.crt", "The certificate file used for serving catalog contents over HTTPS")
		flag.StringVar(&keyfile, "tls-key", "/var/certs/tls.key", "The key file used for serving catalog contents over HTTPS")

⚠️ Make catalog server serve catalog contents over HTTPS #243

⚠️ Make catalog server serve catalog contents over HTTPS #243

Conversation

everettraven commented Apr 17, 2024 • edited

codecov bot commented Apr 17, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

grokspawn commented Apr 24, 2024

grokspawn Apr 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford Apr 24, 2024 • edited

Choose a reason for hiding this comment

trgeiger May 2, 2024 • 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

tmshort Apr 24, 2024 • 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

everettraven commented Apr 24, 2024

everettraven commented May 7, 2024

everettraven commented Apr 17, 2024 •

edited

codecov bot commented Apr 17, 2024 •

edited

grokspawn Apr 24, 2024 •

edited

joelanford Apr 24, 2024 •

edited

trgeiger May 2, 2024 •

edited

tmshort Apr 24, 2024 •

edited