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

sub claim on Corppass ID token is missing uuid and has unexpected u #670

Open
cflee opened this issue May 27, 2024 · 2 comments
Open

sub claim on Corppass ID token is missing uuid and has unexpected u #670

cflee opened this issue May 27, 2024 · 2 comments

Comments

@cflee
Copy link
Contributor

cflee commented May 27, 2024

Describe the bug
sub claim on Corppass ID token is missing uuid field, and the u field should be different when a given user logs in to different UENs

To Reproduce
Steps to reproduce the behavior:

  1. Go through the authorise flow
  2. Exchange the auth code with token endpoint to receive the ID token and access token
  3. Inspect the ID token, it is something like s=F1234567P, u=0f14a2fc-09c2-4780-95f0-8c28347f2780, c=SG

Expected behavior
ID token's sub claim should match what is described in the Corppass spec:

The principal that is the subject of the JWT. Contains a comma-separated, key=value mapping that identifies the user; possibly including multiple alternate IDs representing the user. The format is "s=Identity ID (e.g. NRIC/FIN/Foreign ID), uuid=User’s globally unique identifier (i.e. 0f14a2fc-09c2-4780-95f0-8c28347f2780), u=System defined ID (i.e. CP1234), c=2-character country code (e.g. SG)".
Example: "sub": "s=F1234567P, uuid=0f14a2fc-09c2-4780-95f0-8c28347f2780, u=CP192, c=SG"
Refer to ISO 3166-1 Alpha-2 codes for list of expected codes. {+}25https://tools.ietf.org/html/rfc7519#section-4.1.2+

Also, from a previous comment #615 (comment) (on a different issue):

Actual (redacted) output from CP staging 'sub' response.
's=F..., uuid=..0..b..-...7-..., u=CP200..., c=SG'

The uuid field should be present in case there are systems that expect or consume it. It is unclear whether this UUID is expected to be the same for a given user across different UENs, but it seems like a reasonable guess for now.

The u field is present, but it is not unique per user-and-UEN combination. From some folks' testing against Corppass staging, they report that they receive a different u value for the same UIN logging into a different UEN to the same Corppass relying party. The different behaviour in Mockpass causes problems for systems that rely on receiving a unique u field value per user-and-UEN combination.

@cflee
Copy link
Contributor Author

cflee commented May 27, 2024

I have a proposed patch but I am not submitting it as a PR yet, as I am still working on getting it tested with a Corppass relying party, and trying to collect more data to confirm whether the uuid field needs to be made unique per user-and-UEN combination. I'm not sure if/when that will be done, so I'm leaving this here first in case someone else could use it.

From 7b9a29c93c63e8d5f53b45ea1c7d82117e7e2128 Mon Sep 17 00:00:00 2001
From: Lee Chiang Fong <[email protected]>
Date: Thu, 23 May 2024 18:57:28 +0800
Subject: [PATCH] fix: corppass id token sub

---
 lib/assertions.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/assertions.js b/lib/assertions.js
index 1ba6d17..cf48fd6 100644
--- a/lib/assertions.js
+++ b/lib/assertions.js
@@ -171,7 +171,7 @@ const oidc = {
         aud,
       }

-      const sub = `s=${nric},u=${uuid},c=SG`
+      const sub = `s=${nric},uuid=${uuid},u=${uen}${nric},c=SG`

       const accessTokenClaims = {
         ...baseClaims,
--
2.39.3 (Apple Git-146)

@cflee
Copy link
Contributor Author

cflee commented Jun 19, 2024

From some folks' testing against Corppass staging, they report that they receive a different u value for the same UIN logging into a different UEN to the same Corppass relying party.

Apparently sometimes the same UIN and same UEN to the same system results in a different u=... value from Corppass production, so something is wrong with our understanding – will hold off on submitting this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant