[KS-OIDC] Remove special characters form sub OIDC standard claim #5018

ghost · 2022-06-30T14:20:16Z

Issue

Users cannot login using an OIDC identity provider.

Kubesphere version

3.2.1

Error Message

Logs form ks-apiserver

{
  code: 422,
  statusText: 'Unprocessable Entity',
  message: `User.iam.kubesphere.io "{UNLUCKY-USER}" is invalid: metadata.labels: Invalid value: "{UNLUCK-USER_OIDC_SUB-StartOrEnd-Special-Char}": a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue',  or 'my_value',  or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')\n`
}

Cause

The standard claim sub in OIDC specs https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims is used as label iam.kubesphere.io/origin-uid in https://github.com/kubesphere/kubesphere/blob/master/pkg/models/auth/authenticator.go#L82 , for the case OIDC as identity provided referenced in https://github.com/kubesphere/kubesphere/blob/master/pkg/apiserver/authentication/identityprovider/oidc/oidc.go#L118

Kubernetes doesn't allow special characters in start and end of labels value as for https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set

Proposed Workaround

Since is OIDC claim cannot be substituted by another claim , the proposed solution is to replace any special characters at start or end with 0

Solution

Replace the label iam.kubesphere.io/origin-uid with unique generated UID based on combining OIDC claims while respecting Kubernetes labels standards.

ks-ci-bot · 2022-06-30T14:20:28Z

@najib-houcine: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ks-ci-bot · 2022-06-30T14:20:36Z

Hi @najib-houcine. Thanks for your PR.

I'm waiting for a kubesphere member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

wansir · 2022-07-01T05:45:47Z

@najib-houcine Thanks for your contribution. These changes seem to have some risks, removing the special characters directly would cause duplication of uid, it cannot be tolerated. It's better to use base64.RawURLEncoding.EncodeToString here.

ghost · 2022-07-04T07:56:44Z

@wansir I took a look into it and base64 encoding will grantee an == at the end of orgin_uid label , which will fail any user with good or bad Sub.

wansir · 2022-07-04T08:36:31Z

@wansir I took a look into it and base64 encoding will grantee an == at the end of orgin_uid label , which will fail any user with good or bad Sub.

RawURLEncoding is the unpadded alternate base64 encoding defined in RFC 4648. It is typically used in URLs and file names. This is the same as URLEncoding but omits padding characters.

https://pkg.go.dev/encoding/base64@master#pkg-variables

BTW, the length of base64-encoded uid will be limited to 63 characters, I think it can be accepted. And we should convert the uid labels after the upgrade.

ghost · 2022-07-04T09:11:55Z

Thanks @wansir , I updated the PR

wansir · 2022-07-04T09:14:06Z

/ok-to-test

wansir · 2022-07-04T10:09:51Z

@najib-houcine Please take a look at the failed CI jobs.

FAILED verify-gofmt.sh 2s
Verifying verify-goimports.sh
The following files are not import formatted
./pkg/apiserver/authentication/identityprovider/oidc/oidc.go
Please run the following command:
make goimports

ghost · 2022-07-04T13:46:55Z

@wansir , Thanks I checked and managed to fix the verification step but unit-test keep on-failing , could you check what wrong ?

wansir · 2022-07-05T01:49:29Z

pkg/apiserver/authentication/identityprovider/oidc/oidc_test.go

@@ -198,7 +198,7 @@ var _ = Describe("OIDC", func() {
 			req := &http.Request{URL: url}
 			identity, err := provider.IdentityExchangeCallback(req)
 			Expect(err).Should(BeNil())
-			Expect(identity.GetUserID()).Should(Equal("110169484474386276334"))
+			Expect(string(base64.URLEncoding.DecodeString(identity.GetUserID()))).Should(Equal("110169484474386276334"))


Suggested change

Expect(string(base64.URLEncoding.DecodeString(identity.GetUserID()))).Should(Equal("110169484474386276334"))

Expect(identity.GetUserID()).Should(Equal(base64.RawURLEncoding.EncodeToString([]byte("110169484474386276334"))))

wansir · 2022-07-05T10:21:00Z

@wansir , Thanks I checked and managed to fix the verification step but unit-test keep on-failing , could you check what wrong ?

vet: pkg/apiserver/authentication/identityprovider/oidc/oidc_test.go:201:18: 2-valued base64.URLEncoding.DecodeString(identity.GetUserID()) (value of type ([]byte, error)) where single value is expected

@najib-houcine Refer to https://prow.kubesphere.io/view/s3/prow-logs/pr-logs/pull/kubesphere_kubesphere/5018/pull-kubesphere-unit-test/1543948976712060928

ghost · 2022-07-05T10:31:56Z

Thanks @wansir for the hint , but unit test failed also https://prow.kubesphere.io/view/s3/prow-logs/pr-logs/pull/kubesphere_kubesphere/5018/pull-kubesphere-unit-test/1544265767887835136 I will investigate more into it.

wansir · 2022-07-05T10:47:58Z

/retest

wansir · 2022-07-06T10:10:09Z

@najib-houcine

You can reformat the code using make goimports

ghost · 2022-07-06T11:52:16Z

@wansir seems good now .

wansir · 2022-07-07T03:00:41Z

/lgtm
/approve

ks-ci-bot · 2022-07-07T03:00:47Z

LGTM label has been added.

Git tree hash: 98e617509679bf09756783c50bf36a6b6dd5a745

ks-ci-bot · 2022-07-07T03:00:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: najib-houcine, wansir

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~~ [wansir]

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

…aim (kubesphere#5018)" This reverts commit 45a0625.

Revert "[KS-OIDC] Remove special characters form sub OIDC standard claim (#5018)" This reverts commit 45a0625.

Revert "[KS-OIDC] Remove special characters form sub OIDC standard claim (kubesphere#5018)" This reverts commit 45a0625.

@wansir

…esphere#5018) * [KS-OIDC] Remove special characters form sub OIDC standard claim * [KS-OIDC] Change to base64 RawURLEncoding * [KS-OIDC] Import encoding/base64 * [KS-OIDC] Change import * [KS-OIDC] Damn Go * [KS-OIDC] Damn Spaces * [KS-OIDC] Backport to test * [KS-OIDC] Backport to test: the other way * [KS-OIDC] Backport to test: convert to string * [KS-OIDC] Backport to test: Hint from @wansir * [KS-OIDC] Backport to test: Damn Space

Revert "[KS-OIDC] Remove special characters form sub OIDC standard claim (kubesphere#5018)" This reverts commit 45a0625.

[KS-OIDC] Remove special characters form sub OIDC standard claim

1186a4f

ks-ci-bot added do-not-merge/release-note-label-needed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-ok-to-test labels Jun 30, 2022

[KS-OIDC] Change to base64 RawURLEncoding

0eaeded

ks-ci-bot added ok-to-test and removed needs-ok-to-test labels Jul 4, 2022

najib-houcine added 3 commits July 4, 2022 11:17

[KS-OIDC] Import encoding/base64

f5b6c68

[KS-OIDC] Change import

e1fcac0

[KS-OIDC] Damn Go

637e201

najib-houcine added 4 commits July 4, 2022 14:04

[KS-OIDC] Damn Spaces

f5d037f

[KS-OIDC] Backport to test

401a1a2

[KS-OIDC] Backport to test: the other way

5bd9ff2

[KS-OIDC] Backport to test: convert to string

38266e4

wansir reviewed Jul 5, 2022

View reviewed changes

[KS-OIDC] Backport to test: Hint from @wansir

41585ed

[KS-OIDC] Backport to test: Damn Space

763c521

ks-ci-bot assigned wansir Jul 7, 2022

ks-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2022

ks-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2022

ks-ci-bot merged commit 45a0625 into kubesphere:master Jul 7, 2022

zhou1203 mentioned this pull request Feb 22, 2023

User ID of Third-party users with special characters cannot register in KS #5547

Closed

zhou1203 added a commit to zhou1203/kubesphere that referenced this pull request Feb 22, 2023

Revert "[KS-OIDC] Remove special characters form sub OIDC standard cl…

4c293f0

…aim (kubesphere#5018)" This reverts commit 45a0625.

ks-ci-bot pushed a commit that referenced this pull request Feb 27, 2023

Revert PR #5018 (#5548)

9dad732

Revert "[KS-OIDC] Remove special characters form sub OIDC standard claim (#5018)" This reverts commit 45a0625.

sekfung pushed a commit to sekfung/kubesphere that referenced this pull request Mar 2, 2023

Revert PR kubesphere#5018 (kubesphere#5548)

9261f14

Revert "[KS-OIDC] Remove special characters form sub OIDC standard claim (kubesphere#5018)" This reverts commit 45a0625.

sologgfun pushed a commit to sologgfun/kubesphere that referenced this pull request Apr 24, 2023

Revert PR kubesphere#5018 (kubesphere#5548)

3771f7e

Revert "[KS-OIDC] Remove special characters form sub OIDC standard claim (kubesphere#5018)" This reverts commit 45a0625.

wanjunlei pushed a commit to wanjunlei/kubesphere that referenced this pull request Aug 11, 2023

Revert PR kubesphere#5018 (kubesphere#5548)

4b4c55b

Revert "[KS-OIDC] Remove special characters form sub OIDC standard claim (kubesphere#5018)" This reverts commit 45a0625.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KS-OIDC] Remove special characters form sub OIDC standard claim #5018

[KS-OIDC] Remove special characters form sub OIDC standard claim #5018

ghost commented Jun 30, 2022

ks-ci-bot commented Jun 30, 2022

ks-ci-bot commented Jun 30, 2022

wansir commented Jul 1, 2022

ghost commented Jul 4, 2022

wansir commented Jul 4, 2022

ghost commented Jul 4, 2022

wansir commented Jul 4, 2022

wansir commented Jul 4, 2022

ghost commented Jul 4, 2022 •

edited by wansir

Loading

wansir Jul 5, 2022

wansir commented Jul 5, 2022

ghost commented Jul 5, 2022

wansir commented Jul 5, 2022

wansir commented Jul 6, 2022

ghost commented Jul 6, 2022

wansir commented Jul 7, 2022

ks-ci-bot commented Jul 7, 2022

ks-ci-bot commented Jul 7, 2022

	Expect(string(base64.URLEncoding.DecodeString(identity.GetUserID()))).Should(Equal("110169484474386276334"))
	Expect(identity.GetUserID()).Should(Equal(base64.RawURLEncoding.EncodeToString([]byte("110169484474386276334"))))

[KS-OIDC] Remove special characters form sub OIDC standard claim #5018

[KS-OIDC] Remove special characters form sub OIDC standard claim #5018

Conversation

ghost commented Jun 30, 2022

Issue

Kubesphere version

Error Message

Cause

Proposed Workaround

Solution

ks-ci-bot commented Jun 30, 2022

ks-ci-bot commented Jun 30, 2022

wansir commented Jul 1, 2022

ghost commented Jul 4, 2022

wansir commented Jul 4, 2022

ghost commented Jul 4, 2022

wansir commented Jul 4, 2022

wansir commented Jul 4, 2022

ghost commented Jul 4, 2022 • edited by wansir Loading

wansir Jul 5, 2022

Choose a reason for hiding this comment

wansir commented Jul 5, 2022

ghost commented Jul 5, 2022

wansir commented Jul 5, 2022

wansir commented Jul 6, 2022

ghost commented Jul 6, 2022

wansir commented Jul 7, 2022

ks-ci-bot commented Jul 7, 2022

ks-ci-bot commented Jul 7, 2022

ghost commented Jul 4, 2022 •

edited by wansir

Loading