Skip to content

Commit e68167f

Browse files
timothyatertonclaudeaterga
authored
feat(be): OpenIdCredentialKey with data migration — add aud for SSO security (dfinity#3784)
## Summary Add the `aud` (audience / client_id) field to `OpenIdCredentialKey`, changing it from `(iss, sub)` to `(iss, sub, aud)`. This is a security prerequisite for SSO: since SSO allows anyone to provide a `client_id` via their `ii-openid-configuration` endpoint, without `aud` in the key two different OIDC clients at the same provider with the same user `sub` would collide, enabling impersonation. ## Changes - **Type update**: `OpenIdCredentialKey` type alias changed from `(Iss, Sub)` to `(Iss, Sub, Aud)` in both `internet_identity_interface` and the `openid` module - **CBOR encoding**: `StorableOpenIdCredentialKey` rewritten with manual `Encode`/`Decode` impls — new entries use CBOR map format `{0:iss, 1:sub, 2:aud}`; the decoder also handles legacy CBOR array format `[iss, sub]` for backward compatibility - **Migration**: `post_upgrade` drains the credential key index via `pop_first`, resolves `aud` from each anchor's `StorableOpenIdCredential` (which already stores `aud` at CBOR index `#[n(2)]`), and re-inserts with the complete `(iss, sub, aud)` key. Unresolvable entries are preserved with empty `aud` for retry on next upgrade. - **Key construction**: Updated `OpenIdCredential::key()`, `StorableOpenIdCredential::key()`, `calculate_delegation_seed()`, and all call sites - **Candid interface**: Updated `.did` file and generated JS/TS declarations - **Frontend**: Updated credential removal call to pass `aud` - **Tests**: Added unit tests for new CBOR map encoding, legacy array decoding, and round-trip serialization. Updated existing test assertions to use 3-tuple keys. ## Delegation seed backward compatibility The `calculate_delegation_seed` function already receives `client_id` (which equals `aud`) as a separate parameter. The seed calculation is unchanged — `aud` from the key tuple is ignored (`_aud`) in the destructuring, preserving identical `Principal` derivation for existing credentials. ## Migration safety - Uses `pop_first()` to drain the BTreeMap, avoiding byte-level encoding mismatches between legacy array-encoded keys and new map-encoded keys - Resolves `aud` from the anchor's stored `StorableOpenIdCredential` which already has `aud` at CBOR index 2 - Falls back to re-inserting with empty `aud` if resolution fails, with a logged warning — the entry is preserved for retry on next upgrade - Idempotent: safe to run on every upgrade; entries already in the new format are preserved unchanged ## Test plan - [x] All 209 unit tests pass (including Candid interface compatibility) - [ ] Integration tests (require canister WASM build — pass in CI) - [ ] Deploy to testnet and verify migration of existing credentials - [ ] Verify credential lookup works after migration - [ ] Verify new credential registration includes `aud` in key --- [< Previous PR](dfinity#3778) | [Next PR >](dfinity#3785) --------- Co-authored-by: Claude Agent <noreply@anthropic.com> Co-authored-by: Arshavir Ter-Gabrielyan <arshavir.ter.gabrielyan@dfinity.org>
1 parent 6e660b1 commit e68167f

15 files changed

Lines changed: 626 additions & 37 deletions

File tree

src/frontend/src/lib/generated/internet_identity_idl.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -481,7 +481,7 @@ export const idlFactory = ({ IDL }) => {
481481
'Unauthorized' : IDL.Principal,
482482
'JwtVerificationFailed' : IDL.Null,
483483
});
484-
const OpenIdCredentialKey = IDL.Tuple(Iss, Sub);
484+
const OpenIdCredentialKey = IDL.Tuple(Iss, Sub, Aud);
485485
const OpenIdCredentialRemoveError = IDL.Variant({
486486
'InternalCanisterError' : IDL.Text,
487487
'OpenIdCredentialNotFound' : IDL.Null,

src/frontend/src/lib/generated/internet_identity_types.d.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -932,7 +932,7 @@ export type OpenIdCredentialAddError = {
932932
{ 'JwtExpired' : null } |
933933
{ 'Unauthorized' : Principal } |
934934
{ 'JwtVerificationFailed' : null };
935-
export type OpenIdCredentialKey = [Iss, Sub];
935+
export type OpenIdCredentialKey = [Iss, Sub, Aud];
936936
export type OpenIdCredentialRemoveError = { 'InternalCanisterError' : string } |
937937
{ 'OpenIdCredentialNotFound' : null } |
938938
{ 'Unauthorized' : Principal };

src/frontend/src/routes/(new-styling)/manage/(authenticated)/(access-and-recovery)/access/+page.svelte

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,7 @@
187187
.openid_credential_remove($authenticatedStore.identityNumber, [
188188
removingAccessMethod.openid.iss,
189189
removingAccessMethod.openid.sub,
190+
removingAccessMethod.openid.aud,
190191
])
191192
.then(throwCanisterError);
192193
}

src/frontend/src/routes/(new-styling)/manage/(authenticated)/(access-and-recovery)/access/utils.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,8 @@ export const toAccessMethods = (
5555

5656
/**
5757
* Returns a unique string key for an access method.
58-
* Uses credential ID for passkeys and issuer+subject for OpenID.
58+
* Uses credential ID for passkeys and a JSON-encoded (iss, sub, aud) triple
59+
* for OpenID so components can't collide via concatenation.
5960
*/
6061
export const toKey = (accessMethod: AccessMethod): string => {
6162
if (
@@ -67,7 +68,11 @@ export const toKey = (accessMethod: AccessMethod): string => {
6768
);
6869
}
6970
if ("openid" in accessMethod) {
70-
return accessMethod.openid.iss + accessMethod.openid.sub;
71+
return JSON.stringify([
72+
accessMethod.openid.iss,
73+
accessMethod.openid.sub,
74+
accessMethod.openid.aud,
75+
]);
7176
}
7277
throw new Error("Unknown access method type");
7378
};

src/internet_identity/internet_identity.did

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -403,7 +403,7 @@ type OidcConfig = record {
403403
issuer : opt text;
404404
};
405405

406-
type OpenIdCredentialKey = record { Iss; Sub };
406+
type OpenIdCredentialKey = record { Iss; Sub; Aud };
407407

408408
type AnalyticsConfig = variant {
409409
Plausible : record {

src/internet_identity/src/anchor_management.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,7 @@ pub fn remove_openid_credential(
229229
key: &OpenIdCredentialKey,
230230
) -> Result<Operation, AnchorError> {
231231
anchor.remove_openid_credential(key)?;
232-
let (iss, _) = key;
232+
let (iss, _, _) = key;
233233
Ok(Operation::RemoveOpenIdCredential { iss: iss.clone() })
234234
}
235235

src/internet_identity/src/attributes.rs

Lines changed: 49 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -978,7 +978,11 @@ mod tests {
978978
assert!(requested.is_empty());
979979

980980
let attrs = result
981-
.get(&(GOOGLE_ISSUER.to_string(), "google-user-123".to_string()))
981+
.get(&(
982+
GOOGLE_ISSUER.to_string(),
983+
"google-user-123".to_string(),
984+
"test-client-id".to_string(),
985+
))
982986
.expect("google verified email attributes should be present");
983987

984988
pretty_assert_eq!(attrs.len(), 1);
@@ -1011,7 +1015,11 @@ mod tests {
10111015
assert!(requested.is_empty());
10121016

10131017
let attrs = result
1014-
.get(&(GOOGLE_ISSUER.to_string(), "google-user-123".to_string()))
1018+
.get(&(
1019+
GOOGLE_ISSUER.to_string(),
1020+
"google-user-123".to_string(),
1021+
"test-client-id".to_string(),
1022+
))
10151023
.expect("credential key should be present");
10161024
pretty_assert_eq!(
10171025
attrs.len(),
@@ -1034,7 +1042,11 @@ mod tests {
10341042
assert!(requested.is_empty());
10351043

10361044
let attrs = result
1037-
.get(&(GOOGLE_ISSUER.to_string(), "google-user-123".to_string()))
1045+
.get(&(
1046+
GOOGLE_ISSUER.to_string(),
1047+
"google-user-123".to_string(),
1048+
"test-client-id".to_string(),
1049+
))
10381050
.expect("credential key should be present");
10391051
pretty_assert_eq!(
10401052
attrs.len(),
@@ -1057,7 +1069,11 @@ mod tests {
10571069
assert!(requested.is_empty());
10581070

10591071
let attrs = result
1060-
.get(&(GOOGLE_ISSUER.to_string(), "google-user-123".to_string()))
1072+
.get(&(
1073+
GOOGLE_ISSUER.to_string(),
1074+
"google-user-123".to_string(),
1075+
"test-client-id".to_string(),
1076+
))
10611077
.expect("credential key should be present");
10621078
pretty_assert_eq!(
10631079
attrs.len(),
@@ -1087,7 +1103,11 @@ mod tests {
10871103
assert!(requested.is_empty());
10881104

10891105
let attrs = result
1090-
.get(&(GOOGLE_ISSUER.to_string(), "google-user-123".to_string()))
1106+
.get(&(
1107+
GOOGLE_ISSUER.to_string(),
1108+
"google-user-123".to_string(),
1109+
"test-client-id".to_string(),
1110+
))
10911111
.expect("credential key should be present");
10921112
pretty_assert_eq!(
10931113
attrs.len(),
@@ -1123,7 +1143,11 @@ mod tests {
11231143
assert!(requested.is_empty());
11241144

11251145
let attrs = result
1126-
.get(&(GOOGLE_ISSUER.to_string(), "google-user-123".to_string()))
1146+
.get(&(
1147+
GOOGLE_ISSUER.to_string(),
1148+
"google-user-123".to_string(),
1149+
"test-client-id".to_string(),
1150+
))
11271151
.expect("credential key should be present");
11281152
pretty_assert_eq!(
11291153
attrs.len(),
@@ -1157,6 +1181,7 @@ mod tests {
11571181
.get(&(
11581182
MICROSOFT_RESOLVED_ISSUER.to_string(),
11591183
"ms-user-456".to_string(),
1184+
"test-client-id".to_string(),
11601185
))
11611186
.expect("microsoft verified email attributes should be present");
11621187

@@ -1200,7 +1225,11 @@ mod tests {
12001225
assert!(requested.is_empty());
12011226

12021227
let attrs = result
1203-
.get(&(enterprise_iss, "enterprise-user".to_string()))
1228+
.get(&(
1229+
enterprise_iss,
1230+
"enterprise-user".to_string(),
1231+
"test-client-id".to_string(),
1232+
))
12041233
.expect("credential key should be present");
12051234
pretty_assert_eq!(
12061235
attrs.len(),
@@ -1259,6 +1288,7 @@ mod tests {
12591288
.get(&(
12601289
MICROSOFT_RESOLVED_ISSUER.to_string(),
12611290
"ms-user-456".to_string(),
1291+
"test-client-id".to_string(),
12621292
))
12631293
.expect("credential key should be present");
12641294
pretty_assert_eq!(
@@ -1310,7 +1340,11 @@ mod tests {
13101340
pretty_assert_eq!(result.len(), 2, "both credentials should be present");
13111341

13121342
let google_attrs = result
1313-
.get(&(GOOGLE_ISSUER.to_string(), "google-user-123".to_string()))
1343+
.get(&(
1344+
GOOGLE_ISSUER.to_string(),
1345+
"google-user-123".to_string(),
1346+
"test-client-id".to_string(),
1347+
))
13141348
.expect("google attributes");
13151349
pretty_assert_eq!(
13161350
attribute_pairs(google_attrs),
@@ -1324,6 +1358,7 @@ mod tests {
13241358
.get(&(
13251359
MICROSOFT_RESOLVED_ISSUER.to_string(),
13261360
"ms-user-456".to_string(),
1361+
"test-client-id".to_string(),
13271362
))
13281363
.expect("microsoft attributes");
13291364
pretty_assert_eq!(
@@ -1367,7 +1402,11 @@ mod tests {
13671402
assert!(requested.is_empty());
13681403

13691404
let attrs = result
1370-
.get(&(GOOGLE_ISSUER.to_string(), "google-user-123".to_string()))
1405+
.get(&(
1406+
GOOGLE_ISSUER.to_string(),
1407+
"google-user-123".to_string(),
1408+
"test-client-id".to_string(),
1409+
))
13711410
.expect("google attributes");
13721411

13731412
pretty_assert_eq!(attrs.len(), 3, "all three attributes should be returned");
@@ -1540,6 +1579,7 @@ mod tests {
15401579
.get(&(
15411580
MICROSOFT_RESOLVED_ISSUER.to_string(),
15421581
"ms-user-456".to_string(),
1582+
"test-client-id".to_string(),
15431583
))
15441584
.expect("credential key should be present");
15451585
pretty_assert_eq!(attrs.len(), 1, "email attribute should be returned");

src/internet_identity/src/main.rs

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,9 @@ use ic_canister_sig_creation::signature_map::LABEL_SIG;
1313
use ic_cdk::api::{caller, set_certified_data, trap};
1414
use ic_cdk::call;
1515
use ic_cdk_macros::{init, post_upgrade, pre_upgrade, query, update};
16+
use ic_cdk_timers::TimerId;
17+
use std::cell::RefCell;
18+
use std::time::Duration;
1619

1720
use internet_identity_interface::archive::types::{BufferedEntry, Operation};
1821
use internet_identity_interface::http_gateway::{HttpRequest, HttpResponse};
@@ -75,6 +78,95 @@ const INTERNETCOMPUTER_ORG_ORIGIN: &str = "https://identity.internetcomputer.org
7578
const ID_AI_DOMAIN: &str = "id.ai";
7679
const ID_AI_ORIGIN: &str = "https://id.ai";
7780

81+
// ---- OpenID credential key migration (temporary, see PR #3784) ----
82+
//
83+
// The `OpenIdCredentialKey` type grew an `aud` field. Existing index entries
84+
// were written in the legacy `(iss, sub)` CBOR array shape and must be
85+
// upgraded to the new `(iss, sub, aud)` CBOR map shape. Upgrading all entries
86+
// synchronously in `post_upgrade` would blow the instruction limit on II's
87+
// production canister, so we batch the work via an interval timer using the
88+
// same convention as the prior anchor migration (#3713).
89+
90+
/// How long to wait between migration batches.
91+
const OIDC_KEY_MIGRATION_BATCH_BACKOFF_SECONDS: Duration = Duration::from_secs(1);
92+
93+
/// Maximum number of index entries to upgrade per batch (= per ingress message).
94+
/// Matches the 2 000-anchor-per-batch convention used for previous migrations.
95+
const OIDC_KEY_MIGRATION_BATCH_SIZE: u64 = 2_000;
96+
97+
thread_local! {
98+
// TODO: Remove these after the data migration is complete.
99+
static OIDC_KEY_MIGRATION_DONE: RefCell<bool> = const { RefCell::new(false) };
100+
static OIDC_KEY_MIGRATION_PROCESSED: RefCell<u64> = const { RefCell::new(0) };
101+
static OIDC_KEY_MIGRATION_ERRORS: RefCell<Vec<String>> = const { RefCell::new(Vec::new()) };
102+
static OIDC_KEY_MIGRATION_TIMER_ID: RefCell<Option<TimerId>> = const { RefCell::new(None) };
103+
}
104+
105+
/// Temporary hidden endpoint: returns any orphan-entry errors encountered
106+
/// during the OpenID credential key migration.
107+
#[update(hidden = true)]
108+
fn list_oidc_key_migration_errors() -> Vec<String> {
109+
OIDC_KEY_MIGRATION_ERRORS.with_borrow(|errors| errors.clone())
110+
}
111+
112+
/// Temporary hidden endpoint: returns `(processed_entries, is_done)` so
113+
/// monitoring can track migration progress.
114+
#[query(hidden = true)]
115+
fn oidc_key_migration_status() -> (u64, bool) {
116+
(
117+
OIDC_KEY_MIGRATION_PROCESSED.with_borrow(|c| *c),
118+
OIDC_KEY_MIGRATION_DONE.with_borrow(|d| *d),
119+
)
120+
}
121+
122+
/// Process one batch of the OpenID credential key migration. Bound to the
123+
/// interval timer set up in [`init_oidc_key_migration_timer`]; clears the
124+
/// timer once the migration signals completion.
125+
fn run_oidc_key_migration_batch() {
126+
if OIDC_KEY_MIGRATION_DONE.with_borrow(|done| *done) {
127+
return;
128+
}
129+
130+
let outcome = state::storage_borrow_mut(|storage| {
131+
storage.migrate_openid_credential_keys_batch(OIDC_KEY_MIGRATION_BATCH_SIZE)
132+
});
133+
134+
OIDC_KEY_MIGRATION_PROCESSED.with_borrow_mut(|count| {
135+
*count = count.saturating_add(outcome.processed);
136+
});
137+
if !outcome.errors.is_empty() {
138+
OIDC_KEY_MIGRATION_ERRORS.with_borrow_mut(|errors| errors.extend(outcome.errors));
139+
}
140+
141+
if outcome.is_done {
142+
OIDC_KEY_MIGRATION_DONE.replace(true);
143+
OIDC_KEY_MIGRATION_TIMER_ID.with_borrow_mut(|id_slot| {
144+
if let Some(timer_id) = id_slot.take() {
145+
ic_cdk_timers::clear_timer(timer_id);
146+
}
147+
});
148+
let processed = OIDC_KEY_MIGRATION_PROCESSED.with_borrow(|c| *c);
149+
ic_cdk::println!(
150+
"OpenID credential key migration COMPLETED ({processed} entries processed)."
151+
);
152+
}
153+
}
154+
155+
/// Start the interval timer driving [`run_oidc_key_migration_batch`]. Safe to
156+
/// call from both `init` (the first batch will immediately see an empty index
157+
/// and mark the migration done) and `post_upgrade`.
158+
fn init_oidc_key_migration_timer() {
159+
let timer_id = ic_cdk_timers::set_timer_interval(
160+
OIDC_KEY_MIGRATION_BATCH_BACKOFF_SECONDS,
161+
run_oidc_key_migration_batch,
162+
);
163+
OIDC_KEY_MIGRATION_TIMER_ID.with_borrow_mut(|id_slot| {
164+
if let Some(old_id) = id_slot.replace(timer_id) {
165+
ic_cdk_timers::clear_timer(old_id);
166+
}
167+
});
168+
}
169+
78170
#[update]
79171
async fn init_salt() {
80172
state::init_salt().await;
@@ -639,6 +731,11 @@ fn initialize(maybe_arg: Option<InternetIdentityInit>) {
639731
if let Some(oidc_configs) = config.oidc_configs {
640732
openid::setup_oidc(oidc_configs);
641733
}
734+
735+
// Kick off the OpenID credential key batch migration. Processes at most
736+
// `OIDC_KEY_MIGRATION_BATCH_SIZE` entries per tick so each batch fits in
737+
// one ingress message; timer self-clears once the migration signals done.
738+
init_oidc_key_migration_timer();
642739
}
643740

644741
fn apply_install_arg(maybe_arg: Option<InternetIdentityInit>) {

0 commit comments

Comments
 (0)