fix(credentials): store JMX session credentials in ThreadLocal

[andrewazores]

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 the last commit: git commit --amend --signoff

---

Fixes: cryostatio/cryostat-web#656

Description of the change:

This adds a ThreadLocal to the CredentialsManager containing a Map<String, Credentials>. The String is just the targetId of the current request, if any, and the Credentials are likewise the corresponding session credentials X-JMX-Authorization extracted from the API request context, if any. The CredentialsManager, and for now (needs refactor cleanup) the ConnectionDescriptor, reference this ThreadLocal to check for any session-provided credentials. If they are present then they are used and preferred over backend stored credentials. Once the current request is completed these credentials are cleared out of the ThreadLocal Map. This map is only expected to contain a singleton value currently and should only ever have one key corresponding to the current request's targetId URL parameter, but for testing purposes and request safety I used the Map to ensure credentials are not accidentally spilling between calls.

This concept is pretty standard in the "thread-per-request" model used by many webservers, where a new thread is spawned for each incoming client request, and that thread handles the entire execution lifespan, finally terminating after the response has been written. Here we use Vert.x which is reactive and processes requests using a main verticle thread and a worker pool. Most of our API handlers, in particular ones that may handle passthrough credentials, are handled by a worker in the Vert.x threadpool. Which exact thread handles the request is not predictable, but and depending on the exact execution path of that request, the thread may end up dropping the request and another thread (the same or another) may pick up the request later, if the request call stack contains something like vertx.executeBlocking() or targetConnectionManager.executeConnectedAsync(), either of which push the next unit of executable code into a work queue to be picked up by a worker thread. If the request execution is handed off between threads in this way then the session credentials held in the ThreadLocal will not be accessible and will appear to be null to the thread that picks up the request. If, however, all of these asynchronous delegation calls happen late in the call stack, ie after the point where the original thread has saved the session credentials to ThreadLocal and then picked them back up again for the last time, then this does not matter.

Motivation for the change:

This allows API requests using the old JMX passthrough authentication mechanism, the X-JMX-Authorization header, to work as expected again including for requests that trigger executions in the stack in other classes like the JvmIdHelper that do not have a direct call from the API handler that can pass through the RoutingContext/RequestParameters object directly so that the session credentials can be extracted at the call site. As explained above, if all of the requests are structured and executed in the right way, then the session credentials can be stored in a "global" ThreadLocal so that as the worker thread processes the request, the session credentials can be retrieved from any point in the call stack, without having to pass the RoutingContext object through as a method call argument the whole way through the stack. For reviewers reading this, this can be thought of as somewhat analogous to a React <Context> where pieces of state are available to descendant components anywhere below the provider in the component hierarchy. The net effect, if the above assumptions hold, is that the passthrough credentials mechanism works as expected again, without any significant refactoring of the internal interfaces to allow passing the RoutingContext through every layer of the call stack, and without resorting to temporary storage of session credentials in the encrypted keyring database.

How to manually test:

1. Run CRYOSTAT_IMAGE=quay.io... sh smoketest.sh...
2. Open the web-client or use curl/httpie and exercise as many API calls as possible, without using stored credentials. In the web-client this would mean deleting any previously stored credentials, going to /settings -> Advanced and setting the credentials storage to Session (Browser Memory). Use browser dev tools to inspect outgoing network requests for ex. active recordings queries and ensure that the X-JMX-Authorization credentials passthrough header is included in the request and that the request succeeds.

[andrewazores]

Still a draft since it needs more testing to ensure it really works as expected. Once we're confident that it does then I will go about refactoring and cleaning it up before final code review.

[github-actions]

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1388-260a3c6054b93c92f853bb42061bf3a8a76acb93 sh smoketest.sh

[andrewazores]

ex.

$ https :8181/api/v1/targets/localhost:9094/recordings
HTTP/1.1 401 Unauthorized
X-WWW-Authenticate: Basic
content-encoding: gzip
content-length: 83
content-type: text/plain

HTTP Authorization Failure caused by HttpException: Unauthorized

---

$ https --auth-type=basic --auth=user:pass :8181/api/v1/targets/localhost:9094/recordings
HTTP/1.1 427 Client Error (427)
X-JMX-Authenticate: Basic
content-encoding: gzip
content-length: 104
content-type: text/plain

JMX Authentication Failure caused by FailedLoginException: Invalid username or password

---

$ https --auth-type=basic --auth=user:pass :8181/api/v1/targets/localhost:9094/recordings X-JMX-Authorization:"Basic $(echo -n admin:adminpass123 | base64)"
HTTP/1.1 200 OK
content-encoding: gzip
content-length: 28
content-type: application/json

[]

[andrewazores]

A quick and clear reproducer for the bug is to use session credentials and then try to start a recording on a target that requires credentials, for example Cryostat itself in the smoketest scenario. Before this PR queries like listing active recordings or populating the Events page should work, but attempting to start a recording will fail and present the auth dialog again. With this PR it succeeds and the recording is created as expected.

[andrewazores]

Latest commit should work with GraphQL now. It still uses the same X-JMX-Authorization header with a single session credential value, which right now is just presumed valid for all of the targets that the GraphQL query might be executing against. Should be enough to prove the concept. Expanding that further by changing the header value format or adding a different header for multiple mapped targetId/credentials values should not be too hard and shouldn't affect the request execution data flow.

[github-actions]

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1388-b44b7ddf701da3f4fefbb9f5386d20a57e93c1a1 sh smoketest.sh

[github-actions]

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1388-bd3bab720714ad4fd2dd61d8694f00cc12bea0b8 sh smoketest.sh

[maxcao13]

Most operations now seem to work, looks very nice!

Just some things that probably still need fixing:

1. Automated rules with session stored credentials may need fixing. If my session has credentials for a jmx-authenticated target, starting an automated rule on it will not start a recording.
2. GraphQL jvmId getting. This does not work to get jvmIds:

http --auth-type=basic --auth=user:pass :8181/api/v2.2/graphql query=@conf/my-query1.graphql X-JMX-Authorization:"Basic $(echo -n admin:adminpass123 | base64)"

my-query1:

query {
    targetNodes {
        name
        target {
            jvmId
        }
    }
}

this results only vertx-fib-demo-1 returning a jvmId, even though we have passed in the jmx-auth header that applies to both demo-2 and demo-3.

[maxcao13]

But I think everything else looks really good and feels good to use as well. As far as my testing goes, I think we can move forward with this idea.

[andrewazores]

Automated rules with session stored credentials may need fixing. If my session has credentials for a jmx-authenticated target, starting an automated rule on it will not start a recording.

I think this is just going to stay broken, pretty much intentionally. The session credentials are supposed to only be used for the current client session, which in terms of the web-client means until you close the browser tab essentially, so activating rules based on session credentials would also imply deactivating rules when the session ends, and I don't think that's really a well-defined enough concept to try to support here. The backend only knows about request lifetimes anyway, not actual client sessions, so firing rule activations as soon as a valid credential is passed through a session credential header might seem a little haphazard IMO.

GraphQL jvmId getting. This does not work to get jvmIds:

Interesting. The GraphQL fetchers/mutators are wrapped in a piece of code that causes each stage to be performed asynchronously by the worker thread pool - I suspect the fact that targetNodes returns multiple values means that the next subquery is parallelized into the worker pool and so the ThreadLocal doesn't get correctly shared across them.

I'll see if I can figure a way around that.

[github-actions]

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1388-cc7444430db79dd4f6b29a47951d35e3c7bb9e0a sh smoketest.sh

[andrewazores]

Interesting. The GraphQL fetchers/mutators are wrapped in a piece of code that causes each stage to be performed asynchronously by the worker thread pool - I suspect the fact that targetNodes returns multiple values means that the next subquery is parallelized into the worker pool and so the ThreadLocal doesn't get correctly shared across them.

I think this is true but in fact the cause for the null IDs in the case you gave seems to be even simpler.

The query is looking for the jvmId field of the ServiceRef object which is fetched from the Discovery database at the beginning of the request. The request is already in progress and this object loaded out of the database and into memory by the time we check for session credentials, let alone see that they are present, connect to the target, compute the jvmId, and update the database entry.

The way around here is actually to query for the "live" JVM ID field that is nested under the mbeanMetrics query, this way the ID check is performed at the end of a query execution chain when we have a target connection open, rather than operating over stale data loaded from the database.

Maybe the direct serviceRef jvmId should be removed from the GraphQL schema in favour of this live mbeanMetrics version.

[github-actions]

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1388-e514c4339d3630a47c5b050bc74cc48719649244 sh smoketest.sh

[andrewazores]

query {
    targetNodes(filter: { annotations: ["PORT in (9094, 9095)"] }) {
        name
        target {
            jvmId
            annotations {
                cryostat
            }
        }
        mbeanMetrics {
            jvmId
        }
    }
}

[andrewazores]

I left the original ServiceRef jvmId in place because it's probably actually useful to have a way to retrieve it from the database, rather than requiring a JMX connection be opened to check it. We also already have integration tests that depend on that database query and the resulting null jvmId value for targets requiring credentials that aren't stored. This is just a caveat that should be mentioned somewhere. I guess in the GraphQL schema file makes sense?

[github-actions]

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1388-08fed21d382fb39976a9e5978f8a07c747af3559 sh smoketest.sh

[github-actions]

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1388-c119043fb3b18baee2ecac82d1ef7d944a9c43bb sh smoketest.sh

[github-actions]

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1388-34c9c6a7ef70d8f7db7194c69314a6bdf509e377 sh smoketest.sh