add built-in users to the database #486

davepacheco · 2021-12-06T20:40:54Z

See #419 for background. This change takes the predefined users that we use in the test suite and elsewhere and moves them into the database. This doesn't change much practically but it makes the built-in users look more like we expect things to look once we've integrated with some identity provider.

davepacheco · 2021-12-06T20:45:22Z

nexus/src/db/error.rs

+/// Converts a Diesel pool error to an external error for a case where we do not
+/// expect the query to fail and the error cannot be easily summarized for an
+/// end user
+pub fn public_error_from_diesel_pool_shouldnt_fail(


@smklein FYI I refactored the functions here a bit so I could handle things a bit differently during Nexus startup.

Nice, this eliminates the need for specifying the resource type. I know I'm late here, but I think there are a handful of spots that could use this for internal operations.

Yes, I think you're right!

common/src/sql/dbinit.sql

teisenbe · 2021-12-06T23:08:20Z

common/src/sql/dbinit.sql

+    time_modified
+) VALUES (
+    /* NOTE: this uuid and name are duplicated in nexus::authn. */
+    '001de000-05e4-0000-0000-000000000001',


I think this UUID isn't strictly valid, though I don't think it's likely to bite us unless a UUID lib is trying to interpret the value

Yeah, all of the built-in users have made-up uuids described in the comment in authn.rs. I believe they all parse with uuid::Uuid::from_str, though. When you say it's not strictly valid, did you mean that it's not consistent with any of the v1-v4 specs?

yeah, and it's in the legacy variant 0 (NCS) space, which has its own subfields

Yeah, that's true. How about I change all the built-in user uuids to v4 variant 1 (but otherwise keep their structure the same)? Another option would be to stick with variant 0 (which appears obsolete) or maybe variant 2 (which appears irrelevant to us), but I don't think there's any particular reason to do that. When I wrote up this scheme, I was definitely thinking of them as v4 ones.

Done in 539e538.

teisenbe · 2021-12-06T23:44:09Z

nexus/src/db/datastore.rs

+        name: &Name,
+    ) -> LookupResult<UserBuiltin> {
+        use db::schema::user_builtin::dsl;
+        opctx.authorize(authz::Action::ListChildren, authz::FLEET)?;


Should this be Action::Read?

I don't think so? Check out

omicron/nexus/src/authz/omicron.polar

Lines 41 to 80 in 038dbd0

#

# Permissions and predefined roles

#

# For now, we define the following permissions for most resources in the system:

#

# - "create_child": required to create child resources.

#

# - "list_children": required to list children (of all types) of a resources

#

# - "modify": required to modify or delete a resource or any of its children

#

# - "read": required to read a resource

#

# We define the following predefined roles for only a few high-level resources:

#

# - "admin": has all permissions on the resource

#

# - "collaborator": has "list_children" and "create_$child" for all children.

# They'll inherit the "admin" role for any resources that they create.

#

# - "viewer": has "read" and "list_children" on a resource

#

# Below the project level, permissions are granted at the Project level. For

# example, for someone to be able to create, modify, or delete any Instances,

# they must be granted project.collaborator, which means they can create,

# modify, or delete _all_ resources in the Project.

#

# The complete set of predefined roles:

#

# - fleet.admin (superuser for the whole system)

# - fleet.collaborator (can create and own orgs)

# - organization.admin (complete control over an organization)

# - organization.collaborator (can create, modify, and delete projects)

# - project.admin (complete control over a project)

# - project.collaborator (can create, modify, and delete all resources within

# the project, but cannot modify or delete the project

# itself)

# - project.viewer (can see everything in the project, but cannot modify

# anything)

#

. I chose to treat these as children of the top-level Fleet. Let me know what you think.

From these descriptions it still sounds like Read to me? Unless we consider resolving a single object by name to be a listing operation

Oh, I see your point.

What resource would the "read" action be applied to? I assumed at first that you meant that we'd create a new Polar resource for UserBuiltin as a child of Fleet. That's totally reasonable. I elected not do that only because it's a bunch more plumbing and it wasn't clear to me that it was useful to be able to assign permissions on builtin users. Admittedly, it's kind of implicit to piggy-back on the fleet-level list-children permission.

The policy currently doesn't say anything about most system-wide resources, including built-in users and all the hardware resources that we'll eventually have. Until we have reason to think that people are going to want to split these up, I'd propose treating these similar to ProjectResource today: we could create one Polar type for SystemResource with no roles of its own, but permissions granted based on one's roles on the Fleet. It's a little hollow because unlike ProjectResource, there's only one possible parent (so there would be no state in a SystemResource struct). But it might be more explicit. What do you think?

Another option is to keep the resource in this check as Fleet, treating the users and other system-wide information as part of the Fleet itself. I think that's not great because I can imagine even pretty unprivileged users getting "read" permission on the Fleet to be able to view system-wide configuration or something, but that doesn't mean they should be able to see all system-wide resources.

I like the middle ground of creating one SystemResource for all these things to share. I agree with Tess than using a list children permission to read a single thing is weird.

Done in 9f562e9 -- let me know what you think.

teisenbe · 2021-12-06T23:51:25Z

nexus/src/db/datastore.rs

+            &*authn::USER_TEST_PRIVILEGED,
+            &*authn::USER_TEST_UNPRIVILEGED,


Do we want to conditionally include these?

I think you're right that at some point, we'll want to leave them out of some configurations. But right now, USER_TEST_PRIVILEGED is necessary to do anything useful with oxapi_demo or the console, even if you're not in #[cfg(test)]. Once we add authz to the rest of the endpoints, you really won't be able to do anything useful with Nexus without these. We will need to revisit this as part of initial rack setup, I think.

On console we use a DB seeding script for local dev and for the QA instance deployed to GCP. Maybe these users could eventually move into something like that rather than a #[cfg(test)]. Or both, actually. #[cfg(test)] for the tests and a script for local dev.

It might make sense to add a comment/create an appropriately tagged issue to track that?

@teisenbe Sure thing. I had a TODO-security for this (which I'm hoping we'll audit before we ship...but we don't track that either), but it got moved around and no longer really covers it. Moved and clarified in af85c83.

@david-crespo Longer-term, another option would be to officially support local users and add "create" endpoints for them. Maybe these would be better modeled as service accounts? Then we'd just have the test suite create the ones it wants. For developers playing around, they could create one with oxapi_demo. But...with what creds would they create one? Anyway I'd definitely like to move that discussion to a follow-on PR.

davepacheco · 2021-12-07T00:39:48Z

Thanks for taking a look @teisenbe!

david-crespo

looks great!

david-crespo · 2021-12-07T16:40:02Z

nexus/src/external_api/console_api.rs

-        "privileged" => Some(TEST_USER_UUID_PRIVILEGED.parse().unwrap()),
-        "unprivileged" => Some(TEST_USER_UUID_UNPRIVILEGED.parse().unwrap()),
+        "privileged" => Some(USER_TEST_PRIVILEGED.id),
+        "unprivileged" => Some(USER_TEST_UNPRIVILEGED.id),


that was easy

davepacheco added 16 commits November 23, 2021 10:57

looking for help

a45e261

more help -- compiler error

34806ad

more work

32ee49d

rename test users to match convention

94e8dbe

Merge branch 'main' into authz-users

34b6d6e

fix mismerge and update nexus.json

d42866d

Merge branch 'main' into authz-users

7508ebe

user names should be constants

66d5d47

fix comment

516fda9

a better way to wait

877823d

remove xxx

bfb7a07

some tests

be5bc94

rename predefined -> built-in

e79f76a

Merge branch 'main' into authz-users

a9bb8d3

built-in users could be more structured

f986fc0

fix format

b19c6e9

davepacheco mentioned this pull request Dec 6, 2021

tracking issue for authz work #419

Closed

71 tasks

davepacheco commented Dec 6, 2021

View reviewed changes

davepacheco marked this pull request as ready for review December 6, 2021 20:45

davepacheco requested a review from teisenbe December 6, 2021 20:45

teisenbe reviewed Dec 6, 2021

View reviewed changes

davepacheco added 2 commits December 6, 2021 16:41

fix typo

b10ec68

fix TODO

af85c83

david-crespo approved these changes Dec 7, 2021

View reviewed changes

davepacheco added 2 commits December 7, 2021 08:59

add more first-class FleetChild

9f562e9

built-in uuids could be valid v4 uuids

539e538

teisenbe approved these changes Dec 7, 2021

View reviewed changes

davepacheco merged commit 66d1be0 into main Dec 7, 2021

davepacheco deleted the authz-users branch December 7, 2021 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add built-in users to the database #486

add built-in users to the database #486

davepacheco commented Dec 6, 2021

davepacheco Dec 6, 2021

smklein Dec 7, 2021

davepacheco Dec 7, 2021

teisenbe Dec 6, 2021

davepacheco Dec 7, 2021

teisenbe Dec 7, 2021 •

edited

Loading

davepacheco Dec 7, 2021

davepacheco Dec 7, 2021

teisenbe Dec 6, 2021

davepacheco Dec 7, 2021

teisenbe Dec 7, 2021

davepacheco Dec 7, 2021

david-crespo Dec 7, 2021

davepacheco Dec 7, 2021

teisenbe Dec 6, 2021

davepacheco Dec 7, 2021

david-crespo Dec 7, 2021 •

edited

Loading

teisenbe Dec 7, 2021

davepacheco Dec 7, 2021

davepacheco commented Dec 7, 2021

david-crespo left a comment

david-crespo Dec 7, 2021

	#
	# Permissions and predefined roles
	#
	# For now, we define the following permissions for most resources in the system:
	#
	# - "create_child": required to create child resources.
	#
	# - "list_children": required to list children (of all types) of a resources
	#
	# - "modify": required to modify or delete a resource or any of its children
	#
	# - "read": required to read a resource
	#
	# We define the following predefined roles for only a few high-level resources:
	#
	# - "admin": has all permissions on the resource
	#
	# - "collaborator": has "list_children" and "create_$child" for all children.
	# They'll inherit the "admin" role for any resources that they create.
	#
	# - "viewer": has "read" and "list_children" on a resource
	#
	# Below the project level, permissions are granted at the Project level. For
	# example, for someone to be able to create, modify, or delete any Instances,
	# they must be granted project.collaborator, which means they can create,
	# modify, or delete _all_ resources in the Project.
	#
	# The complete set of predefined roles:
	#
	# - fleet.admin (superuser for the whole system)
	# - fleet.collaborator (can create and own orgs)
	# - organization.admin (complete control over an organization)
	# - organization.collaborator (can create, modify, and delete projects)
	# - project.admin (complete control over a project)
	# - project.collaborator (can create, modify, and delete all resources within
	# the project, but cannot modify or delete the project
	# itself)
	# - project.viewer (can see everything in the project, but cannot modify
	# anything)
	#

		&*authn::USER_TEST_PRIVILEGED,
		&*authn::USER_TEST_UNPRIVILEGED,

add built-in users to the database #486

add built-in users to the database #486

Conversation

davepacheco commented Dec 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teisenbe Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-crespo Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepacheco commented Dec 7, 2021

david-crespo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teisenbe Dec 7, 2021 •

edited

Loading

david-crespo Dec 7, 2021 •

edited

Loading