update Silo APIs for RFD 321 #1768

davepacheco · 2022-09-30T23:02:16Z

This makes a few changes based on the determinations of RFD 321 in preparation for support for password-based login. I hope there are no surprises here!

Changes here:

the routes for manipulating SAML IdPs move from /system/silos/:silo_name/saml-identity-providers to /system/silos/:silo_name/identity-providers/saml
the routes for logging into SAML Silos move from /login/:silo_name/:provider_name to /login/:silo_name/saml/:provider_name
Internally, there's a new AuthenticationMode alongside the UserProvisionType. AuthenticationMode may be either "SAML" or "Local".
Externally, the user_provision_type / UserProvisionType has turned into silo_identity_mode / SiloIdentityMode, which encapsulates both the authentication mode and the user provision type.

This also renames UserProvisionType::Fixed to UserProvisionType::ApiOnly for clarity.

This does not implement the "local" identity provider tyep, CRUD for local users, or password login -- those will be in follow-up PRs.

This will be a breaking change to the API because of the change to params::SiloCreate, views::Silo, and the change in route structure. CC @david-crespo @zephraph @karencfv.

This will be a breaking change to the Remote Access Preview procedure (both Mercury and Pluto) for the same reason. (Those procedures use these APIs directly.) Further, if you have a Silo that's already set up with an identity provider (like saml.eng), I'm not sure if it will continue to work because the login URL that the IdP POSTs back to has changed. I'm not sure if this is recorded anywhere though. If this is a problem for anyone, I'm happy to dig in further.

davepacheco · 2022-09-30T23:02:42Z

Meant to CC @smklein because of the Remote Access Preview procedure change.

davepacheco · 2022-09-30T23:10:50Z

Heads up: I'm going to go ahead and rebase on top of "main" because the dependency on #1757 is very minimal and we may as well keep these separately. That's going to involve a force push to this branch so you may want to defer review until I've done that.

davepacheco · 2022-09-30T23:23:58Z

This is ready for review now.

davepacheco · 2022-09-30T23:31:55Z

One thing I want to call out: per RFD 321, the params::SiloCreate and views::Silo have a single unified enum called the SiloIdentityMode. This translates into two different enums internally: the authentication mode and the user provision type. After discussing with @jmpesp, we decided to store the authentication mode and provision type separately in the database. Note that only two combinations of (authn mode, user provision mode) are currently valid -- other combinations are not allowed. Silos with those other combinations cannot be created by the API. That's basically the reason SiloIdentityMode exists: to describe the valid choices.

A consequence of all this is that it's possible to store Silo configurations that aren't valid. The only time this will cause a problem is if someone tries to fetch a Silo with an invalid config (either by listing Silos or fetching it explicitly). In that case, the request will fail to convert the db::model::Silo to a views::Silo and produce a 500 (since this is essentially a bug -- how did we save an invalid Silo in the first place?).

In an ideal world, I think db::model::Silo would use silo_identity_mode but it would be serialized using the two separate enums. That way, we'd fail to read the invalid Silo from the database and won't try to operate on it at all. If we did that, it would still be true that most code only needs to consider one of the two backend enums because it's trivial -- and always valid -- to convert the silo identity mode to these other two enums. The main blocker is I wasn't sure how to do this with our model types / Diesel. But this also isn't a big deal: it should be an impossible case, and it's easy to change this behavior later, since it wouldn't affect the API or the database schema.

jmpesp · 2022-09-30T23:38:26Z

Further, if you have a Silo that's already set up with an identity provider (like saml.eng), I'm not sure if it will continue to work because the login URL that the IdP POSTs back to has changed.

This will break our test SAML config(s), yeah. But the URL can be updated as soon as this lands (or before in order to test?)

jmpesp

LGTM!

jmpesp · 2022-09-30T23:40:01Z

nexus/src/app/silo.rs

-                    // If the user provision type is fixed, do not a new user if
-                    // one does not exist.
-                    db::model::UserProvisionType::Fixed => {
+                    // If the user provision type is ApiOnly, do not a new user


this doesn't look right - maybe it should be "do not create a new user"

Fixed in 394c3be.

jmpesp · 2022-09-30T23:40:59Z

nexus/src/external_api/console_api.rs

 #[endpoint {
   method = POST,
-   path = "/login/{silo_name}/{provider_name}",
+   path = "/login/{silo_name}/saml/{provider_name}",


This is the URL that will have to change in the Keycloak configuration, it's the path the IdP POSTs to.

Let's also maybe update the remote access docs to use a different name for the provider instead of saml, lest we end up with the repetitive /login/mercury/saml/saml 😅

jmpesp · 2022-09-30T23:42:59Z

nexus/tests/integration_tests/saml.rs


    const SILO_NAME: &str = "saml-silo";
-    create_silo(&client, SILO_NAME, true, shared::UserProvisionType::Fixed)
+    create_silo(&client, SILO_NAME, true, shared::SiloIdentityMode::LocalOnly)


this shouldn't be LocalOnly, right? one of the future planned changes is to reject configuring an identity provider if it doesn't match the identity mode, which means this test will be broken.

Yeah, good call. I was separately trying to sneak in one more change that validates that and updates several of these tests accordingly. That's coming shortly.

Fixed in 394c3be.

jmpesp · 2022-09-30T23:43:39Z

nexus/tests/integration_tests/silos.rs

    let nexus = &cptestctx.server.apictx.nexus;

    let test_cases: Vec<TestSiloUserProvisionTypes> = vec![
        // A silo configured with a "fixed" user provision type should fetch a


comment has to be updated

Fixed in 394c3be.

jmpesp · 2022-09-30T23:44:48Z

nexus/types/src/external_api/shared.rs

+pub enum SiloIdentityMode {
+    /// Users are authenticated with SAML using an external authentication
+    /// provider.  The system updates information about users and groups only
+    /// during authentication (i.e,. "JIT provisioning" of users and groups).


The system updates information about users and groups only during authentication

Nit: only as a result of successful authentication.

Fixed in 394c3be.

davepacheco · 2022-10-01T01:41:27Z

I fixed the tests, but it was more complicated than expected. The "unauthorized" test implicitly relied on being able to create an IdP in the default Silo, which is no longer allowed. So I had it use a new Silo. But I had to give the privileged test user permissions on that Silo. Then we trip on the belt-and-suspenders check in the lookup macro that you're not accessing a resource from outside your Silo. I think the solution here is an allowlist (or config option, really) to allow certain resources to be accessible to users in other Silos. We've long known this would be needed for a few resources like identity providers.

There's one remaining issue here, which is the hardcoded "allowlist" -- the XXX -- in the db lookup macro.

davepacheco · 2022-10-03T20:37:25Z

There are two notable changes since this was last reviewed, both falling out of the "unauthorized.rs" test failure.

The "test-privileged" user needs to be able to see the IdPs in a Silo that they're not in. Since we've previously said this is an important use case for us anyway, I updated omicron.polar so that Fleet viewers/admins have corresponding privileges on all Silos' IdPs.
The db lookup macro has a belt-and-suspenders check preventing you from fetching siloed resources in Silos that aren't yours. I added an override for this to be applied to identity providers so that they'd be visible outside the Silo.

david-crespo · 2022-10-04T19:30:54Z

I didn't review in detail, but I'm standing by to update the console once this is merged.

Crucible changes are: update to latest `vergen` (#1770) Update rand dependencies, and fallout from that. (#1764) [crucible-downstairs] migrate to API traits (#1768) [crucible-agent] migrate to API trait (#1766) [crucible-pantry] migrate to API trait (#1767) Add back job delays in the downstairs with the --lossy flag (#1761) Propolis changes are: Crucible update plus a few other dependency changes. (#948) [2/n] [propolis-server] switch to API trait (#946) [1/n] add a temporary indent to propolis server APIs (#945) Handle Intel CPUID leaves 4 and 18h, specialize CPUID for VM shape (#941) Increase viona receive queue length to 2048 (#935) Expand viona header pad to account for options (#937) fix linux p9fs multi message reads (#932) add a D script to report VMs' CPUID queries (#934) Update GH actions Re-enable viona packet data loaning

Crucible changes are: update to latest `vergen` (#1770) Update rand dependencies, and fallout from that. (#1764) [crucible-downstairs] migrate to API traits (#1768) [crucible-agent] migrate to API trait (#1766) [crucible-pantry] migrate to API trait (#1767) Add back job delays in the downstairs with the --lossy flag (#1761) Propolis changes are: Crucible update plus a few other dependency changes. (#948) [2/n] [propolis-server] switch to API trait (#946) [1/n] add a temporary indent to propolis server APIs (#945) Handle Intel CPUID leaves 4 and 18h, specialize CPUID for VM shape (#941) Increase viona receive queue length to 2048 (#935) Expand viona header pad to account for options (#937) fix linux p9fs multi message reads (#932) add a D script to report VMs' CPUID queries (#934) Update GH actions Re-enable viona packet data loaning --------- Co-authored-by: Alan Hanson <alan@oxide.computer> Co-authored-by: Sean Klein <sean@oxide.computer> Co-authored-by: Rain <rain@oxide.computer> Co-authored-by: John Gallagher <john@oxidecomputer.com> Co-authored-by: iliana etaoin <iliana@oxide.computer> Co-authored-by: Sean Klein <sean@oxidecomputer.com> Co-authored-by: Alex Plotnick <alex@oxidecomputer.com> Co-authored-by: David Pacheco <dap@oxidecomputer.com> Co-authored-by: Andrew J. Stone <andrew@oxidecomputer.com>

davepacheco requested a review from jmpesp September 30, 2022 23:02

davepacheco added 4 commits September 30, 2022 16:19

updates SAML IdP routes for RFD 321

eb0c95b

UserProvisionType, AuthenticationMode, SiloIdentityMode

b575ec2

RFD 321: add authn mode enum, unified enum

79326e2

remove XXX

a98dced

davepacheco force-pushed the silo-idp-routes-rfd-321 branch from 527b660 to a98dced Compare September 30, 2022 23:23

davepacheco changed the base branch from dropshot-redirects to main September 30, 2022 23:23

jmpesp approved these changes Sep 30, 2022

View reviewed changes

davepacheco added 3 commits September 30, 2022 16:53

prevent creating SAML IdP in LocalOnly Silos

394c3be

fix test

d321b79

rustfmt

dcdd5d5

davepacheco added 3 commits October 3, 2022 12:42

Merge branch 'main' into silo-idp-routes-rfd-321

099d42d

replace hardcoded behavior with configurable

cf2ece1

Fleet privileges should be enough to see IdPs

84ad026

jmpesp approved these changes Oct 4, 2022

View reviewed changes

davepacheco merged commit 9dfa2d3 into main Oct 4, 2022

davepacheco deleted the silo-idp-routes-rfd-321 branch October 4, 2022 20:07

leftwo mentioned this pull request Sep 23, 2025

Update Propolis and Crucible #9061

Merged

update Silo APIs for RFD 321 #1768

update Silo APIs for RFD 321 #1768

Uh oh!

Conversation

davepacheco commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davepacheco commented Sep 30, 2022

Uh oh!

davepacheco commented Sep 30, 2022

Uh oh!

davepacheco commented Sep 30, 2022

Uh oh!

davepacheco commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmpesp commented Sep 30, 2022

Uh oh!

jmpesp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davepacheco commented Oct 1, 2022

Uh oh!

davepacheco commented Oct 3, 2022

Uh oh!

david-crespo commented Oct 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

davepacheco commented Sep 30, 2022 •

edited

Loading

davepacheco commented Sep 30, 2022 •

edited

Loading