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

Initial JMX authentication foundation #209

Merged
merged 17 commits into from
Jul 28, 2020

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Jul 16, 2020

Related to #5

See also cryostatio/cryostat-core#42

This PR lays down initial support for connections to targets using JMX access control/authentication. No mechanism for the user to supply JMX credentials is yet implemented, but connections to targets configured with JMX security will now fail with an identifiable exception, including an appropriate HTTP header for HTTP API requests or an appropriate status code for WebSocket API commands. HTTP API support for supplying credentials will be relatively simple - the AbstractAuthenticatedRequestHandler#getConnectionDescriptorFromContext method will simply be updated to check the Proxy-Authorization request header for Basic auth credentials to pass on to the target JMX connection. WebSocket API support for optional credentials will require more work (see: #205 for one proposed solution), or perhaps the WebSocket API will simply never gain JMX auth support and all such requests will need to go through the HTTP REST API instead.

Additionally, the ContainerJFR container image is now built with a custom entrypoint script, and the entrypoint by default enables JMX access control on the container's own remote JMX port, with an autogenerated password for the default "containerjfr" user. This password can be seen in the ContainerJFR instance's logs at startup time. This authentication (and the password generation) can be disabled for testing/development purposes. The SelfDisoveryPlatformClient is also removed and JDP autodiscovery is enabled instead so that the ContainerJFR instance can use JDP to discover itself within its container in a more uniform manner, providing itself with its own JMXServiceURL and Main-Class name. In environments where JDP autodiscovery is not used, ex. Kubernetes/OpenShift, this also has the net effect of preventing the self-connection list item from appearing in the discovered targets list.

@andrewazores andrewazores force-pushed the jmx-auth branch 3 times, most recently from 41f8047 to 2500246 Compare July 20, 2020 19:11
@andrewazores andrewazores requested a review from ebaron July 21, 2020 19:56
@andrewazores andrewazores marked this pull request as ready for review July 21, 2020 19:57
Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Andrew, sorry for the delay. Looks very good in general, just a few minor comments.

Beyond this, I think we might need something beyond scraping the password from the log for the operator setup. Maybe Container JFR could support credentials generated by the operator via environment variables or something?

@andrewazores
Copy link
Member Author

Yea, I wasn't sure what made the most sense from the Operator's POV, but that makes sense. If a password is provided via env var then use that, otherwise generate it as implemented here. Do you have any preference for how that env var is named?

@ebaron
Copy link
Member

ebaron commented Jul 28, 2020

Yea, I wasn't sure what made the most sense from the Operator's POV, but that makes sense. If a password is provided via env var then use that, otherwise generate it as implemented here. Do you have any preference for how that env var is named?

Maybe just CONTAINER_JFR_RJMX_{USER,PASS} or something? That can be in a future PR though, this one looks good!

@andrewazores andrewazores merged commit 6924d94 into cryostatio:main Jul 28, 2020
@andrewazores andrewazores deleted the jmx-auth branch July 28, 2020 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants