Skip to content
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.

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

Allow for both "sub" claim formats to be used in login #1616

Merged
merged 5 commits into from
Jan 25, 2024

Conversation

ivan-degtiarenko
Copy link
Contributor

@ivan-degtiarenko ivan-degtiarenko commented Jan 23, 2024

Description

This PR is based on next assumptions:

  • There's ongoing process of switching from old sub claim format to new one
  • Console dot is returning sub in new format under sub claim name; it will return sub in old format under deprecated_sub claim name
  • When using dynamic client, sso.r.c returns sub in old format; at some point switch will be made to return sub in new format

This PR adds auth provider groups for both old and new format so that the owner of an instance can login whatever format is returned from sso.r.c.

Note: this PR only solves the problem for new instances. Already existing instances will continue to work until a switch in dynamic clients happens. This case should be handled in a separate PR.

Checklist (Definition of Done)

  • Unit and integration tests added
  • Added test description under Test manual
  • Documentation added if necessary (i.e. changes to dev setup, test execution, ...)
  • CI and all relevant tests are passing
  • Add the ticket number to the PR title if available, i.e. ROX-12345: ...
  • Discussed security and business related topics privately. Will move any security and business related topics that arise to private communication channel.
  • Add secret to app-interface Vault or Secrets Manager if necessary
  • RDS changes were e2e tested manually
  • Check AWS limits are reasonable for changes provisioning new resources

Test manual

TODO: Add manual testing efforts

# To run tests locally run:
make db/teardown db/setup db/migrate
make ocm/setup
make verify lint binary test test/integration

@ivan-degtiarenko
Copy link
Contributor Author

/retest

Copy link

@dhaus67 dhaus67 left a comment

Choose a reason for hiding this comment

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

A couple of things on the noted changes of the claims you mentioned returned by the console dot client:

The user_id claim value currently is used within the telemetery parts to record the user and associate them with a Central. In case of switching the claim values to the old f:<id>:<id> format, this will now be borked and present wrong data. We have to have some idea around this instead (seeing as the previous user_id value will now be the sub value, changing this can be considered IMO).

Also, what'd be nice to call out in the description is the planned timeline: when will the claim be available within the respective environments?

@@ -44,6 +44,9 @@ type CentralRequest struct {
OwnerAccountID string `json:"owner_account_id"`
// OwnerUserID is the subject claim (confusingly it is NOT the user_id claim) of the Red Hat SSO token.
OwnerUserID string `json:"owner_user_id"`
// OwnerAlternativeUserID is the user_id claim of the Red Hat SSO token.
// It is introduced to allow for a seamless migration of the subject claim format.
OwnerAlternativeUserID string `json:"owner_alternative_user_id"`
Copy link

Choose a reason for hiding this comment

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

nit: I think the proper naming (and also aligning with the language we use in the ACS claims stuff) would be OwnerAlternateUserID.

"gorm.io/gorm"
)

func addAlternativeUserIDFieldToCentralRequests() *gormigrate.Migration {
Copy link

Choose a reason for hiding this comment

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

Hmm, how will this migration help in the following case:

  • A Central has been created with the "old" sub format (i.e. owner ID will be set to "f::").
  • The new sub format is now being returned by the console client and the dynamic SSO client.
  • After the migration, the value for OwnerAlternativeUserID will be empty, since this is only populated during creation.

Now, when a user attempts to login, the following happens:

  • A group exists which will map userid to the old f:<id>:<id> format. The owner ID of the Central hasn't changed.
  • Login will be denied, since the SSO client will now return the new sub format, which won't be a match.
  • The user won't be able to login as administrator within their instance and is effectively locked out.

So, this only saves when a new instance has been created after the sub format changes have been introduced, however it does not save existing instances that have been created before the format changes.

To me, it seems like we have handled the "console dot side" by ensuring both formats are present on the token, however the current plan for handling the dynamic client token claim values is insufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, for existing instances we need to either find a way to obtain sub in new format or use deprecated_sub claim in dynamic clients

@openshift-ci openshift-ci bot added the lgtm label Jan 25, 2024
Copy link
Contributor

openshift-ci bot commented Jan 25, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhaus67, ivan-degtiarenko

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [dhaus67,ivan-degtiarenko]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ivan-degtiarenko ivan-degtiarenko merged commit 6b013f9 into main Jan 25, 2024
9 checks passed
@ivan-degtiarenko ivan-degtiarenko deleted the ivan/acs-login-alternative-user-id branch January 25, 2024 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants