⚠️ Serve catalog over HTTPS #263

trgeiger · 2024-05-06T16:49:55Z

This adds the ability to use HTTPS on the catalog server.

If the TLS cert and key filenames are provided via the relevant flags, then the server will be set to use HTTPS. If neither or only one of those options are provided, the server will use HTTP.

This also changes the http-external-address flag to just external-address, with the user no longer prepending the address with http or https.

Should I add a check for if the user does have http or https in the address, and adjust it to the proper one depending on the presence of the key/cert?
Should we add a duplicate flag --http-external-address for backwards compatibility and just map it to the new flag if used?

This PR also includes an overhaul of the organization of the manifests. Following the example of rukpak, cert-manager is used in the Makefile build targets so HTTPS is enabled in kind, but cert-manager is not a required dependency of catalogd. Instead, it's applied as an overlay which we target in the Makefile.

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>

This allows the use of alternate certificate managers. Signed-off-by: Tayler Geiger <tayler@redhat.com>

codecov · 2024-05-06T20:43:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.84%. Comparing base (250e348) to head (e7133a6).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #263   +/-   ##
=======================================
  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.

Fix a few manifest issues as well. Signed-off-by: Tayler Geiger <tayler@redhat.com>

cmd/manager/main.go

internal/third_party/server/server.go

cmd/manager/main.go

everettraven · 2024-05-09T15:02:52Z

config/base/rbac/role.yaml

+- apiGroups:
+  - ""
+  resources:
+  - pods


Out of scope for this PR, but seeing this I don't think we need these permissions on pods anymore. we don't use a pod to unpack the catalog contents anymore and probably forgot to come back and cleanup these permissions. I'll create an issue for this.

internal/serverutil/serverutil.go

joelanford · 2024-05-10T13:03:27Z

internal/serverutil/serverutil.go

+	"sigs.k8s.io/controller-runtime/pkg/certwatcher"
+)
+
+func ConfigureListener(cert string, key string, addr string, catalogAddr string, mgr ctrl.Manager) (net.Listener, string, error) {


As I mentioned in Slack, I didn't realize how complex the signature for this function would be when I suggested it. At this point, it seems like we might as well go one step further and setup the entire server as part of this function.

I think just adding the storeDir would give us all the info we need to do that?

Also some nit suggestions on naming and order of parameters.

Put the manager parameter first. That aligns with other functions that configure/add stuff to the manager.

Group storeDir and externalAddr together, since they are used to build the storage struct.

Use certFile and keyFile (instead of cert and key to make it more clear that they are the filenames of the cert and key, not the cert and key themselves)

Group listenAddr, certFile, and keyFile together since they are used to build the listener

Putting all that together, WDYT of this?

func AddCatalogServerToManager(mgr ctrl.Manager, storeDir, externalAddr, listenAddr, certFile, keyFile string) error

If we are concerned about the size of the function signature, it can probably be reduced to something like:

type CatalogServerConfig struct { ... } func AddCatalogServerToManager(mgr ctrl.Manager, cfg CatalogServerConfig) (net.Listener, string, error) { ... }

cmd/manager/main.go

internal/serverutil/serverutil.go

- Add error for missing either tls-key or tls-cert arguments. - Move server creation and configuration to serverutil Signed-off-by: Tayler Geiger <tayler@redhat.com>

everettraven

LGTM - thanks @trgeiger . I'll hold merging for a bit to allow @joelanford to take another pass

trgeiger · 2024-05-13T14:09:24Z

Thanks, Bryce. Should we adjust anything in the readme's quickstart instructions or anywhere else in docs?

everettraven · 2024-05-13T14:17:18Z

Thanks, Bryce. Should we adjust anything in the readme's quickstart instructions or anywhere else in docs?

Ah, great question (and good catch). We should probably update the quickstart instructions for port-forwarding and the curl command for fetching catalog contents to reflect the new state with an HTTPS based server as opposed to HTTP. I think this information is also present in https://github.com/operator-framework/catalogd/blob/main/docs/fetching-catalog-contents.md

trgeiger · 2024-05-13T15:12:48Z

How should we handle changing the instructions, given that HTTP is still supported if no TLS keys are provided, and the cert-manager is now an overlay. Do we just add that information as caveats to the instructions? Should I detail pulling down the cert and providing it to curl? etc.

everettraven · 2024-05-13T15:25:23Z

How should we handle changing the instructions, given that HTTP is still supported if no TLS keys are provided, and the cert-manager is now an overlay. Do we just add that information as caveats to the instructions? Should I detail pulling down the cert and providing it to curl? etc.

I'm not too familiar with how the overlays work, but IIUC the default deployment (i.e what is released) is going to be including the cert-manager resources and enable TLS (please correct me if I am wrong). If that is the default I would document whatever the default is.

I would not document pulling down the cert and providing it to curl and instead document with curl -k to ignore the certs. We can include a disclaimer in the docs that this is only for demonstration purposes and that if they want to trust the certificates to follow some other documentation on how to do that (I'm sure there is some documentation on this that already exists)

trgeiger · 2024-05-13T20:34:16Z

I made a couple small edits to README.md and https://github.com/operator-framework/catalogd/blob/main/docs/fetching-catalog-contents.md, let me know what you think

docs/fetching-catalog-contents.md

everettraven

A couple nits on the docs edits but I won't hold this PR on those. Great work @trgeiger !

trgeiger · 2024-05-14T14:42:13Z

Those were quick fixes, just made em so no need to hold off for another issue/pr for those.

everettraven and others added 3 commits May 6, 2024 09:58

make catalog server serve catalog contents over HTTPS

3b8522e

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

fix e2e

3791977

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

Reorganize manifests for cert-manager overlay

b298977

This allows the use of alternate certificate managers. Signed-off-by: Tayler Geiger <tayler@redhat.com>

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 May 6, 2024

trgeiger force-pushed the https-tayler branch 2 times, most recently from 30449a5 to 5428233 Compare May 6, 2024 20:41

This was referenced May 7, 2024

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

Closed

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

Closed

trgeiger force-pushed the https-tayler branch 2 times, most recently from c20a488 to d42ad79 Compare May 7, 2024 20:16

trgeiger marked this pull request as ready for review May 7, 2024 20:22

trgeiger requested a review from a team as a code owner May 7, 2024 20:22

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

Reconfigure TLS functionality to use Listener

d34081c

Fix a few manifest issues as well. Signed-off-by: Tayler Geiger <tayler@redhat.com>

trgeiger force-pushed the https-tayler branch from d42ad79 to d34081c Compare May 7, 2024 20:32

everettraven reviewed May 7, 2024

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

joelanford reviewed May 8, 2024

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

internal/third_party/server/server.go Show resolved Hide resolved

trgeiger mentioned this pull request May 8, 2024

Bump controller-runtime to v0.18.x #265

Open

trgeiger force-pushed the https-tayler branch 7 times, most recently from 81314f6 to 02e3232 Compare May 9, 2024 01:13

trgeiger changed the title ~~⚠ Serve catalog over HTTPS~~ ⚠️ Serve catalog over HTTPS May 9, 2024

everettraven reviewed May 9, 2024

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

everettraven reviewed May 9, 2024

View reviewed changes

everettraven mentioned this pull request May 9, 2024

Cleanup permissions for Pod resources that are no longer needed #266

Open

trgeiger force-pushed the https-tayler branch 2 times, most recently from 904d3d7 to 04cc7c0 Compare May 9, 2024 20:02

joelanford reviewed May 10, 2024

View reviewed changes

internal/serverutil/serverutil.go Outdated Show resolved Hide resolved

joelanford reviewed May 10, 2024

View reviewed changes

internal/serverutil/serverutil.go Outdated Show resolved Hide resolved

joelanford reviewed May 10, 2024

View reviewed changes

internal/serverutil/serverutil.go Outdated Show resolved Hide resolved

joelanford reviewed May 10, 2024

View reviewed changes

trgeiger force-pushed the https-tayler branch 2 times, most recently from 7577dc3 to fa4bb3c Compare May 10, 2024 16:51

trgeiger commented May 10, 2024

View reviewed changes

cmd/manager/main.go Show resolved Hide resolved

everettraven reviewed May 10, 2024

View reviewed changes

internal/serverutil/serverutil.go Outdated Show resolved Hide resolved

Add certwatcher for TLS cert and key from controller-runtime

8376ca4

- Add error for missing either tls-key or tls-cert arguments. - Move server creation and configuration to serverutil Signed-off-by: Tayler Geiger <tayler@redhat.com>

trgeiger force-pushed the https-tayler branch from fa4bb3c to 8376ca4 Compare May 10, 2024 18:52

everettraven previously approved these changes May 10, 2024

View reviewed changes

trgeiger dismissed everettraven’s stale review via 0d46314 May 13, 2024 20:33

everettraven reviewed May 14, 2024

View reviewed changes

docs/fetching-catalog-contents.md Outdated Show resolved Hide resolved

everettraven reviewed May 14, 2024

View reviewed changes

docs/fetching-catalog-contents.md Outdated Show resolved Hide resolved

everettraven previously approved these changes May 14, 2024

View reviewed changes

Update README and docs for HTTPS

e7133a6

trgeiger dismissed everettraven’s stale review via e7133a6 May 14, 2024 14:41

trgeiger force-pushed the https-tayler branch from 0d46314 to e7133a6 Compare May 14, 2024 14:41

everettraven approved these changes May 14, 2024

View reviewed changes

everettraven added this pull request to the merge queue May 14, 2024

Merged via the queue into operator-framework:main with commit cf384e4 May 14, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Serve catalog over HTTPS #263

⚠️ Serve catalog over HTTPS #263

trgeiger commented May 6, 2024 •

edited

Loading

codecov bot commented May 6, 2024 •

edited

Loading

everettraven May 9, 2024

joelanford May 10, 2024 •

edited

Loading

everettraven May 10, 2024

everettraven left a comment

trgeiger commented May 13, 2024

everettraven commented May 13, 2024

trgeiger commented May 13, 2024

everettraven commented May 13, 2024

trgeiger commented May 13, 2024

everettraven left a comment

trgeiger commented May 14, 2024

⚠️ Serve catalog over HTTPS #263

⚠️ Serve catalog over HTTPS #263

Conversation

trgeiger commented May 6, 2024 • edited Loading

codecov bot commented May 6, 2024 • edited Loading

Codecov Report

everettraven May 9, 2024

Choose a reason for hiding this comment

joelanford May 10, 2024 • edited Loading

Choose a reason for hiding this comment

everettraven May 10, 2024

Choose a reason for hiding this comment

everettraven left a comment

Choose a reason for hiding this comment

trgeiger commented May 13, 2024

everettraven commented May 13, 2024

trgeiger commented May 13, 2024

everettraven commented May 13, 2024

trgeiger commented May 13, 2024

everettraven left a comment

Choose a reason for hiding this comment

trgeiger commented May 14, 2024

trgeiger commented May 6, 2024 •

edited

Loading

codecov bot commented May 6, 2024 •

edited

Loading

joelanford May 10, 2024 •

edited

Loading