[OIDC] Use username claimed from OIDC instead of "subiss" in the database

[julienlefur]

Is your feature request related to a problem? Please describe.
When login to Harbor using an OIDC Provider, the user existence is checked in the Harbor database by "subiss" which is a concatenation of the "sub" and "iss" field in the JWT token.
If the user is known, it is authenticated.

However, if the URL of the OIDC provider is modified or if the "sub" ID is renewed/changed, the user is not recognised and Harbor starts a new onboarding process for this user.

Describe the solution you'd like
The key used to retrieve the user from the database could be the username claimed from the OIDC provider. Looks like this is in progress in the PR #9311

Describe the main design/architecture of your solution
In the OIDC controller, in the callback method, instead of getting the user by "subiss", get the user by common.OIDCUserClaim so that any changes in the OIDC provider won't affect Harbor side.

harbor/src/core/controllers/oidc.go

Lines 113 to 116 in 0bc3241

	u, err := dao.GetUserBySubIss(info.Subject, info.Issuer)
	if err != nil {
	oc.SendInternalServerError(err)
	return

In the OIDC controller, in the onboard method, instead of register the user with "subiss", register the user with his username claimed which must be unique:

harbor/src/core/controllers/oidc.go

Lines 194 to 210 in 0bc3241

	oidcUser := models.OIDCUser{
	SubIss: d.Subject + d.Issuer,
	Secret: s,
	Token: t,
	}
	email := d.Email
	user := models.User{
	Username: username,
	Realname: d.Username,
	Email: email,
	GroupIDs: gids,
	OIDCUserMeta: &oidcUser,
	Comment: oidcUserComment,
	}
	err = dao.OnBoardOIDCUser(&user)

[renmaosheng]

Daniel, put this in your queue to take a look, thanks.

[reasonerjt]

@julienlefur I don't think it's a valid requirement, according to the openid spec, the sub is the unique identity of a user, if you update sub it's should be considered another user.

For the same reason, once the OIDC provider's URL is changed it should also be considered another issuer, just like you cannot assume "jack smith" in google and "jack smith" in azure are the same user.

If you want to them them as the same user, you must do some manual update in DB to make sure the sub and issuer map to your new OIDC provider

[julienlefur]

@reasonerjt Thanks for your reply.
We are integrating Harbor in our organization and we had to change the URL of our OIDC issuer (which is Keycloak by the way) and due to the actual implementation, we had to update the "subiss" column for all our users.

Why not create an other table referencing the OIDC issuer with the URL and map the users to the issuer using an internal ID? This way, the URL (or whatever OIDC parameters) could be changed when needed. The URL is only a way to access the OIDC issuer and it can change.

Concerning the "sub", I understand what you mean. However, the RFC 7519 spec says it must be unique but it can change while it remains unique. The RFC spec also says that the use of this claim is optional.
In our implementation, we use an LDAP to identify the users (it's our users federation). Keycloak generates and ID for each user synced from the LDAP.
Once, we had to restore on old version of the database of Keycloak because of an incident. So when the users started to login again, they obtained new IDs in Keycloak so new "sub" IDs in the JWT token. However, they were the same users.

In our organization, the username must be unique and this is what is identifying a user.
The claimed "username" can differ from one organization to the other but using it as the key in Harbor will give more flexibility and compatibility. When I change my OIDC issuer (URL, technology, whatever...) my users stay the same.

[reasonerjt]

I see your point, the root cause is not the sub has changed, it's because the URL of your OIDC provider changed so the issuer changed? So if we do not use subiss, instead only use sub to identify an oidc user it would fix your problem right?

I'd also like to point out the sub is required
https://openid.net/specs/openid-connect-core-1_0.html#IDToken
and RFC 7519 is an RFC for jwt not OIDC.

The username is even less reliable than sub because it's not even in the id token according to the spec.

[julienlefur]

@reasonerjt you're right about the RFC. My bad.
Yes I think only sub is definitively better than subiss.

But still, I'm not convinced that you need to store the sub 😊. It's like you don't trust the OIDC issuer. It says "hey here is the profile of the user" but you still check in your database if it's really the same user. You cannot have 2 sources of trust.

We implemented the OIDC with many other tools like Kubernetes, ElasticSearch, Gitlab, Grafana and none of them stores the sub field to check the user. They just get/claim whatever they need: a username, a group, an email, to identify the user.

[xaleeks]

@reasonerjt No I think his sub is also changing, and issuer is changing, so both can change but he wants to discard sub from being used as part of this key to authenticate against. And using just sub wouldn't solve his problem. Is it required or recommended best practice to use this 'sub' for authentication purpose. He brings up a good point that you are essentially double checking.
But I do think that change in both of these (sub and iss) constitute a 'change in identity' so re-verification is justified. @julienlefur we can look at some literature on how this is typically done if you have some

[julienlefur]

@reasonerjt Yes you can look at the kubernetes documentation concerning the OIDC configuration: https://kubernetes.io/docs/reference/access-authn-authz/authentication/

By default the JWT claim is sub to identify the user but an admin can modify the claim in order to use the username or email. When you choose to use a different claim like username, you don't use sub anymore.

[reasonerjt]

I can double check k8s implementation to understand how it handles it.

When Harbor get an id token in the callback it needs a way to determine if there is a user record in the DB mapping to the person associated with the id token or not. Currently it checks the sub and issuer

Before moving away from that we need a reliable way to do the mapping

I guess k8s has some way to map a claim in the token to a user. If k8s allows the claim to identify a user to change, all the relationship between a role and a user will be broken once the claim is updated, you may need to update all the rolebindings, essentially it's the same situation in Harbor, you update the DB to adjust the key to identify the users, so it maps again.

@julienlefur using username maybe good for your scenario.
But I don't think it's safe to assume 2 id token from 2 id providers with same username as the same person.

[julienlefur]

The big difference is that kubernetes trusts the OIDC issuer completely. It does not store any ID or token in a database. The "User" is composed of:

• oidc-username-claim: JWT claim to use as the user name
• oidc-username-prefix: Prefix prepended to username claims

If, as an administrator, I decide to set:
oidc-username-claim=username
oidc-username-prefix=oidc

The username for kubernetes will be: oidc:{{username}}
Any rolebinding would have to refer to that username to give roles to this user.

If I change the OIDC Issuer technology or URL, I can keep the same username and it will give the same role to that user because in my implementation it's the same user.

This ability to customize the integration of the OIDC is very important for many companies in my opinion. This could offer to Harbor a better integration in these companies.

In addition, you should always have only one oidc issuer at a time so that your issuer can ensure that the username is unique.

[reasonerjt]

I think for k8s, in your example if admin decides to update oidc-username-claim from username to sub, then rolebindings will be updated? Do you consider it an invalid use case?

I get your point we should make the "Primary Key" of ID token configurable, but we need to be careful what will happen when this "Primary Key" changes.

[julienlefur]

@reasonerjt In kubernetes, if the admin changes the claim he has to change all the rolebindings as well. But it's his choice and that is very important. Depending on the context, administrators can choose the way to customize the integration of Harbor: it increases the adaptivity of Harbor.

[Moulick]

@julienlefur were you able to get OIDC working between Gitlab and Harbor ?

[julienlefur]

@julienlefur were you able to get OIDC working between Gitlab and Harbor ?

@Moulick We are not using Gitlab as OIDC Provider but Keycloak.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[Morriz]

bump

[j-zimnowoda]

Hi, I see very interesting conversation here and I would like to put my two cents.

First of all, I think that using sub scope for user identification is quite good approach, because an email or username can be changed in IdP (e.g: changing a surname).

However, I do not understand the fact that username and email are also required to be unique during the user onboarding process. I am scratching my head while thinking about how many unique identifiers Harbor needs for a single users 🤔

If I migrate my users to a new IdP and change Harbor OIDC settings, I still would like these users to be able to onboard in Harbor. That would be great for those who leverage Harbor with OIDC groups - I hope, I am not too selfish now 👀
Unfortunately users can't onboard, because of Conflict, the user with same username or email has been onboarded. error 🆘

It would be great if Harbor provides some guideline about changing OIDC Provider and migrating Harbor users.

[github-actions]

This issue is being marked stale due to a period of inactivity. If this issue is still relevant, please comment or remove the stale label. Otherwise, this issue will close in 30 days.

[Morriz]

bump

[github-actions]

This issue is being marked stale due to a period of inactivity. If this issue is still relevant, please comment or remove the stale label. Otherwise, this issue will close in 30 days.

[github-actions]

This issue was closed because it has been stalled for 30 days with no activity. If this issue is still relevant, please re-open a new issue.