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

test(sample): update sample application images and configs #928

Merged
merged 10 commits into from
Oct 25, 2024

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Aug 8, 2024

  • test(sample): update sample application image
  • test(sample): set up Agent sample application with new image and TLS config

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"

See cryostatio/test-applications#1
See cryostatio/test-applications#7
Related to cryostatio/cryostat-agent#139
Depends on cryostatio/cryostat-agent#491

Description of the change:

Updates the two existing sample app definitions for the newer split TLS truststore/keystore where the Agent's stores are now independent from the host application's stores. Also adds a new sample app definition for an Agent that uses the nginx TLS client auth proxy, rather than the standard OAuth proxy.

How to manually test:

  1. make sample_app sample_app_agent sample_app_agent_proxy and ensure all three cases become discovered and are interactable as usual.
  2. make undeploy_sample_app undeploy_sample_app_agent undeploy_sample_app_agent_proxy and ensure all three cases are correctly torn down and removed from discovery.

@andrewazores
Copy link
Member Author

andrewazores commented Aug 8, 2024

@mwangggg could you help figure out the new TLS setup to use here? Using the current configuration in this PR:

    env:
        - name: KEYSTORE_PASS
          valueFrom:
            secretKeyRef:
              key: KEYSTORE_PASS
              name: cryostat-sample-keystore
        - name: JAVA_OPTS_APPEND
          value: |-
            -Dquarkus.http.host=0.0.0.0
            -Djava.util.logging.manager=org.jboss.logmanager.LogManager
            -Dcom.sun.management.jmxremote.port=9097
            -Dcom.sun.management.jmxremote.ssl=false
            -Dcom.sun.management.jmxremote.authenticate=false
            -javaagent:/deployments/app/cryostat-agent.jar
            -Dcryostat.agent.webclient.tls.truststore.cert[0].path=/var/run/secrets/myapp/truststore.p12
            -Dcryostat.agent.webclient.tls.truststore.cert[0].type=X.509
            -Dcryostat.agent.webclient.tls.truststore.cert[0].alias=cryostat-sample
        image: quay.io/redhat-java-monitoring/quarkus-cryostat-agent:latest
...
        volumeMounts:
        - mountPath: /var/run/secrets/myapp/truststore.p12
          name: truststore
          subPath: truststore.p12
      volumes:
      - name: truststore
        secret:
          secretName: cryostat-sample-tls

I get an Agent startup failure with logs like this:

Starting the Java application using /opt/jboss/container/java/run/run-java.sh ...
INFO exec -a "java" java -XX:MaxRAMPercentage=80.0 -XX:+UseParallelGC -XX:MinHeapFreeRatio=10 -XX:MaxHeapFreeRatio=20 -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90 -XX:+ExitOnOutOfMemoryError -Dquarkus.http.host=0.0.0.0 -cp "." -jar /deployments/quarkus-run.jar 
INFO running in /deployments
2024-08-08 18:59:56:263 +0000 [cryostat-agent-main] INFO io.cryostat.agent.Agent - Cryostat Agent starting...
2024-08-08 18:59:57:961 +0000 [cryostat-agent-main] ERROR io.cryostat.agent.Agent - Agent startup failure
java.lang.RuntimeException: java.security.cert.CertificateParsingException: signed fields invalid
	at io.cryostat.agent.MainModule.provideClientSslContext(MainModule.java:249)
	at io.cryostat.agent.MainModule_ProvideClientSslContextFactory.provideClientSslContext(MainModule_ProvideClientSslContextFactory.java:54)
	at io.cryostat.agent.MainModule_ProvideClientSslContextFactory.get(MainModule_ProvideClientSslContextFactory.java:43)
	at io.cryostat.agent.MainModule_ProvideClientSslContextFactory.get(MainModule_ProvideClientSslContextFactory.java:13)
	at dagger.internal.DoubleCheck.get(DoubleCheck.java:47)
	at io.cryostat.agent.MainModule_ProvideHttpClientFactory.get(MainModule_ProvideHttpClientFactory.java:55)
	at io.cryostat.agent.MainModule_ProvideHttpClientFactory.get(MainModule_ProvideHttpClientFactory.java:14)
	at dagger.internal.DoubleCheck.get(DoubleCheck.java:47)
	at io.cryostat.agent.MainModule_ProvideCryostatClientFactory.get(MainModule_ProvideCryostatClientFactory.java:64)
	at io.cryostat.agent.MainModule_ProvideCryostatClientFactory.get(MainModule_ProvideCryostatClientFactory.java:15)
	at dagger.internal.DoubleCheck.get(DoubleCheck.java:47)
	at io.cryostat.agent.MainModule_ProvideRegistrationFactory.get(MainModule_ProvideRegistrationFactory.java:83)
	at io.cryostat.agent.MainModule_ProvideRegistrationFactory.get(MainModule_ProvideRegistrationFactory.java:13)
	at dagger.internal.DoubleCheck.get(DoubleCheck.java:47)
	at io.cryostat.agent.DaggerAgent_Client$ClientImpl.registration(DaggerAgent_Client.java:299)
	at io.cryostat.agent.Agent.accept(Agent.java:225)
	at io.cryostat.agent.Agent.lambda$agentmain$0(Agent.java:159)
	at java.base/java.lang.Thread.run(Thread.java:840)
Caused by: java.security.cert.CertificateParsingException: signed fields invalid
	at java.base/sun.security.x509.X509CertImpl.parse(X509CertImpl.java:1774)
	at java.base/sun.security.x509.X509CertImpl.<init>(X509CertImpl.java:178)
	at java.base/sun.security.provider.X509Factory.engineGenerateCertificate(X509Factory.java:102)
	at java.base/java.security.cert.CertificateFactory.generateCertificate(CertificateFactory.java:355)
	at io.cryostat.agent.MainModule.provideClientSslContext(MainModule.java:187)

I wonder if I just have the wrong alias, or something.

But also, the previous configuration used the KEYSTORE_PASS - the new configuration doesn't seem to have any way to use that. Maybe this is really the problem? I think the Secret is actually storing a whole truststore object, with that password required to read it. In this new implementation the Agent is programmatically creating its own new truststore, and then uses the configuration properties to import individual certificates into the truststore. So maybe we need to do something else here instead entirely?

@andrewazores
Copy link
Member Author

^ maybe the Agent should have some other config property to allow it to import a whole truststore as well? Like cryostat.agent.webclient.tls.truststore.path=/path/to/init/truststore, cryostat.agent.webclient.tls.truststore.pass=changeit. I think this could be used in conjunction with the new configs from cryostatio/cryostat-agent#448 - if these properties aren't specified then it does the same thing as cryostatio/cryostat-agent#448, but if they are specified then probably just this bit of code needs to be adjusted to start with the provided truststore?

@mwangggg
Copy link
Member

mwangggg commented Aug 8, 2024

We could do that, or we could add the cryostat's cert to the quarkus-cryostat-agent container with a volume mount instead of the "keystore" truststore volume mount

@mwangggg
Copy link
Member

mwangggg commented Aug 8, 2024

^I'll test that method out and see if it works- although giving users the option of importing a whole truststore is also a good idea.

@andrewazores
Copy link
Member Author

Ah, right, there's already a Secret that contains the bare cert we need, isn't there. That sounds like it should work well with your existing Agent work.

I think the init-truststore idea is still something worth considering - I can see that being useful for some users in the future. It'd be nice to allow specifying the whole truststore as a starting point instead of requiring users to "unwrap" it into its individual certs, if that isn't the form they already have available.

Copy link

github-actions bot commented Oct 4, 2024

This PR/issue depends on:

@andrewazores andrewazores marked this pull request as ready for review October 16, 2024 15:19
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@andrewazores andrewazores merged commit 0ec9a84 into cryostatio:main Oct 25, 2024
5 checks passed
@andrewazores andrewazores deleted the test-applications branch October 25, 2024 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants