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

feat(jmx): custom entrypoint to support copying TLS certs into truststore for JMX #330

Merged
merged 7 commits into from
Apr 18, 2024

Conversation

andrewazores
Copy link
Member

Welcome to Cryostat3! 👋

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 using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main


Based on #329
Related to #2
Related to #71

Description of the change:

Copies a simplified version of Cryostat 2.4's entrypoint.bash and truststore-setup.sh into the container build, sets up the container to initialize with the truststore copy and the entrypoint to copy in any additional user-supplied certificates into this truststore, before finally launching the server JVM.

Motivation for the change:

This allows users (the ones who deploy Cryostat, not the ones who interact with the UI and API) to supply TLS certificates that their target applications present for JMX connections. This restores Cryostat 3's ability to connect to such JMX/TLS-enabled targets.

How to manually test:

  1. mkdir truststore in the project directory
  2. Copy https://github.com/andrewazores/vertx-fib-demo/blob/master/src/main/extras/app/resources/vertx-fib-demo.cer into this new truststore
  3. See fix(jmxauth): tolerate JMX auth failures at discovery, re-attempt if new matching Credentials added #329. sample-app-3:9095 should now work as normal too. smoketest.bash will copy the local project directory truststore into a test volume and mount that to the Cryostat container. The Cryostat container should then see the new certificate and import it into its runtime truststore, then start the JVM which will now trust the certificate presented by sample-app-3.

Copy link

@andrewazores andrewazores marked this pull request as ready for review March 22, 2024 18:33
@andrewazores andrewazores requested a review from a team as a code owner March 22, 2024 18:33
@andrewazores
Copy link
Member Author

/build_test

Copy link

Workflow started at 3/22/2024, 2:34:06 PM. View Actions Run.

@andrewazores
Copy link
Member Author

@aali309 @mwangggg please review and ensure functionality works in your testing.

@ebaron this keeps the same filesystem conventions as in Cryostat 2.x in terms of where the truststore is expected to be mounted into the container. I think that's OK to keep in this PR, but if we have time in this release cycle it's probably something worth adjusting, too. WDYT?

Copy link

CI build and push: All tests pass ✅ (JDK21)
https://github.com/cryostatio/cryostat3/actions/runs/8394915475

Copy link

No OpenAPI schema changes detected.

Copy link

CI build and push: All tests pass ✅ (JDK17)
https://github.com/cryostatio/cryostat3/actions/runs/8394915475

@ebaron
Copy link
Member

ebaron commented Apr 5, 2024

@ebaron this keeps the same filesystem conventions as in Cryostat 2.x in terms of where the truststore is expected to be mounted into the container. I think that's OK to keep in this PR, but if we have time in this release cycle it's probably something worth adjusting, too. WDYT?

Sorry for the late reply. What sort of changes did you have in mind?

@andrewazores
Copy link
Member Author

Just changing some of the default paths:

https://github.com/cryostatio/cryostat3/pull/330/files#diff-e3747e19b9bb64cbff5eac47d57431f05f7edef025babe7c282566db82becf5aR34

Rather than /opt/cryostat.d and /truststore we could place things under /deployments (like what the rest of the application goes into), or at least move the truststore location inside of /opt somewhere so it's a bit more FHS-like.

@ebaron
Copy link
Member

ebaron commented Apr 5, 2024

Just changing some of the default paths:

https://github.com/cryostatio/cryostat3/pull/330/files#diff-e3747e19b9bb64cbff5eac47d57431f05f7edef025babe7c282566db82becf5aR34

Rather than /opt/cryostat.d and /truststore we could place things under /deployments (like what the rest of the application goes into), or at least move the truststore location inside of /opt somewhere so it's a bit more FHS-like.

Oh yeah, that should be fine to do. Will only require changing some paths on the operator and Helm chart.

@andrewazores
Copy link
Member Author

Cool. I'll file some issues so we can settle on what the paths should be and implement the changes some time later.

@andrewazores
Copy link
Member Author

I added a simple endpoint that walks the truststore directory and returns a list of the paths to files it finds in there. I figure this fits in well with the other changes, and relates to #306 as it gives us something to replace the old "JMX SSL cert upload" UX with - now we can have a simple "Target TLS certificates list" view instead that just renders this list as a table and tells the user that if they need to add more certs, they have to do it by adding them to the truststore and restarting the Cryostat server.

@andrewazores
Copy link
Member Author

/build_test

Copy link

Workflow started at 4/16/2024, 9:30:25 AM. View Actions Run.

Copy link

OpenAPI schema change detected:

diff --git a/schema/openapi.yaml b/schema/openapi.yaml
index b7db9ef..fcda791 100644
--- a/schema/openapi.yaml
+++ b/schema/openapi.yaml
@@ -2177,20 +2177,33 @@ paths:
                 type: object
           description: OK
         "401":
           description: Not Authorized
         "403":
           description: Not Allowed
       security:
         - SecurityScheme: []
       tags:
         - Reports
+  /api/v3/tls/certs:
+    get:
+      responses:
+        "200":
+          content:
+            application/json:
+              schema:
+                items:
+                  type: string
+                type: array
+          description: OK
+      tags:
+        - Trust Store
   /health:
     get:
       responses:
         "200":
           description: OK
       tags:
         - Health
   /health/liveness:
     get:
       responses:

Copy link

CI build and push: At least one test failed ❌ (JDK17)
https://github.com/cryostatio/cryostat3/actions/runs/8706545675

@andrewazores
Copy link
Member Author

/build_test

Copy link

Workflow started at 4/16/2024, 9:40:22 AM. View Actions Run.

Copy link

OpenAPI schema change detected:

diff --git a/schema/openapi.yaml b/schema/openapi.yaml
index b7db9ef..fcda791 100644
--- a/schema/openapi.yaml
+++ b/schema/openapi.yaml
@@ -2177,20 +2177,33 @@ paths:
                 type: object
           description: OK
         "401":
           description: Not Authorized
         "403":
           description: Not Allowed
       security:
         - SecurityScheme: []
       tags:
         - Reports
+  /api/v3/tls/certs:
+    get:
+      responses:
+        "200":
+          content:
+            application/json:
+              schema:
+                items:
+                  type: string
+                type: array
+          description: OK
+      tags:
+        - Trust Store
   /health:
     get:
       responses:
         "200":
           description: OK
       tags:
         - Health
   /health/liveness:
     get:
       responses:

Copy link

CI build and push: All tests pass ✅ (JDK17)
https://github.com/cryostatio/cryostat3/actions/runs/8706694552

@ebaron
Copy link
Member

ebaron commented Apr 18, 2024

@ebaron OK to merge this as-is? I filed separate issues to track changing the default file paths, if we decide to go for it this release.

Sounds good

@andrewazores
Copy link
Member Author

/build_test

Copy link

Workflow started at 4/18/2024, 5:12:07 PM. View Actions Run.

Copy link

No OpenAPI schema changes detected.

Copy link

CI build and push: All tests pass ✅ (JDK17)
https://github.com/cryostatio/cryostat3/actions/runs/8744745231

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request safe-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants