Skip to content

feat(be): OpenIdCredentialKey with data migration — add aud for SSO security#3784

Merged
aterga merged 4 commits intodfinity:mainfrom
timothyaterton:credential-key-aud
Apr 20, 2026
Merged

feat(be): OpenIdCredentialKey with data migration — add aud for SSO security#3784
aterga merged 4 commits intodfinity:mainfrom
timothyaterton:credential-key-aud

Conversation

@timothyaterton
Copy link
Copy Markdown
Contributor

@timothyaterton timothyaterton commented Apr 16, 2026

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

  • 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 | Next PR >

github-merge-queue bot pushed a commit that referenced this pull request Apr 20, 2026
…#3778)

## Summary

- Introduces `DiscoverableOidcConfig` type and `oidc_configs` init field
(mutually exclusive with existing `openid_configs`) that relies on OIDC
discovery (`.well-known/openid-configuration`) instead of requiring all
provider details in the static config
- Adds `DiscoverableProvider` that periodically fetches discovery
metadata to obtain `issuer` and `jwks_uri` for JWT verification
- Adds `discovered_oidc_configs` query endpoint returning `OidcConfig`
with resolved provider state
- Validates that discovered `issuer` domain matches the `discovery_url`
domain (prevents impersonation)
- When both `openid_configs` and `oidc_configs` are provided, falls back
to `openid_configs` as the proven path

## Test plan

- [x] 7 new integration tests in `config/oidc_configs.rs` (init,
upgrade, retain, XOR, query)
- [x] Existing `openid_configs` tests pass unchanged (backward compat)
- [ ] Manual E2E with deployed canister using `oidc_configs`

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---
[Next PR >](#3784)

---------

Co-authored-by: Arshavir Ter-Gabrielyan <arshavir.ter.gabrielyan@dfinity.org>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: timothyaterton <timothyaterton@users.noreply.github.com>
@aterga aterga force-pushed the credential-key-aud branch from 48afa23 to e278b05 Compare April 20, 2026 14:26
Change OpenIdCredentialKey from (iss, sub) to (iss, sub, aud) to
prevent impersonation when different OIDC clients at the same provider
share a user sub claim. This is a security prerequisite for SSO support.

Changes:
- Update OpenIdCredentialKey type alias to include Aud in both the
  interface crate and the openid module
- Rewrite StorableOpenIdCredentialKey with manual CBOR encode/decode:
  new entries use map format {0:iss, 1:sub, 2:aud}, decoder also
  handles legacy array format [iss, sub] for backward compatibility
- Add post_upgrade migration that drains the credential key index,
  resolves aud from each anchor's StorableOpenIdCredential, and
  re-inserts with the complete (iss, sub, aud) key
- Update all key construction sites: OpenIdCredential::key(),
  StorableOpenIdCredential::key(), calculate_delegation_seed(),
  and test helpers
- Update Candid interface, generated JS/TS declarations, and
  frontend credential removal call to include aud
- Add unit tests for new CBOR encoding, legacy decoding, and
  round-trip serialization

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@aterga aterga force-pushed the credential-key-aud branch from e278b05 to dcda156 Compare April 20, 2026 14:43
@aterga aterga marked this pull request as ready for review April 20, 2026 15:44
@aterga aterga requested a review from a team as a code owner April 20, 2026 15:44
@aterga aterga requested review from Copilot and sea-snake April 20, 2026 15:44
Comment thread src/internet_identity/src/openid.rs
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR strengthens OpenID credential indexing by including aud (client_id) in OpenIdCredentialKey, preventing key collisions across multiple OIDC clients at the same issuer—an important prerequisite for safely enabling SSO/discoverable client configurations.

Changes:

  • Expand OpenIdCredentialKey from (iss, sub) to (iss, sub, aud) across the backend, interface crate, Candid, tests, and frontend call sites.
  • Update stable-storage encoding for StorableOpenIdCredentialKey (new CBOR map format) with legacy decoding support and add a post-upgrade migration to re-key existing index entries.
  • Regenerate Candid JS/TS bindings and adjust frontend removal flow to pass aud.

Reviewed changes

Copilot reviewed 12 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/internet_identity_interface/src/internet_identity/types/openid.rs Extends public interface type alias to include aud.
src/internet_identity/tests/integration/openid.rs Updates integration test helper key construction to include aud.
src/internet_identity/src/storage/storable/openid_credential_key.rs Implements new CBOR map encoding + legacy array decoding and adds unit tests.
src/internet_identity/src/storage/storable/openid_credential.rs Updates stored credential key derivation to include aud.
src/internet_identity/src/storage.rs Adds post_upgrade migration to re-key OpenID credential index entries with aud.
src/internet_identity/src/openid.rs Updates core key type and key construction; keeps delegation seed behavior unchanged.
src/internet_identity/src/main.rs Runs OpenID credential key migration during post_upgrade.
src/internet_identity/src/attributes.rs Adjusts tests/maps keyed by OpenID credential key to use 3-tuple.
src/internet_identity/src/anchor_management.rs Updates key destructuring to match 3-tuple.
src/internet_identity/internet_identity.did Updates Candid OpenIdCredentialKey to include Aud.
src/frontend/src/routes/(new-styling)/manage/(authenticated)/(access-and-recovery)/access/utils.ts Updates OpenID access-method key generation to include aud.
src/frontend/src/routes/(new-styling)/manage/(authenticated)/(access-and-recovery)/access/+page.svelte Updates credential removal call to pass aud.
src/frontend/src/lib/generated/internet_identity_types.d.ts Regenerates TS types to reflect 3-tuple key.
src/frontend/src/lib/generated/internet_identity_idl.js Regenerates IDL to reflect 3-tuple key.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/internet_identity/src/storage/storable/openid_credential_key.rs Outdated
Comment thread src/internet_identity/src/storage/storable/openid_credential_key.rs Outdated
aterga and others added 2 commits April 20, 2026 18:32
- Delegation seed now takes `aud` directly from the key tuple instead of a
  redundant `client_id` parameter. All prior callers passed `self.aud` as
  `client_id`, so seed bytes are unchanged and existing Principals are
  preserved — but the dependency of the seed on `aud` is now explicit in the
  code and documented as the security property that makes SSO safe.
- StorableOpenIdCredentialKey decoder no longer matches `MapIndef`: we only
  ever emit definite-length maps, and `d.map()?.unwrap_or(0)` would silently
  produce a bogus empty map on indefinite input. Now returns a clear error.
- Frontend `toKey` for OpenID access methods uses `JSON.stringify([iss, sub,
  aud])` instead of raw concatenation so component boundaries can't collide.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the manual `Encode`/`Decode` impls with `#[derive(Encode, Decode)]`
and `#[cbor(map)]` on the struct. Backward compatibility for the legacy
array form is handled by a separate `LegacyStorableOpenIdCredentialKey`
decoded in `from_bytes` after peeking at the top-level CBOR type — this
keeps the derive idiomatic and avoids hand-written map parsing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@aterga aterga changed the title feat: add aud to OpenIdCredentialKey for SSO security feat(be): OpenIdCredentialKey with data migration — add aud for SSO security Apr 20, 2026
Running the full `(iss, sub) → (iss, sub, aud)` migration synchronously
from `post_upgrade` would blow the ingress instruction limit on the
production II canister (index size scales with OpenID credentials).
Switch to the 2 000-entry-per-tick convention used by the prior anchor
migrations (dfinity#3713):

- `Storage::migrate_openid_credential_keys_batch(batch_size)` processes
  at most `batch_size` legacy entries per call via an `iter().filter()
  .take()` scan, then removes and re-inserts each in the new CBOR map
  format. Signals completion when a scan finds fewer than `batch_size`
  legacy entries — works regardless of how legacy and new-format
  entries interleave in the stable BTreeMap's byte order.
- Orphan index entries (no backing anchor/credential) are dropped with a
  logged warning instead of being re-inserted; re-inserting with empty
  `aud` would keep `is_legacy()` true forever and block progress.
- `post_upgrade` no longer runs the migration inline. Instead
  `initialize()` starts a 1-second interval timer that calls the batch
  function until completion, then clears itself.
- New hidden endpoints for monitoring: `oidc_key_migration_status` (query,
  returns `(processed_count, is_done)`) and `list_oidc_key_migration_errors`
  (update).
- Unit tests cover: empty index, single-entry upgrade, multi-batch
  split, orphan drop, idempotency, and pre-existing new-format entries.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@aterga aterga enabled auto-merge April 20, 2026 17:22
@aterga aterga added this pull request to the merge queue Apr 20, 2026
Merged via the queue into dfinity:main with commit e68167f Apr 20, 2026
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants