feat(agent): add nginx proxy for agent communication #957

ebaron · 2024-09-20T21:05:16Z

Welcome to Cryostat! 👋

Before contributing, make sure you have:

Read the contributing guidelines
Linked a relevant issue which this PR resolves
Linked any other relevant issues, PR's, or documentation, if any
Resolved all conflicts, if any
Rebased your branch PR on top of the latest upstream main branch
Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
Signed all commits: git commit -S -m "YOUR_COMMIT_MESSAGE"

Fixes: #952

Description of the change:

Adds an nginx container to the Cryostat deployment, which acts as a reverse proxy. Not exposed over an Ingress or Route.
When TLS using cert-manager is enabled, the proxy requires a client certificate signed by our CA to authenticate
Only selected Cryostat HTTP API endpoints are permitted and forwarded by the proxy. These are the endpoints required by the agent.

Motivation for the change:

Allows agents to communicate with Cryostat without using APIServer tokens. The tokens were merely a convenient way of authenticating, but don't work well for [Epic] Application annotations for Cryostat Agent integrations #784 without increasing the risk of these tokens being exposed to attackers.

How to manually test:

make create_cryostat_cr
make sample-app
SAMPLE_POD="$(kubectl get pod -l app=quarkus-test -o jsonpath='{$.items[0].metadata.name}'")

Add key/certs to pod

kubectl get secret cryostat-agent-c44e3ce7d8452282f4cf1ab14d08cfda2875fa727912e41595c6979bffe0f693 -o json | jq -r '.data["tls.key"] | @base64d' > /tmp/tls.key
kubectl get secret cryostat-agent-c44e3ce7d8452282f4cf1ab14d08cfda2875fa727912e41595c6979bffe0f693 -o json | jq -r '.data["tls.crt"] | @base64d' > /tmp/tls.crt
kubectl get secret cryostat-agent-c44e3ce7d8452282f4cf1ab14d08cfda2875fa727912e41595c6979bffe0f693 -o json | jq -r '.data["ca.crt"] | @base64d' > /tmp/ca.crt
kubectl cp /tmp/tls.key "${SAMPLE_POD}":/tmp/tls.key
kubectl cp /tmp/tls.crt "${SAMPLE_POD}":/tmp/tls.crt
kubectl cp /tmp/ca.crt "${SAMPLE_POD}":/tmp/ca.crt

Check the proxy

No client cert: kubectl exec "${SAMPLE_POD}" -- curl -sS --cacert /tmp/ca.crt -L https://cryostat-sample-agent.cryostat-operator-system.svc:8282/health

<html>
<head><title>400 No required SSL certificate was sent</title></head>
<body>
<center><h1>400 Bad Request</h1></center>
<center>No required SSL certificate was sent</center>
<hr><center>nginx/1.24.0</center>
</body>
</html>

With client cert: kubectl exec "${SAMPLE_POD}" -- curl -sS --key /tmp/tls.key --cert /tmp/tls.crt --cacert /tmp/ca.crt -L https://cryostat-sample-agent.cryostat-operator-system.svc:8282/health

{"cryostatVersion":"v4.0.0-snapshot","build":{"git":{"hash":"98a9d53519ceeef8eced0fd924b66d3a824b8340"}},"reportsConfigured":false,"reportsAvailable":true,"dashboardAvailable":true,"datasourceAvailable":true,"datasourceConfigured":true,"dashboardConfigured":true}

With bad path: kubectl exec "${SAMPLE_POD}" -- curl -sS --key /tmp/tls.key --cert /tmp/tls.crt --cacert /tmp/ca.crt -L https://cryostat-sample-agent.cryostat-operator-system.svc:8282/api/v1/recordings/
```
<html>
<head><title>404 Not Found</title></head>
<body>
<center><h1>404 Not Found</h1></center>
<hr><center>nginx/1.24.0</center>
</body>
</html>
```

Disable cert-manager

kubectl patch --type=merge -p '{"spec": {"enableCertManager": false}}' cryostat/cryostat-sample

No cert needed: kubectl exec "${SAMPLE_POD}" -- curl -sS -L http://cryostat-sample-agent.cryostat-operator-system.svc:8282/health

{"reportsConfigured":false,"build":{"git":{"hash":"85b43f9781d927debe0bcc909225cbd17f482c02"}},"cryostatVersion":"v4.0.0-snapshot","dashboardConfigured":true,"datasourceConfigured":true,"datasourceAvailable":true,"dashboardAvailable":true,"reportsAvailable":true}

Still restricts paths: kubectl exec "${SAMPLE_POD}" -- curl -sS -L http://cryostat-sample-agent.cryostat-operator-system.svc:8282/api/v1/recordings/

<html>
<head><title>404 Not Found</title></head>
<body>
<center><h1>404 Not Found</h1></center>
<hr><center>nginx/1.24.0</center>
</body>
</html>

Re-enable cert-manager

kubectl patch --type=merge -p '{"spec": {"enableCertManager": true}}' cryostat/cryostat-sample

Correct usage still works: kubectl exec "${SAMPLE_POD}" -- curl -sS --key /tmp/tls.key --cert /tmp/tls.crt --cacert /tmp/ca.crt -L https://cryostat-sample-agent.cryostat-operator-system.svc:8282/health

{"dashboardConfigured":true,"datasourceConfigured":true,"datasourceAvailable":true,"dashboardAvailable":true,"reportsAvailable":true,"reportsConfigured":false,"build":{"git":{"hash":"85b43f9781d927debe0bcc909225cbd17f482c02"}},"cryostatVersion":"v4.0.0-snapshot"}

HTTP doesn't work: kubectl exec "${SAMPLE_POD}" -- curl -sS -L http://cryostat-sample-agent.cryostat-operator-system.svc:8282/health

<html>
<head><title>400 The plain HTTP request was sent to HTTPS port</title></head>
<body>
<center><h1>400 Bad Request</h1></center>
<center>The plain HTTP request was sent to HTTPS port</center>
<hr><center>nginx/1.24.0</center>
</body>
</html>

Re-create the Cryostat CR (and its CA)

make create_cryostat_cr
kubectl exec "${SAMPLE_POD}" -- curl -sS --key /tmp/tls.key --cert /tmp/tls.crt -k -L https://cryostat-sample-agent.cryostat-operator-system.svc:8282/health

<html>
<head><title>400 The SSL certificate error</title></head>
<body>
<center><h1>400 Bad Request</h1></center>
<center>The SSL certificate error</center>
<hr><center>nginx/1.24.0</center>
</body>
</html>

ebaron · 2024-09-20T21:06:08Z

/build_test

ebaron · 2024-09-20T21:08:08Z

@andrewazores I think even after this is merged, we'll need some work on the agent to provide client certificates. I think the key/cert pair is used for the HTTP server, but not client.

github-actions · 2024-09-20T21:19:30Z

/build_test completed successfully ✅.
View Actions Run.

andrewazores · 2024-09-23T14:08:22Z

@andrewazores I think even after this is merged, we'll need some work on the agent to provide client certificates. I think the key/cert pair is used for the HTTP server, but not client.

Yes, I think you're right. All the work done in and around cryostatio/cryostat-agent#138 was just focused on getting the Cryostat server and agent to know how to get each of their HTTPS clients to trust the certificates presented by each of their own webservers, so there will need to be a bit more done so that the agent's client also knows how to pass a client certificate for authentication.

internal/controllers/configmaps.go

andrewazores · 2024-09-25T17:46:23Z

Does the ca.crt for the TLS proxy trust here work the same way as the equivalent server cert used for the auth proxies (or, more recently)? We already have a setup and some testing to ensure that the OAuth proxies can be configured to present TLS, and that the Agent can be configured to trust that same cert, so if this works the same way then that work should already be done.

So far in cryostatio/cryostat-agent#491 I have something working and tested at least for getting the Agent to send its client cert along to the new proxy as authentication, but I haven't checked that the ca.crt you describe in this PR can be trusted by the Agent client. I've just been telling the Agent HTTP client to trust all server certificates, assuming that this works the same way as it does for the other proxies.

ebaron · 2024-09-26T18:45:53Z

I may be misunderstanding the question, but the ca cert is the same self-signed CA used by all Cryostat components. So from the agent's perspective, the server CA and client CA are the exact same certificates.

andrewazores · 2024-09-26T18:54:09Z

I think that clears up most of what I'm unsure of (because I still don't fully understand how the Operator interacts with cert-manager).

So the two CAs are the same actual object, there are just two copies of it in two different Secrets (ex. cryostat-sample-tls and cryostat-sample-agent-tls). I think one is intended for the OAuth proxy and one for the TLS-auth proxy, but they are actually just the same thing then, right?

So then, the existing code and configuration options that we have for setting up the Agent truststore to be able to validate the OAuth proxy would not need any changes to validate the TLS-auth proxy. The (identical) ca.crt from the new Secret has to be mounted/copied in instead, and the existing configuration options would just be set to tell the Agent to load this one into its client truststore instead. Does that sound right?

andrewazores · 2024-09-26T19:02:59Z

I guess to make that a little more concrete:

IIRC the plan is for this Operator feature to work by doing something like watching for changes to Deployments (or Pods?), and checking if some "Cryostat Agent automatic" annotation is set. If it is, then the Operator will go ahead and edit the Deployment so that this cryostat-sample-agent-tls Secret gets mounted to it (providing the tls.crt, tls.key, and "new" ca.crt) into the container filesystem. Then the Agent is somehow configured and launched - maybe by setting environment variables to hook it in to the container as a -javaagent statically loaded Agent, or else by forking a process in the target container to exec and dynamically attach the Agent.

So that "somehow configured and launched" step is where the ca.crt question is, but I think I understand it now. It will hook in to the Agent's custom truststore the same way as how it already does it, since it's really the exact same thing - just provided in a second Secret so that everything can be mounted at once.

ebaron · 2024-09-26T19:05:12Z

That sounds right to me. cert-manager adds a copy of the CA certificate into each secret it creates to make a certificate chain. When using the agent auto-configuration, we'd set up the agent with cryostat.agent.webclient.tls.truststore.certs and cryostat.agent.webserver.tls.cert entries. These would reference the same certificate secret, since the cryostat-sample-agent-tls secret contains a private key, a certificate with both client and server key usages, and the CA certificate.

I was also thinking we may want to remove support for the agent to use the OAuth Proxy, IIRC, this would allow us to remove the openshift-delegate-urls arguments and allow us to remove some cluster level permissions for TokenReviews and SARs.

andrewazores · 2024-09-26T19:15:16Z

I was also thinking we may want to remove support for the agent to use the OAuth Proxy, IIRC, this would allow us to remove the openshift-delegate-urls arguments and allow us to remove some cluster level permissions for TokenReviews and SARs.

Hmm... the Agent does that by just passing the Authorization: Bearer abcd1234 header, so I think removing support on the OAuth proxy side means generally removing support for authentication/authorization using this mechanism. We don't directly use it anywhere else, but it can certainly be handy during development or for troubleshooting since this also allows us to use curl/HTTPie/etc. directly on the Route, or on the Service from another Pod within the cluster.

It'd still be possible to use curl using oc exec on one of the Cryostat Pods behind the proxy so that curl is talking directly to Cryostat, so it doesn't become impossible, just somewhat more complicated. This also depends on the assumption that one of those Pods will have curl (or a similar utility) in its base image, which may not be true forever.

I think it's worth considering anyway. Just wanted to highlight that removing that support doesn't affect only the Agent.

ebaron · 2024-10-04T17:42:05Z

/build_test

github-actions · 2024-10-04T17:54:25Z

/build_test : At least one test failed ❌.
View Actions Run.

andrewazores · 2024-10-04T17:57:48Z

Image:      ghcr.io/cryostatio/cryostat-operator-scorecard:pr-957-1bdfc3e61ab7fd4e7e7759f0007a09ad27efb057
Entrypoint: [cryostat-scorecard-tests cryostat-recording]
Labels:
	"suite":"cryostat"
	"test":"cryostat-recording"
Results:
	State: fail

	Errors:
		invalid character 'p' looking for beginning of value
	Log:
		panic: runtime error: invalid memory address or nil pointer dereference
		[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1646604]
		
		goroutine 1 [running]:
		github.com/cryostatio/cryostat-operator/internal/test/scorecard.(*RecordingClient).Create(0xc0001981b8, {0x1d0d010, 0x2a9aca0}, 0xc00017c7a0?, 0xc00010f9e0)
			/workspace/internal/test/scorecard/clients.go:338 +0x64
		github.com/cryostatio/cryostat-operator/internal/test/scorecard.CryostatRecordingTest(0x23?, {0xc000046044, 0x1b}, 0x0)
			/workspace/internal/test/scorecard/tests.go:200 +0xc05
		main.runTests({0xc0001b4010?, 0xa?, 0x1b?}, 0xc0000929c0, {0xc000046044, 0x1b}, 0x0)
			/workspace/internal/images/custom-scorecard-tests/main.go:129 +0x8aa
		main.main()
			/workspace/internal/images/custom-scorecard-tests/main.go:73 +0x458

This probably has to do with the V4 API changes.

ebaron · 2024-10-04T18:37:42Z

Image:      ghcr.io/cryostatio/cryostat-operator-scorecard:pr-957-1bdfc3e61ab7fd4e7e7759f0007a09ad27efb057
Entrypoint: [cryostat-scorecard-tests cryostat-recording]
Labels:
	"suite":"cryostat"
	"test":"cryostat-recording"
Results:
	State: fail

	Errors:
		invalid character 'p' looking for beginning of value
	Log:
		panic: runtime error: invalid memory address or nil pointer dereference
		[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1646604]
		
		goroutine 1 [running]:
		github.com/cryostatio/cryostat-operator/internal/test/scorecard.(*RecordingClient).Create(0xc0001981b8, {0x1d0d010, 0x2a9aca0}, 0xc00017c7a0?, 0xc00010f9e0)
			/workspace/internal/test/scorecard/clients.go:338 +0x64
		github.com/cryostatio/cryostat-operator/internal/test/scorecard.CryostatRecordingTest(0x23?, {0xc000046044, 0x1b}, 0x0)
			/workspace/internal/test/scorecard/tests.go:200 +0xc05
		main.runTests({0xc0001b4010?, 0xa?, 0x1b?}, 0xc0000929c0, {0xc000046044, 0x1b}, 0x0)
			/workspace/internal/images/custom-scorecard-tests/main.go:129 +0x8aa
		main.main()
			/workspace/internal/images/custom-scorecard-tests/main.go:73 +0x458

This probably has to do with the V4 API changes.

Looks to me like the target was nil. Does the create target response no longer look like this?

"data": {
  "result": {
    "id": 123,
    "connectUrl": "foo",
    "alias": "bar"
  }
}

andrewazores · 2024-10-04T18:43:50Z

That'd do it. The .data.result JSON layering format has been removed since it was never really useful in the way I intended back when I introduced it in 2.0, so it's gone in 4.0.

$ https -v --auth=user:pass :8443/api/v4/targets connectUrl=reports:11224 alias=reports
POST /api/v4/targets HTTP/1.1
Accept: application/json, */*;q=0.5
Accept-Encoding: gzip, deflate, br
Authorization: Basic dXNlcjpwYXNz
Connection: keep-alive
Content-Length: 51
Content-Type: application/json
Host: localhost:8443
User-Agent: HTTPie/3.2.2

{
    "alias": "reports",
    "connectUrl": "reports:11224"
}


HTTP/1.1 201 Created
Content-Length: 257
Content-Type: application/json;charset=UTF-8
Date: Fri, 04 Oct 2024 18:43:40 GMT
Gap-Auth: user
Location: https://localhost:8443/api/v4/targets/1

{
    "agent": false,
    "alias": "reports",
    "annotations": {
        "cryostat": [
            {
                "key": "REALM",
                "value": "Custom Targets"
            }
        ],
        "platform": []
    },
    "connectUrl": "service:jmx:rmi:///jndi/rmi://reports:11224/jmxrmi",
    "id": 1,
    "jvmId": "XaWFeOExJ5-CiPQioXregbBEf7SU9Rn5fgFOYhEIkXU=",
    "labels": []
}

ebaron · 2024-10-04T18:57:30Z

/build_test

github-actions · 2024-10-04T19:11:16Z

/build_test completed successfully ✅.
View Actions Run.

ebaron · 2024-10-04T19:16:33Z

@andrewazores looks like that last commit fixed it, mind approving again?

ebaron added 3 commits September 19, 2024 15:12

feat(agent): add nginx proxy for agent communication

b4dc2ac

Add/fix test cases

bf860bc

Allow proxy to work without TLS

f81fbf4

ebaron added the feat New feature or request label Sep 20, 2024

ebaron requested a review from a team September 20, 2024 21:05

ebaron changed the title ~~Nginx tls proxy~~ feat(agent): add nginx proxy for agent communication Sep 20, 2024

mergify bot added the safe-to-test label Sep 20, 2024

andrewazores reviewed Sep 23, 2024

View reviewed changes

internal/controllers/configmaps.go Outdated Show resolved Hide resolved

andrewazores mentioned this pull request Sep 23, 2024

[Task] Agent should be able to pass TLS client certificate for authentication to server (proxy) cryostatio/cryostat-agent#490

Closed

Add exact match location blocks to avoid 301

af7f01f

andrewazores mentioned this pull request Sep 25, 2024

feat(tls): agent can pass TLS client auth cert cryostatio/cryostat-agent#491

Merged

V4 API paths

fb58f1f

andrewazores previously approved these changes Oct 4, 2024

View reviewed changes

fix scorecard tests for v4 API changes

fa2e9cc

ebaron dismissed andrewazores’s stale review via fa2e9cc October 4, 2024 18:57

andrewazores approved these changes Oct 4, 2024

View reviewed changes

ebaron merged commit af55876 into cryostatio:main Oct 4, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(agent): add nginx proxy for agent communication #957

feat(agent): add nginx proxy for agent communication #957

ebaron commented Sep 20, 2024

ebaron commented Sep 20, 2024

ebaron commented Sep 20, 2024

github-actions bot commented Sep 20, 2024

andrewazores commented Sep 23, 2024

andrewazores commented Sep 25, 2024 •

edited

Loading

ebaron commented Sep 26, 2024

andrewazores commented Sep 26, 2024

andrewazores commented Sep 26, 2024

ebaron commented Sep 26, 2024

andrewazores commented Sep 26, 2024

ebaron commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

andrewazores commented Oct 4, 2024

ebaron commented Oct 4, 2024

andrewazores commented Oct 4, 2024 •

edited

Loading

ebaron commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

ebaron commented Oct 4, 2024

feat(agent): add nginx proxy for agent communication #957

feat(agent): add nginx proxy for agent communication #957

Conversation

ebaron commented Sep 20, 2024

Welcome to Cryostat! 👋

Before contributing, make sure you have:

Description of the change:

Motivation for the change:

How to manually test:

Add key/certs to pod

Check the proxy

Disable cert-manager

Re-enable cert-manager

Re-create the Cryostat CR (and its CA)

ebaron commented Sep 20, 2024

ebaron commented Sep 20, 2024

github-actions bot commented Sep 20, 2024

andrewazores commented Sep 23, 2024

andrewazores commented Sep 25, 2024 • edited Loading

ebaron commented Sep 26, 2024

andrewazores commented Sep 26, 2024

andrewazores commented Sep 26, 2024

ebaron commented Sep 26, 2024

andrewazores commented Sep 26, 2024

ebaron commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

andrewazores commented Oct 4, 2024

ebaron commented Oct 4, 2024

andrewazores commented Oct 4, 2024 • edited Loading

ebaron commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

ebaron commented Oct 4, 2024

andrewazores commented Sep 25, 2024 •

edited

Loading

andrewazores commented Oct 4, 2024 •

edited

Loading