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(credentials): add credentials test endpoint #1432

Merged
merged 8 commits into from
Mar 28, 2023

Conversation

maxcao13
Copy link
Member

@maxcao13 maxcao13 commented Mar 27, 2023

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: #1431

Description of the change:

Adds an endpoint to specifically test JMX connection w/credentials to a target JVM at /beta/credentials/:targetId

The endpoint from the backend should query these targets themselves with the supplied credentials, and give a resulting output as the response body.

They should return status=NA for "this target does not need credentials, status=SUCCESS, this target is authenticated with these supplied credentials, and status=FAILURE, which means these credentials can not be authenticated with these credentials.

Motivation for the change:

See #1431

How to manually test:

  1. Run with smoketest setup.
  2. Try these queries:
http --form --auth=user:pass POST localhost:8181/api/beta/credentials/service%3Ajmx%3Armi%3A%2F%2F%2Fjndi%2Frmi%3A%2F%2Flocalhost%3A9095%2Fjmxrmi/ username="admin" password="adminpass123"

Try on target that doesn't need credentials as well: e.g. 9093 vertx-fib-demo

@maxcao13 maxcao13 added the feat New feature or request label Mar 27, 2023
@maxcao13 maxcao13 requested review from andrewazores and tthvo March 27, 2023 21:30
@mergify mergify bot added the safe-to-test label Mar 27, 2023
@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1432-2c0146c3c7b806d1cfd656420c33afaea5818dc8 sh smoketest.sh

Copy link
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

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

Looks good to me! Any tests planned?

@maxcao13
Copy link
Member Author

Right, I will write some tests, almost forgot.

@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1432-5c76e194a8be1d50fb230f57dc8b07559c2f0377 sh smoketest.sh

@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1432-536f8584b49e6bdc6ec54c7bae98f031ba625ca3 sh smoketest.sh

tthvo
tthvo previously approved these changes Mar 27, 2023
Copy link
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Signed-off-by: Max Cao <macao@redhat.com>
Signed-off-by: Max Cao <macao@redhat.com>
Signed-off-by: Max Cao <macao@redhat.com>
Signed-off-by: Max Cao <macao@redhat.com>
Signed-off-by: Max Cao <macao@redhat.com>
Signed-off-by: Max Cao <macao@redhat.com>
Signed-off-by: Max Cao <macao@redhat.com>
@maxcao13
Copy link
Member Author

I've updated the pr.

I believe the failure comes from here https://github.com/cryostatio/cryostat/blob/main/src/main/java/io/cryostat/net/AgentClient.java#L234

It seems the AgentConnection connect, throws ConnectionException which cannot be used to figure out whether the nested exception in the throwable chain was a specific exception, so instead I use the exception message and catching the ConnectionException within the CredentialTest handler.

@maxcao13
Copy link
Member Author

I'm not sure what should happen with agent testing credentials though. As of now, if the credentials the agent uses are deleted, any tests will result in an invalid credentials message, which makes sense. But if agent credentials exist in the backend, the credentials test will end up as N/A, because the AgentClient invoke method will always use backend credentials if they exist.

Do these interactions make sense?

@andrewazores
Copy link
Member

I've updated the pr.

I believe the failure comes from here https://github.com/cryostatio/cryostat/blob/main/src/main/java/io/cryostat/net/AgentClient.java#L234

It seems the AgentConnection connect, throws ConnectionException which cannot be used to figure out whether the nested exception in the throwable chain was a specific exception, so instead I use the exception message and catching the ConnectionException within the CredentialTest handler.

I think adding a nullcheck at line 232 there makes sense. This still probably just ends up in a ConnectionException but at least it isn't rooted on a NullPointerException.

I'm not sure what should happen with agent testing credentials though. As of now, if the credentials the agent uses are deleted, any tests will result in an invalid credentials message, which makes sense. But if agent credentials exist in the backend, the credentials test will end up as N/A, because the AgentClient invoke method will always use backend credentials if they exist.
Do these interactions make sense?

Sure, it makes enough sense for now. It's a bit late to do for 2.3, but early in the next development cycle I think it would be good to tackle this and make the AgentClient act a bit more like the AbstractAuthenticatedRequestHandler in how credentials are picked up and used.

@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1432-97f87ccd7d08b21d3770b22ab5d7fda0509bbf67 sh smoketest.sh

@andrewazores andrewazores merged commit 3f4ef0f into cryostatio:main Mar 28, 2023
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
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Request] Credential test endpoint
3 participants