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): implement matchExpression-based credentials #1000

Merged

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Jun 16, 2022

Related to #895
Depends on cryostatio/cryostat-web#475

This PR enhances the existing CredentialsManager by replacing the targetId field of stored credentials with a matchExpression. This uses the same expression evaluation internal infrastructure already in place for Automated Rules, so the syntax and behaviour is exactly the same. There is a migration function that checks all stored credentials already on disk and checks if they have the targetId field. If they do not then they are skipped over, since they are either invalid files or conform to the new format already.

{
  "targetId": "foo",
  "credentials": {
    "username": "user",
    "password": "pass"
  }
}

is migrated by applying a simple transformation to:

{
  "matchExpression": "target.connectUrl == \"foo\"",
  "credentials": {
    "username": "user",
    "password": "pass"
  }
}

This allows the new expression-based system to migrate existing credentials while maintaining the same semantics, and also allows the old credentials API endpoints to continue working with the same behaviour.

Still TODO in follow-up PRs:

  1. Add new API endpoints to allow clients to define and delete credentials with generalized matchExpression, not only the backward-compatible target.connectUrl == "foo" style
  2. Use the new CredentialsGetHandler on the frontend to enhance the Security view to better reflect these expression-based credentials [Story] Enhance JMX Credentials UI for matchExpressions cryostat-web#465

Since it is now possible for one set of credentials to match multiple targets, the AbstractAuthenticatedRequestHandler no longer deletes credentials if they are used for a JMX connection and the connection fails - since the credentials may apply to multiple targets, they may be valid for other targets than the one that was just checked.

It is also possible that one target may be matched by multiple sets of credentials. There is no protection against this case, so it would simply be considered a user configuration error. If this occurs then the credentialsManager will simply provide the "first" matching set of credentials for a given target when requested. The ordering of credentials is intentionally undefined at this time. Multiple credentials for a given target is not an expected valid configuration and so no scheme of trying each of them in order is attempted, or any other more complex behaviours.

@andrewazores andrewazores force-pushed the credentials-matchexpression branch 3 times, most recently from 0cabc68 to 1e15090 Compare June 16, 2022 14:06
@andrewazores andrewazores added feat New feature or request needs-documentation labels Jun 16, 2022
@andrewazores andrewazores force-pushed the credentials-matchexpression branch 3 times, most recently from dea8979 to acfc209 Compare June 16, 2022 14:53
@andrewazores andrewazores force-pushed the credentials-matchexpression branch from acfc209 to 73d2f3b Compare June 16, 2022 15:15
@andrewazores andrewazores marked this pull request as ready for review June 16, 2022 17:24
@andrewazores andrewazores requested review from jan-law and hareetd June 16, 2022 17:24
jan-law
jan-law previously approved these changes Jun 16, 2022
Copy link
Contributor

@jan-law jan-law left a comment

Choose a reason for hiding this comment

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

Works well

@andrewazores andrewazores force-pushed the credentials-matchexpression branch from 66b0050 to e22452b Compare June 16, 2022 20:54
@andrewazores
Copy link
Member Author

Actually, hold on. I'll add unit tests for the CredentialsManager. It probably should have had them before and it certainly should now.

@andrewazores
Copy link
Member Author

Not done adding tests yet, just added basic tests for the new migration function so far. I'll add some more tomorrow.

@andrewazores andrewazores force-pushed the credentials-matchexpression branch from 0061b21 to f635baa Compare June 17, 2022 18:42
Copy link
Contributor

@hareetd hareetd left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the tests yet but the implementation changes look good. However, since the credentials-related notifications have been changed to accommodate the match expression change, the StoreJmxCredentials component (on -web) no longer updates the table state properly when credentials are added or deleted.

@andrewazores
Copy link
Member Author

Yea, I'll fix that with TODO 2 listed in the PR body.

@andrewazores
Copy link
Member Author

Filing the -web side to do later, since I'm on PTO next week and won't have any time to get started on it. I'll leave it open for anyone else to take a stab at if there's interest, otherwise I'll start on it when I'm back to work.

hareetd
hareetd previously approved these changes Jun 17, 2022
@jan-law
Copy link
Contributor

jan-law commented Jun 17, 2022

Backend looks good, I'm assuming you'll merge this once the web client gets updated

@andrewazores
Copy link
Member Author

-web PR opened that just patches the UI enough to not break with this backend change. The actual UI enhancements like using a nested table view and allowing users to create credentials definitions using a matchExpression need to wait for more follow-up backend changes adding API for that.

@andrewazores andrewazores force-pushed the credentials-matchexpression branch from 16272a1 to 3169609 Compare June 29, 2022 18:17
@github-actions
Copy link
Contributor

This PR/issue depends on:

@andrewazores
Copy link
Member Author

-web side merged and updated in this feature branch. Nothing else has changed since previous review approvals.

Copy link
Contributor

@hareetd hareetd left a comment

Choose a reason for hiding this comment

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

Oops, looks like the web-client submodule hasn't been updated with the frontend changes (I don't see it in the changed files).

@andrewazores
Copy link
Member Author

It isn't listed as a commit in this PR since I rebased on top, but the latest frontend change in the submodule is already in main thanks to our new CI-bot: cryostatio/cryostat@d4b448c

@hareetd
Copy link
Contributor

hareetd commented Jun 29, 2022

Oh that makes sense then, cool.

@andrewazores
Copy link
Member Author

No more manually updating the submodule :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants