oxidecomputer / omicron

Omicron: Oxide control plane
Mozilla Public License 2.0
252 stars 40 forks source link

Group attribute name and Silo admin group for IdP should be mandatory #7092

Open wfchandler opened 1 week ago

wfchandler commented 1 week ago

Per @askfongjojo, the group attribute name for an identity provider should be mandatory. Failing to set this value is likely to prevent users from being given access to the silo.

Currently this is an optional field in Nexus, as is listed as such in the CLI and web console.

davepacheco commented 1 week ago

It makes sense that we'd want to guide people to providing this. However, we don't actually need it. If we require it, the system may be undeployable in environments that don't use groups like this (which might include some dev/test environments, though we can deal with that if it's truly the only case). Would it make sense to build a mechanism where the system warns the user if they try to proceed without setting this, but still allow it?

david-crespo commented 1 week ago

If this setup is usually done through the web console, we could try being noisier about it in the UI. We could even require it in the form even if it's not required by the API.

askfongjojo commented 6 days ago

the system may be undeployable in environments that don't use groups like this (which might include some dev/test environments, though we can deal with that if it's truly the only case).

@davepacheco - The problem is that the silo is unusable without any user assigned the admin role. The only way that allows the fleet admin to grant a silo user to the silo admin role is very obscure when no group attribute is specified:

  1. The person who should have admin role logs in and authenticates via the IDP and get a "Something went wrong" error on the web console.
  2. The person uses Oxide CLI to go through the create API token workflow.
  3. The person then uses oxide current-user view to find out their user id and provides the id to the fleet admin.
  4. The fleet admin then use oxide silo policy update --silo $SILO_NAME --json-body policy.json with this payload:
    {
    "role_assignments": [{
    "identity_id": "$idReturnedFromCreateUser",
    "identity_type": "silo_user",
    "role_name": "admin"
    }]
    }
david-crespo commented 6 days ago

Why do they get the error on login? Shouldn’t they be a silo viewer by default?

askfongjojo commented 6 days ago

Why do they get the error on login? Shouldn’t they be a silo viewer by default?

There is no such default. Every role has to be assigned explicitly by someone with the silo admin role (or fleet admin role in the awkward flow mentioned above).

Regarding the workflow: A quicker way of getting to step 4 is to automatically navigate user to https://$SILO_URL/settings/profile with an error message. That would allow the user to find out their id and group memberships without resorting to use CLI. Or we keep the "Something went wrong" page and add a more detailed message (e.g. a link to the settings page and suggestion to review the person's group attribute (value) on the IDP side, if a group attribute is in use). Either way, we'll document the silo policy update CLI command in the relevant user guides (e.g. Troubleshooting guide, FAQ, Silo management).

Another issue that the user encountered is that group attribute is not displayed in the view existing IDP form. It is already returned in the API response and should be displayed in the UI for the fleet admin to know what exactly was entered and if there was a typo.

davepacheco commented 6 days ago

Thanks for those details, @askfongjojo. I see now it makes sense to just make this required. It doesn't foreclose on relaxing the constraint later if we decide it's important enough to fix up the other flow.

david-crespo commented 6 days ago

The missing group name on the existing IdP view is fixed (https://github.com/oxidecomputer/console/pull/2520). That will be out in v12.

askfongjojo commented 4 days ago

The silo admin group field is optional regardless of the identity mode. It should be mandatory when it is using SAML - for the same reason as setting group attribute name mandatory. (The group attribute name is the key while the admin group name is the value; we need the k/v pair for a silo admin assignment workflow.) I've updated the issue subject line to include this requirement.