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

OMDB should participate in authz #6600

Open
hawkw opened this issue Sep 18, 2024 · 2 comments
Open

OMDB should participate in authz #6600

hawkw opened this issue Sep 18, 2024 · 2 comments
Labels
authn Authentication development Bugs, paper cuts, feature requests, or other thoughts on making omicron development better good first issue Issues that are good for learning the codebase

Comments

@hawkw
Copy link
Member

hawkw commented Sep 18, 2024

Currently, OMDB uses the test user's authz actor, which does not have authorization to view a number of objects, such as instances that don't belong to it. This means that many OMDB commands currently run unauthenticated queries, either requiring OMDB commands to reimplement queries that exist in nexus-db-queries, or requiring nexus-db-queries to provide both authenticated and unauthenticated versions of its queries...which creates an opportunity to accidentally misuse the unauthenticated queries elsewhere.

Instead of having OMDB do an end-run around the entire authn/authz system in these cases, it would be much better to give it its own user that has the required access on all silos.

@hawkw hawkw added authn Authentication development Bugs, paper cuts, feature requests, or other thoughts on making omicron development better labels Sep 18, 2024
@davepacheco davepacheco added the good first issue Issues that are good for learning the codebase label Sep 18, 2024
@davepacheco
Copy link
Collaborator

More precisely: the test user that omdb currently uses is a type of silo user in the "default-silo" silo. They have "fleet admin", but that privilege doesn't grant users the ability to look at many resources inside other silos. (See #1681 for more context on this.) Any silo user would have this problem.

Assuming we do want to have this debugging tool that can access everything in the database (which seems reasonable at this point), I'd probably explore using a built-in user (rather than a silo user) to do this. This would involve:

  1. Adding a new built-in user USER_OMDB. It'll look at lot like USER_INTERNAL_API. We'll need to define it similar to that one.
  2. Add it to the test at the bottom of the file, too.
  3. Add a role assignment granting it fleet admin (again like USER_INTERNAL_API).
  4. Make sure it gets loaded by adding it to this array.
  5. Add a constant for it similar to USER_INTERNAL_API.
  6. Add a policy rule for it similar to USER_INTERNAL_API that grants it access to all silos.
  7. Change omdb to create an OpContext with this user.

You can find all this (and anything that I missed) by looking for USER_INTERNAL_API.

I would actually consider adding two users: one that's read-only and one that's read-write. Have omdb use the readonly one unless --destructive is passed. See #4805.

@hawkw
Copy link
Member Author

hawkw commented Sep 18, 2024

I may take a crack at wiring this up once I finish with waves hands around, but if someone else gets to it first, please feel free to!

last-genius added a commit to last-genius/omicron that referenced this issue Nov 11, 2024
As described and suggested in oxidecomputer#6600, create two built-in users to
replace the test user for OMDB's purposes. Follows USER_INTERNAL_API and its
read-only counterpart.

Signed-off-by: Andriy Sultanov <[email protected]>
last-genius added a commit to last-genius/omicron that referenced this issue Nov 13, 2024
As described and suggested in oxidecomputer#6600, create two built-in users to
replace the test user for OMDB's purposes. Follows USER_INTERNAL_API and its
read-only counterpart.

Signed-off-by: Andriy Sultanov <[email protected]>
last-genius added a commit to last-genius/omicron that referenced this issue Nov 13, 2024
As described and suggested in oxidecomputer#6600, create two built-in users to
replace the test user for OMDB's purposes. Follows USER_INTERNAL_API and its
read-only counterpart.

Signed-off-by: Andriy Sultanov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
authn Authentication development Bugs, paper cuts, feature requests, or other thoughts on making omicron development better good first issue Issues that are good for learning the codebase
Projects
None yet
Development

No branches or pull requests

2 participants