opengovsg / mockpass

A mock SingPass/CorpPass/MyInfo server for dev purposes
https://blog.data.gov.sg/mockpass-a-mock-singpass-corppass-server-for-testing-in-development-a583193c898c
MIT License
83 stars 82 forks source link

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

Open cflee opened 3 months ago

cflee commented 3 months ago

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 https://github.com/opengovsg/mockpass/issues/615#issuecomment-1794139122 (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 commented 3 months ago

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 <lee_chiang_fong@tech.gov.sg>
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 commented 3 months ago

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.