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

[Task] "Session credentials" mode seems unreliable for JMX connections #656

Closed
andrewazores opened this issue Nov 15, 2022 · 5 comments · Fixed by cryostatio/cryostat-legacy#1388
Assignees
Labels
bug Something isn't working

Comments

@andrewazores
Copy link
Member

Using "Session credentials" seems unreliable or intermittently fails when connecting to different JMX target applications or Cryostat itself. "Backend credentials" stored in the JMX keyring seem reliable, and the backend itests exercising the X-JMX-Authorization header (the mechanism behind Session credentials) seem solid, so this might just be a frontend error handling bug.

@andrewazores andrewazores added the bug Something isn't working label Nov 15, 2022
@tthvo tthvo self-assigned this Nov 18, 2022
@tthvo tthvo moved this to Todo in 2.3.0 release Nov 18, 2022
@tthvo tthvo moved this to Todo in 2.2.1 release Nov 18, 2022
@tthvo tthvo assigned maxcao13 and unassigned tthvo Nov 22, 2022
@maxcao13
Copy link
Member

This might be something we wait to fix until Cryostat 3.0, honestly. It seems to me the best way to handle this is to rearchitect a lot of Cryostat's internal infastructure to be scope to the lifetime of the request rather than having a static lifetime for the whole application. ie, there would be a new TargetConnectionManager instance created for each request that needs one, with direct access to the request context for retrieving X-JMX-Authorization header credentials, and a new ActiveRecordingHelper also scoped to the request lifecycle for the same reason, etc.

Reposting for visibility from #672 (comment)

What should the plan be then? It seems like the whole session storage might have to be scrapped until then...

@andrewazores
Copy link
Member Author

Well, there is one other hack we can apply in the meantime. I don't think I like it and don't take this as me committing to it.

In ex. AbstractAuthenticatedRequestHandler, or other implementations that handle the X-JMX-Authorization header, we can check for the header and if it's present, define new credentials into the keyring using that and the targetId/sourceTarget from the request URL path. If we do this then we can also add an endHandler onto the RoutingContext that will remove these credentials from the keyring when the request is completed.

This should make the X-JMX-Authorization header mechanism work again, but it is no longer a session-only, no-storage passthrough mechanism - the credentials would still end up briefly in the database before being deleted again. This also means that if clients are making different requests to the same target JVM with different JMX credentials for different JMX users that may exist then things can get messed up and interleaved unless we define all the endpoints as ordered so that Vert.x only processes them serially, but that kills throughput.

@maxcao13
Copy link
Member

maxcao13 commented Dec 9, 2022

Wondering, when is 2.2.1 slated to be released? I believe this feature should definitely be fixed in the bug fix release.

I definitely think there are some hacks that can be put in place until the big refactor like the one you mention, and I dont think it should be too difficult to put in.

@andrewazores
Copy link
Member Author

It's too late for 2.2.1, the merge window for that would've been a couple weeks back. The downstream release is in progress and should be delivered any day now.

@maxcao13 maxcao13 removed this from 2.2.1 release Dec 9, 2022
@andrewazores andrewazores moved this from Todo to Stretch Goals in 2.3.0 release Dec 16, 2022
@andrewazores andrewazores moved this to Stretch Goals in 2.3.0 release Feb 7, 2023
@andrewazores
Copy link
Member Author

I have a semi-hacky solution for this in mind that just might work. It's a totally normal concept for a web application but the way we're using worker thread pools and pushing work off between threads might break it. I'll open a draft PR soon so we can try hammering on it and testing it out to exercise the various API execution paths and see if the credentials get properly passed through the stack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants