Skip to content

/policy for current silo, /global/policy for fleet #1573

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Aug 12, 2022
Merged

/policy for current silo, /global/policy for fleet #1573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f374892
/policy for current silo, /global/policy for fleet
david-crespo Aug 10, 2022
4339550
clippy!!!
david-crespo Aug 10, 2022
e531ea1
verify policy endpoint (well, it fails)
david-crespo Aug 10, 2022
f0b7175
test silo role assignments with `/policy` endpoint
david-crespo Aug 10, 2022
03719ae
update openapi spec
david-crespo Aug 10, 2022
1ce5a09
welp. it was supposed to be public readonly all along
david-crespo Aug 10, 2022
aa1ee4b
share common code. why not
david-crespo Aug 11, 2022
69ae4c1
clean it up. we are ready
david-crespo Aug 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions nexus/src/app/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,16 @@ impl Nexus {
let mid = self.samael_max_issue_delay.lock().unwrap();
*mid
}

// Convenience function that exists solely because writing
// LookupPath::new(&opctx, &nexus.datastore()) in an endpoint handler feels
// like too much
pub fn db_lookup<'a>(
&'a self,
opctx: &'a OpContext,
) -> db::lookup::LookupPath {
db::lookup::LookupPath::new(opctx, &self.db_datastore)
}
Copy link
Contributor Author

@david-crespo david-crespo Aug 11, 2022

Choose a reason for hiding this comment

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

As I say in the comment, we can do without this, but having to refer explicitly to nexus.datastore() inside an endpoint handler to build the lookup feels like it violates the abstraction boundary of the Nexus object. No caller will ever have to make a lookup with a different datastore than the one that hangs of nexus.

}

/// For unimplemented endpoints, indicates whether the resource identified
Expand Down
16 changes: 6 additions & 10 deletions nexus/src/app/silo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,10 @@ impl super::Nexus {
pub async fn silo_fetch_policy(
&self,
opctx: &OpContext,
silo_name: &Name,
silo_lookup: db::lookup::Silo<'_>,
) -> LookupResult<shared::Policy<authz::SiloRole>> {
let (.., authz_silo) = LookupPath::new(opctx, &self.db_datastore)
.silo_name(silo_name)
.lookup_for(authz::Action::ReadPolicy)
.await?;
let (.., authz_silo) =
silo_lookup.lookup_for(authz::Action::ReadPolicy).await?;
let role_assignments = self
.db_datastore
.role_assignment_fetch_visible(opctx, &authz_silo)
Expand All @@ -100,13 +98,11 @@ impl super::Nexus {
pub async fn silo_update_policy(
&self,
opctx: &OpContext,
silo_name: &Name,
silo_lookup: db::lookup::Silo<'_>,
policy: &shared::Policy<authz::SiloRole>,
) -> UpdateResult<shared::Policy<authz::SiloRole>> {
let (.., authz_silo) = LookupPath::new(opctx, &self.db_datastore)
.silo_name(silo_name)
.lookup_for(authz::Action::ModifyPolicy)
.await?;
let (.., authz_silo) =
silo_lookup.lookup_for(authz::Action::ModifyPolicy).await?;
Copy link
Contributor Author

@david-crespo david-crespo Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Assuming the static lifetime isn't wrong, this approach of taking a lookup instead of a name or ID is rad. I like it.


let role_assignments = self
.db_datastore
Expand Down
73 changes: 67 additions & 6 deletions nexus/src/external_api/http_entrypoints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ type NexusApiDescription = ApiDescription<Arc<ServerContext>>;
/// Returns a description of the external nexus API
pub fn external_api() -> NexusApiDescription {
fn register_endpoints(api: &mut NexusApiDescription) -> Result<(), String> {
api.register(global_policy_view)?;
api.register(global_policy_update)?;

api.register(policy_view)?;
api.register(policy_update)?;

Expand Down Expand Up @@ -316,10 +319,10 @@ pub fn external_api() -> NexusApiDescription {
/// Fetch the top-level IAM policy
#[endpoint {
method = GET,
path = "/policy",
path = "/global/policy",
Copy link
Contributor Author

@david-crespo david-crespo Aug 10, 2022

Choose a reason for hiding this comment

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

As @zephraph suggested, this breaks off a little chunk of the big /global/* change just to unblock this work. The /global/ thing is so big that I think this approach makes sense regardless, though the downside is we have a short time where some global things have the prefix and some don't.

tags = ["policy"],
}]
async fn policy_view(
async fn global_policy_view(
rqctx: Arc<RequestContext<Arc<ServerContext>>>,
) -> Result<HttpResponseOk<shared::Policy<authz::FleetRole>>, HttpError> {
let apictx = rqctx.context();
Expand All @@ -342,10 +345,10 @@ struct ByIdPathParams {
/// Update the top-level IAM policy
#[endpoint {
method = PUT,
path = "/policy",
path = "/global/policy",
tags = ["policy"],
}]
async fn policy_update(
async fn global_policy_update(
rqctx: Arc<RequestContext<Arc<ServerContext>>>,
new_policy: TypedBody<shared::Policy<authz::FleetRole>>,
) -> Result<HttpResponseOk<shared::Policy<authz::FleetRole>>, HttpError> {
Expand All @@ -364,6 +367,62 @@ async fn policy_update(
apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
}

/// Fetch the current silo's IAM policy
#[endpoint {
method = GET,
path = "/policy",
tags = ["silos"],
}]
pub async fn policy_view(
rqctx: Arc<RequestContext<Arc<ServerContext>>>,
) -> Result<HttpResponseOk<shared::Policy<authz::SiloRole>>, HttpError> {
let apictx = rqctx.context();
let nexus = &apictx.nexus;
let handler = async {
let opctx = OpContext::for_external_api(&rqctx).await?;
let authz_silo = opctx
.authn
.silo_required()
.internal_context("loading current silo")?;

let lookup = nexus.db_lookup(&opctx).silo_id(authz_silo.id());
let policy = nexus.silo_fetch_policy(&opctx, lookup).await?;
Ok(HttpResponseOk(policy))
};
apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
}

/// Update the current silo's IAM policy
#[endpoint {
method = PUT,
path = "/policy",
tags = ["silos"],
}]
async fn policy_update(
rqctx: Arc<RequestContext<Arc<ServerContext>>>,
new_policy: TypedBody<shared::Policy<authz::SiloRole>>,
) -> Result<HttpResponseOk<shared::Policy<authz::SiloRole>>, HttpError> {
let apictx = rqctx.context();
let nexus = &apictx.nexus;
let new_policy = new_policy.into_inner();

let handler = async {
let nasgns = new_policy.role_assignments.len();
// This should have been validated during parsing.
bail_unless!(nasgns <= shared::MAX_ROLE_ASSIGNMENTS_PER_RESOURCE);
let opctx = OpContext::for_external_api(&rqctx).await?;
let authz_silo = opctx
.authn
.silo_required()
.internal_context("loading current silo")?;
let lookup = nexus.db_lookup(&opctx).silo_id(authz_silo.id());
let policy =
nexus.silo_update_policy(&opctx, lookup, &new_policy).await?;
Ok(HttpResponseOk(policy))
};
apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
}

/// List silos
///
/// Lists silos that are discoverable based on the current permissions.
Expand Down Expand Up @@ -502,7 +561,8 @@ async fn silo_policy_view(

let handler = async {
let opctx = OpContext::for_external_api(&rqctx).await?;
let policy = nexus.silo_fetch_policy(&opctx, silo_name).await?;
let lookup = nexus.db_lookup(&opctx).silo_name(silo_name);
let policy = nexus.silo_fetch_policy(&opctx, lookup).await?;
Ok(HttpResponseOk(policy))
};
apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
Expand Down Expand Up @@ -530,8 +590,9 @@ async fn silo_policy_update(
// This should have been validated during parsing.
bail_unless!(nasgns <= shared::MAX_ROLE_ASSIGNMENTS_PER_RESOURCE);
let opctx = OpContext::for_external_api(&rqctx).await?;
let lookup = nexus.db_lookup(&opctx).silo_name(silo_name);
let policy =
nexus.silo_update_policy(&opctx, silo_name, &new_policy).await?;
nexus.silo_update_policy(&opctx, lookup, &new_policy).await?;
Ok(HttpResponseOk(policy))
};
apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
Expand Down
19 changes: 17 additions & 2 deletions nexus/tests/integration_tests/endpoints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ lazy_static! {
format!("/hardware/sleds/{}", SLED_AGENT_UUID);

// Global policy
pub static ref POLICY_URL: &'static str = "/policy";
pub static ref GLOBAL_POLICY_URL: &'static str = "/global/policy";

// Silo used for testing
pub static ref DEMO_SILO_NAME: Name = "demo-silo".parse().unwrap();
Expand Down Expand Up @@ -520,7 +520,7 @@ lazy_static! {
pub static ref VERIFY_ENDPOINTS: Vec<VerifyEndpoint> = vec![
// Global IAM policy
VerifyEndpoint {
url: *POLICY_URL,
url: *GLOBAL_POLICY_URL,
visibility: Visibility::Public,
unprivileged_access: UnprivilegedAccess::None,
allowed_methods: vec![
Expand Down Expand Up @@ -676,6 +676,21 @@ lazy_static! {
),
],
},
VerifyEndpoint {
url: "/policy",
visibility: Visibility::Public,
unprivileged_access: UnprivilegedAccess::ReadOnly,
allowed_methods: vec![
AllowedMethod::Get,
AllowedMethod::Put(
serde_json::to_value(
&shared::Policy::<authz::SiloRole> {
role_assignments: vec![]
}
).unwrap()
),
],
},
Copy link
Contributor Author

@david-crespo david-crespo Aug 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This fails with the following. It's making a request with the unprivileged user that it expects to 404 but it's 200ing instead. I don't understand why the unprivileged user would 200 — we are doing operations that require ReadPolicy on the silo.

G GET  PUT  POST DEL  TRCE G  URL
0thread 'integration_tests::unauthorized::test_unauthorized' panicked at 'called `Result::unwrap()` on an `Err` value: expected status code 404 Not Found, found 200 OK

omicron/nexus/tests/integration_tests/unauthorized.rs

Lines 444 to 457 in 000333b

if do_test_unprivileged {
let expected_status = match allowed {
Some(_) => unauthz_status,
None => StatusCode::METHOD_NOT_ALLOWED,
};
let response = NexusRequest::new(
RequestBuilder::new(client, method.clone(), &uri)
.body(body.as_ref())
.expect_status(Some(expected_status)),
)
.authn_as(AuthnMode::UnprivilegedUser)
.execute()
.await
.unwrap();

Copy link
Contributor

@just-be-dev just-be-dev Aug 10, 2022

Choose a reason for hiding this comment

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

Given the /policy API is known and documented if you didn't have access to that it'd more likely be a 401. I think you want to change the visibility in your test to public instead of protected. That might not be the original issue, but it is one thing.

Copy link
Contributor Author

@david-crespo david-crespo Aug 10, 2022

Choose a reason for hiding this comment

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

First pass at debugging using the lovely guide. Seems like the unprivileged user does have the permission in question, which is weird?

~/oxide/omicron $ cat /var/folders/lk/vsx86g0545g3g2tsmvs50py40000gn/T/test_all-af18dffa1640b050-test_unauthorized.49638.0.log | rg "authorize result" | rg "/policy" | rg "ReadPolicy" | rg "60001" | jq '{ msg, req_id, authenticated, method, uri, actor, action, result }'               
{
  "msg": "authorize result",
  "req_id": "49c6298b-e273-41f4-b3ac-a79de7259074",
  "authenticated": true,
  "method": "GET",
  "uri": "/global/policy",
  "actor": "Some(Actor::SiloUser { silo_user_id: 001de000-05e4-4000-8000-000000060001, silo_id: 001de000-5110-4000-8000-000000000000, .. })",
  "action": "ReadPolicy",
  "result": "Err(Forbidden)"
}
{
  "msg": "authorize result",
  "req_id": "9b1a600f-70bc-4fd8-8103-5bde60c970bc",
  "authenticated": true,
  "method": "GET",
  "uri": "/silos/demo-silo/policy",
  "actor": "Some(Actor::SiloUser { silo_user_id: 001de000-05e4-4000-8000-000000060001, silo_id: 001de000-5110-4000-8000-000000000000, .. })",
  "action": "ReadPolicy",
  "result": "Err(ObjectNotFound { type_name: Silo, lookup_type: ByName(\"demo-silo\") })"
}
{
  "msg": "authorize result",
  "req_id": "2ba41eb1-bf4f-42d8-9ee4-a1fbe8899082",
  "authenticated": true,
  "method": "GET",
  "uri": "/policy",
  "actor": "Some(Actor::SiloUser { silo_user_id: 001de000-05e4-4000-8000-000000060001, silo_id: 001de000-5110-4000-8000-000000000000, .. })",
  "action": "ReadPolicy",
  "result": "Ok(())"
}
{
  "msg": "authorize result",
  "req_id": "2ba41eb1-bf4f-42d8-9ee4-a1fbe8899082",
  "authenticated": true,
  "method": "GET",
  "uri": "/policy",
  "actor": "Some(Actor::SiloUser { silo_user_id: 001de000-05e4-4000-8000-000000060001, silo_id: 001de000-5110-4000-8000-000000000000, .. })",
  "action": "ReadPolicy",
  "result": "Ok(())"
}

Copy link
Contributor Author

@david-crespo david-crespo Aug 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This was the explanation of what I was seeing. The existing silo policy test was not asking about the user's own silo, which is why the unprivileged user didn't have access. Even the unpriv user has access to their own silo. I am writing an issue to think about how we could do this differently if we wanted to.

omicron/nexus/src/authz/omicron.polar

Lines 148 to 163 in 4528641

# As a special case, all authenticated users can read their own Silo. That's
# not quite the same as having the "viewer" role. For example, they cannot list
# Organizations in the Silo.
#
# One reason this is necessary is because if an unprivileged user tries to
# create an Organization using "POST /organizations", they should get back a 403
# (which implies they're able to see /organizations, which is essentially seeing
# the Silo itself) rather than a 404. This behavior isn't a hard constraint
# (i.e., you could reasonably get a 404 for an API you're not allowed to call).
# Nor is the implementation (i.e., we could special-case this endpoint somehow).
# But granting this permission is the simplest way to keep this endpoint's
# behavior consistent with the rest of the API.
#
# It's unclear what else would break if users couldn't see their own Silo.
has_permission(actor: AuthenticatedActor, "read", silo: Silo)
if silo in actor.silo;


VerifyEndpoint {
url: "/users",
Expand Down
55 changes: 54 additions & 1 deletion nexus/tests/integration_tests/role_assignments.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ async fn test_role_assignments_fleet(cptestctx: &ControlPlaneTestContext) {
const ROLE: Self::RoleType = authz::FleetRole::Admin;
const VISIBLE_TO_UNPRIVILEGED: bool = true;
fn policy_url(&self) -> String {
String::from("/policy")
String::from("/global/policy")
}

fn verify_initial<'a, 'b, 'c, 'd>(
Expand Down Expand Up @@ -218,6 +218,59 @@ async fn test_role_assignments_silo(cptestctx: &ControlPlaneTestContext) {
run_test(client, SiloRoleAssignmentTest {}).await;
}

// same as above except for /policy, where silo is implicit in auth
#[nexus_test]
async fn test_role_assignments_silo_implicit(
cptestctx: &ControlPlaneTestContext,
) {
struct SiloRoleAssignmentTest;
impl RoleAssignmentTest for SiloRoleAssignmentTest {
type RoleType = authz::SiloRole;
const ROLE: Self::RoleType = authz::SiloRole::Admin;
const VISIBLE_TO_UNPRIVILEGED: bool = true;
fn policy_url(&self) -> String {
"/policy".to_string()
}

fn verify_initial<'a, 'b, 'c, 'd>(
&'a self,
_: &'b ClientTestContext,
_current_policy: &'c shared::Policy<Self::RoleType>,
) -> BoxFuture<'d, ()>
where
'a: 'd,
'b: 'd,
'c: 'd,
{
async {
// TODO-coverage TODO-security There is currently nothing that
// requires the ability to modify a Silo. Once there is, we
// should test it here.
}
.boxed()
}

fn verify_privileged<'a, 'b, 'c>(
&'a self,
_: &'b ClientTestContext,
) -> BoxFuture<'c, ()>
where
'a: 'c,
'b: 'c,
{
async {
// TODO-coverage TODO-security There is currently nothing that
// requires the ability to modify a Silo. Once there is, we
// should test it here.
}
.boxed()
}
}

let client = &cptestctx.external_client;
run_test(client, SiloRoleAssignmentTest {}).await;
}

#[nexus_test]
async fn test_role_assignments_organization(
cptestctx: &ControlPlaneTestContext,
Expand Down
6 changes: 4 additions & 2 deletions nexus/tests/output/nexus_tags.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ organization_view_by_id /by-id/organizations/{id}

API operations found with tag "policy"
OPERATION ID URL PATH
policy_update /policy
policy_view /policy
global_policy_update /global/policy
global_policy_view /global/policy

API operations found with tag "projects"
OPERATION ID URL PATH
Expand Down Expand Up @@ -132,6 +132,8 @@ session_sshkey_view /session/me/sshkeys/{ssh_key_name}

API operations found with tag "silos"
OPERATION ID URL PATH
policy_update /policy
policy_view /policy
silo_create /silos
silo_delete /silos/{silo_name}
silo_identity_provider_create /silos/{silo_name}/saml-identity-providers
Comment on lines +135 to 139
Copy link
Contributor

@just-be-dev just-be-dev Aug 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not certain this tag grouping is really correct. I'm not recommending a change in this PR, but broadly it's worth us talking about what we're going to call "global" in the context of the silo. If we're going to use the tag silo (and thus silo as a term to describe this global context) then we're going to need to educate users on it even before they really need to know what a silo is.

Eh, on second thought most of the stuff here will be grouped under the global tag eventually so it's probably moot. Still unsure if silo is the right tag for things at the root that aren't under /organizations but also aren't under /global, but I don't know what else we'd call it.

Expand Down
Loading