feat: Auto-Populate POSIX Information on sign in using SSO [CM-399] #9755

kkunapuli · 2024-07-29T21:30:20Z

Ticket

Description

When the a determined master is configured to expected the POSIX claims in SSO login & they are provided in the claim, then we will populate the relevant fields for the user:

AgentUID   null.Int    `db:"agent_uid" json:"agent_uid"`
AgentGID   null.Int    `db:"agent_gid" json:"agent_gid"`
AgentUser  null.String `db:"agent_user" json:"agent_user"`
AgentGroup null.String `db:"agent_group" json:"agent_group"`

Also re-adds new fields to master configuration (original done here https://github.com/determined-ai/determined/pull/9690/files). The original PR was reverted because of CI errors.

Test Plan

Test using an Okta application.

OIDC

Recommended application: carolina-oidc-test, https://dev-2564556-admin.okta.com/admin/app/oidc_client/instance/0oacxxzecx7a9l5O85d7/#tab-general

add your user to the test application, filling in the fields posix_uid (int), posix_gid (int), agent_user_name (string) and agent_group_name (string)
in your master config, add the following fields to an oidc section:

oidc:
  agent_uid_attribute_name: posix_uid
  agent_gid_attribute_name: posix_gid
  agent_user_name_attribute_name: agent_user_name
  agent_group_name_attribute_name: agent_group_name
  always_redirect: true

start devcluster, and log in using Okta
inspect the database and ensure that it is updated, e.g. SELECT * FROM agent_user_groups; and look at columns uid, gid, user_ and group_

You can also try different combinations! A user should be able to specify any, none, or all of the new parameters.

SAML

Recommended application: Determined SAML CI, https://dev-2564556-admin.okta.com/admin/app/dev-2564556_determinedsamlci_1/instance/0oaganmj6zxHC3A575d7/#tab-assignments

add your user to the test application, the fields will be filled out automatically
in your master config, add the following fields to an saml section:

saml:
  agent_uid_attribute_name: posix_uid
  agent_gid_attribute_name: posix_gid
  agent_user_name_attribute_name: agent_user_name
  agent_group_name_attribute_name: agent_group_name
  always_redirect: true

start devcluster, and log in using Okta
inspect the database and ensure that it is updated, e.g.

determined=# select * from agent_user_groups;
 id | user_id |  user_   | uid |  group_  | gid 
----+---------+----------+-----+----------+-----
  2 |       3 | Kristine |   4 | Kunapuli |   5

user_ will be your first name and group_ will be your last name.

Checklist

Changes have been manually QA'd
New features have been approved by the corresponding PM
User-facing API changes have the "User-facing API Change" label
Release notes have been added as a separate file under docs/release-notes/
See Release Note for details.
Licenses have been included for new code which was copied and/or modified from any external code

netlify · 2024-07-29T21:30:42Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`bdd7865`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/66bb9493a3038e0008e6832a

master/internal/config/saml_config.go

kkunapuli · 2024-07-29T21:31:47Z

master/internal/plugin/saml/service.go

+	strAgentUID := getSAMLAttribute(response, string(s.userConfig.agentUIDAttributeName))
+	tempAgentUID, err := strconv.Atoi(strAgentUID)
+	if err != nil {
+		logrus.WithError(err).WithField("agentUID", strAgentUID).Error("unable to convert to integer")


I think this error should be very rare / surprising but we should log it just in case.

wdy think about having this parser return an err? the only place it's used does support returning an error that might be the right behavior here.

Great idea, thanks!

codecov · 2024-07-29T21:32:18Z

Codecov Report

Attention: Patch coverage is 37.96296% with 67 lines in your changes missing coverage. Please review.

Project coverage is 54.04%. Comparing base (4e47a1e) to head (bdd7865).
Report is 64 commits behind head on main.

Files	Patch %	Lines
master/internal/plugin/oidc/service.go	0.00%	47 Missing ⚠️
master/internal/plugin/saml/service.go	68.42%	18 Missing ⚠️
master/internal/user/postgres_agentusergroup.go	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9755      +/-   ##
==========================================
- Coverage   54.31%   54.04%   -0.27%     
==========================================
  Files        1261     1261              
  Lines      155624   155714      +90     
  Branches     3534     3533       -1     
==========================================
- Hits        84530    84159     -371     
- Misses      70956    71417     +461     
  Partials      138      138

Flag	Coverage Δ
backend	`44.89% <37.96%> (-0.02%)`	⬇️
harness	`71.25% <ø> (-1.37%)`	⬇️
web	`53.79% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
master/internal/config/oidc_config.go	`50.00% <ø> (ø)`
master/internal/config/saml_config.go	`20.00% <ø> (ø)`
master/internal/user/postgres_agentusergroup.go	`86.04% <50.00%> (-3.96%)`	⬇️
master/internal/plugin/saml/service.go	`32.99% <68.42%> (+12.21%)`	⬆️
master/internal/plugin/oidc/service.go	`7.17% <0.00%> (-1.64%)`	⬇️

... and 19 files with indirect coverage changes

kkunapuli · 2024-07-30T20:08:13Z

master/internal/user/postgres_agentusergroup.go

-	if err != nil {
+	if err == sql.ErrNoRows {
+		logrus.WithError(err).Warnf("no agent user group results from workspaceID=%d", workspaceID)
+	} else if err != nil {


workspaceID is supposed to be optional, but bun throws an error if zero results are returned.

kkunapuli · 2024-07-30T20:31:23Z

docs/release-notes/auto-populate-posix.rst

+   `agent_gid_attribute_name`, `agent_user_name_attribute_name`, or
+   `agent_group_name_attribute_name`. Please visit :ref:`oidc master configuration
+   <_master-config-oidc>` or :ref:`saml master configuration <_master-config-saml>` for details. If
+   any of the fields are configured, they will be used and synced to the database.


I don't understand why it's accepting _master-config-saml as a reference link but not _master-config-oidc.

:orphan:

New Features

Master Configuration: Add support for POSIX claims in the master configuration. It now accepts
agent_uid_attribute_name, agent_gid_attribute_name, agent_user_name_attribute_name,
or agent_group_name_attribute_name. Refer to the :ref:OIDC master configuration <master-config-oidc>
or :ref:SAML master configuration <master-config-saml> for details. If any of these fields
are configured, they will be used and synced to the database.

@kkunapuli i think maybe you added the leading underscore and its that part that is not needed. maybe something with the angle brackets. https://docs.readthedocs.io/en/stable/guides/cross-referencing-with-sphinx.html

That was it - thank you so much @tara-det-ai!

tara-det-ai

docs stamp not required to merge; left a suggested edit

hamidzr · 2024-08-05T18:27:20Z

master/internal/plugin/saml/service.go

+	strAgentUID := getSAMLAttribute(response, string(s.userConfig.agentUIDAttributeName))
+	tempAgentUID, err := strconv.Atoi(strAgentUID)
+	if err != nil {
+		logrus.WithError(err).WithField("agentUID", strAgentUID).Error("unable to convert to integer")


wdy think about having this parser return an err? the only place it's used does support returning an error that might be the right behavior here.

varlogtim

Possible concern.

varlogtim

Possible issue

master/internal/plugin/oidc/service.go

varlogtim · 2024-08-07T17:54:52Z

master/internal/plugin/oidc/service.go

+		Group: dbData.Group,
+	}
+
+	if sessionData.AgentUIDSet {


Another nit with this pattern.

I am imagining the following UX:

TL;DR: once a user as a UID/GID set, it can never be unset via Auth claims and would need to be manually removed.

System admin configures security.default_task.user_id: 1234, in master.yaml

Some impetus: results in UserA having AgentUID 5432 being set in the auth claim.

Other impetus: results in UserA's AgentUID being removed from the auth claim for which the admin would expect that tasks run by that user would run as UID 1234, however, I suspect they will continue to run as 5432.

To be clear, I am OK with this behavior, I think... but I wanted to bring it up here for the following reasons:

To get confirmation that what I speculation about the behavior here is correct.

Document that this is the behavior we should expect.

Get any input from folks other than me if this behavior is acceptable.

I tested this behavior today and confirmed that it works as I expected it might.

Once you have enabled this feature and an AgentUID/GID is set for a user in order to have have this revert back to using the security.default_task.uid an admin needs to manually remove the AgentUID/GID settings for that user.

We discussed real would situations in which this type of thing could come and yesterday and the only real one we could think is if a customer switches IdPs and forgets to populate the Agent UID/GID attributes in the auth claim. In this situation we would likely want to keep the UID and GID of the users there and not remove them and have the system go back to user the default. Therefore, I think the current behavior is best.

tara-det-ai · 2024-08-13T16:23:57Z

docs/manage/users-remote.rst

+   reset manually. In order to have have the UID or GID revert back to using the
+   security.default_task.uid(gid), an admin needs to manually remove the agent UID/GID settings for
+   that user.
+


To automatically link custom claims, you can configure the attribute name with the relevant field. For example:

agent_uid_attribute_name: "custom_uid_attribute_name"

.. note::

Once this feature is enabled and an agent UID/GID is assigned to a user, it can only be reset manually.
To revert the UID or GID back to the default value specified by security.default_task.uid(gid), an admin
must manually remove the agent UID/GID settings for that user.

tara-det-ai

suggested edit

cla-bot bot added the cla-signed label Jul 29, 2024

kkunapuli changed the title ~~feat: automatically populate posix fields~~ feat: automatically populate posix fields from master config Jul 29, 2024

kkunapuli commented Jul 29, 2024

View reviewed changes

master/internal/config/saml_config.go Show resolved Hide resolved

kkunapuli commented Jul 29, 2024

View reviewed changes

kkunapuli commented Jul 30, 2024

View reviewed changes

determined-ci added the documentation Improvements or additions to documentation label Jul 30, 2024

determined-ci requested a review from a team July 30, 2024 20:17

kkunapuli changed the title ~~feat: automatically populate posix fields from master config~~ feat: Auto-Populate POSIX Information on sign in using SSO [CM-399] Jul 30, 2024

kkunapuli commented Jul 30, 2024

View reviewed changes

kkunapuli force-pushed the kunapuli/auto-populate-posix-info2 branch from dead50d to edf0afb Compare July 30, 2024 20:43

kkunapuli marked this pull request as ready for review July 31, 2024 17:23

kkunapuli requested a review from a team as a code owner July 31, 2024 17:23

kkunapuli requested a review from hamidzr July 31, 2024 17:23

tara-det-ai reviewed Aug 1, 2024

View reviewed changes

hamidzr approved these changes Aug 5, 2024

View reviewed changes

determined-ci requested a review from a team August 6, 2024 19:06

kkunapuli force-pushed the kunapuli/auto-populate-posix-info2 branch from 140ef56 to f9258b7 Compare August 6, 2024 19:58

varlogtim reviewed Aug 7, 2024

View reviewed changes

master/internal/plugin/oidc/service.go Outdated Show resolved Hide resolved

kkunapuli added 9 commits August 7, 2024 10:11

checkpoint

1b46cc2

saml tests

42d5ba2

oidc

620466d

lint fixes

76d82c7

update docs for new configuration fields

f1d3454

fix docs formatting

24eae89

undo unintentional docs change

dd6c40e

redo helm changes

541a293

oidc fixes

00899df

kkunapuli added 5 commits August 7, 2024 10:11

saml fixes

61ba1ae

fix docs build / linking

7db3b5f

don't log warning if we don't expect results

238e5ba

return error for saml

706ee2c

fix intg tests

5800ffe

kkunapuli force-pushed the kunapuli/auto-populate-posix-info2 branch from f9258b7 to 5800ffe Compare August 7, 2024 14:12

kkunapuli added 2 commits August 7, 2024 11:03

saml: check if uid/gid are set

5b2b9b5

oidc: track if uid/gid set

51360b8

varlogtim reviewed Aug 7, 2024

View reviewed changes

kkunapuli added 3 commits August 13, 2024 10:37

add usage info and a note for users

583c5de

fix tics

2f0e30d

fix docs build

50f603f

tara-det-ai reviewed Aug 13, 2024

View reviewed changes

docs feedback update

bdd7865

determined-ci requested a review from a team August 13, 2024 17:15

kkunapuli merged commit c1b7767 into main Aug 28, 2024
82 of 96 checks passed

kkunapuli deleted the kunapuli/auto-populate-posix-info2 branch August 28, 2024 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Auto-Populate POSIX Information on sign in using SSO [CM-399] #9755

feat: Auto-Populate POSIX Information on sign in using SSO [CM-399] #9755

kkunapuli commented Jul 29, 2024 •

edited

Loading

netlify bot commented Jul 29, 2024 •

edited

Loading

kkunapuli Jul 29, 2024

hamidzr Aug 5, 2024

kkunapuli Aug 7, 2024

codecov bot commented Jul 29, 2024 •

edited

Loading

kkunapuli Jul 30, 2024

kkunapuli Jul 30, 2024

tara-det-ai Aug 1, 2024

tara-det-ai Aug 1, 2024

kkunapuli Aug 6, 2024

tara-det-ai left a comment

hamidzr Aug 5, 2024

varlogtim left a comment

varlogtim left a comment

varlogtim Aug 7, 2024 •

edited

Loading

varlogtim Aug 8, 2024 •

edited

Loading

tara-det-ai Aug 13, 2024

tara-det-ai left a comment

feat: Auto-Populate POSIX Information on sign in using SSO [CM-399] #9755

feat: Auto-Populate POSIX Information on sign in using SSO [CM-399] #9755

Conversation

kkunapuli commented Jul 29, 2024 • edited Loading

Ticket

Description

Test Plan

OIDC

SAML

Checklist

netlify bot commented Jul 29, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 29, 2024 • edited Loading

Codecov Report

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

tara-det-ai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varlogtim left a comment

Choose a reason for hiding this comment

varlogtim left a comment

Choose a reason for hiding this comment

varlogtim Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

varlogtim Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tara-det-ai left a comment

Choose a reason for hiding this comment

kkunapuli commented Jul 29, 2024 •

edited

Loading

netlify bot commented Jul 29, 2024 •

edited

Loading

codecov bot commented Jul 29, 2024 •

edited

Loading

varlogtim Aug 7, 2024 •

edited

Loading

varlogtim Aug 8, 2024 •

edited

Loading