Skip to content

Commit de48668

Browse files
fix: Ensure Microsoft attribute scope is not instantiated with a tenant ID during certification (#3649)
# Motivation Microsoft's OpenID issuer ID contains a `{tid}` placeholder that can have multiple instantiations. Since OpenID attributes scopes are literal OpenID issuer IDs, this whole is also expected to be part of the scope. But the II backend interpolated `{tid}` with the actual tenant ID from the user's credentials. Thus, matching requested OpenID attributed over OpenID attributes available for a given user should be done using the proper (non-interpolated) OpenID issuer ID. # Changes * Use non-interpolated OpenID issuer ID while matching OpenID attribute scopes. # Tests * Regression tests --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
1 parent ed29198 commit de48668

File tree

4 files changed

+309
-7
lines changed

4 files changed

+309
-7
lines changed

src/internet_identity/src/attributes.rs

Lines changed: 122 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,18 @@ impl Anchor {
4242
self.openid_credentials
4343
.iter()
4444
.flat_map(|openid_credential| {
45-
let scope = Some(AttributeScope::OpenId {
46-
issuer: openid_credential.iss.clone(),
47-
});
45+
// It is important here to rely on the issuer ID as opposed to `openid_credential.iss`
46+
// becasue, e.g., for Microsoft accounts, the issuer has a tenant ID parameter
47+
// which should *not* be instantiated for a general Microsoft attribute scope.
48+
let Some(issuer) = openid_credential.config_issuer() else {
49+
ic_cdk::println!(
50+
"No matching OpenID provider for issuer: {}, skipping credential",
51+
openid_credential.iss
52+
);
53+
return BTreeSet::new();
54+
};
55+
56+
let scope = Some(AttributeScope::OpenId { issuer });
4857

4958
let Some(attributes_to_fetch) = attributes.remove(&scope) else {
5059
return BTreeSet::new();
@@ -1220,4 +1229,114 @@ mod tests {
12201229
);
12211230
}
12221231
}
1232+
1233+
/// Regression test for the `get_attributes` fix.
1234+
///
1235+
/// The bug: `get_attributes` built its scope key from `openid_credential.iss`
1236+
/// (the *resolved* issuer, e.g. `…/9188040d-…/v2.0` for Microsoft),
1237+
/// while callers pass the *template* issuer (`…/{tid}/v2.0`) as the scope
1238+
/// key. This caused `attributes.remove(&scope)` to always miss for
1239+
/// Microsoft credentials.
1240+
///
1241+
/// We test via `prepare_openid_attributes` because:
1242+
/// 1. It uses the identical `config_issuer()` lookup that the fix applied
1243+
/// to `get_attributes`.
1244+
/// 2. `get_attributes` calls into canister runtime (`state::assets_and_signatures`,
1245+
/// `ic_cdk::println!`) and cannot be exercised in a unit test.
1246+
/// 3. A test that passes for `prepare_openid_attributes` with the
1247+
/// template-scope key proves that the same key will work in
1248+
/// `get_attributes` after the fix.
1249+
mod get_attributes_issuer_regression_test {
1250+
use super::*;
1251+
use internet_identity_interface::internet_identity::types::{
1252+
OpenIdConfig, OpenIdEmailVerificationScheme,
1253+
};
1254+
1255+
const MICROSOFT_ISSUER_TEMPLATE: &str = "https://login.microsoftonline.com/{tid}/v2.0";
1256+
const MICROSOFT_PERSONAL_TID: &str = "9188040d-6c67-4c5b-b112-36a304b66dad";
1257+
const MICROSOFT_RESOLVED_ISSUER: &str =
1258+
"https://login.microsoftonline.com/9188040d-6c67-4c5b-b112-36a304b66dad/v2.0";
1259+
const ANCHOR_NUMBER: u64 = 4242;
1260+
1261+
fn setup_microsoft_provider() {
1262+
crate::openid::setup(vec![OpenIdConfig {
1263+
name: "Microsoft".to_string(),
1264+
logo: String::new(),
1265+
issuer: MICROSOFT_ISSUER_TEMPLATE.to_string(),
1266+
client_id: "test-client-id".to_string(),
1267+
jwks_uri: String::new(),
1268+
auth_uri: String::new(),
1269+
auth_scope: vec![],
1270+
fedcm_uri: None,
1271+
email_verification: Some(OpenIdEmailVerificationScheme::Microsoft),
1272+
}]);
1273+
}
1274+
1275+
/// Callers of `get_attributes` / `prepare_attributes` always use the
1276+
/// *template* issuer (`{tid}` placeholder) as the scope key.
1277+
fn microsoft_template_scope() -> Option<AttributeScope> {
1278+
Some(AttributeScope::OpenId {
1279+
issuer: MICROSOFT_ISSUER_TEMPLATE.to_string(),
1280+
})
1281+
}
1282+
1283+
/// A Microsoft credential whose `.iss` is the *resolved* issuer
1284+
/// (real tenant ID), as it would be after JWT verification.
1285+
fn microsoft_credential_with_email(email: &str) -> OpenIdCredential {
1286+
OpenIdCredential {
1287+
iss: MICROSOFT_RESOLVED_ISSUER.to_string(),
1288+
sub: "ms-user-456".to_string(),
1289+
aud: "test-client-id".to_string(),
1290+
last_usage_timestamp: None,
1291+
metadata: HashMap::from([
1292+
(
1293+
"email".to_string(),
1294+
MetadataEntryV2::String(email.to_string()),
1295+
),
1296+
(
1297+
"tid".to_string(),
1298+
MetadataEntryV2::String(MICROSOFT_PERSONAL_TID.to_string()),
1299+
),
1300+
]),
1301+
}
1302+
}
1303+
1304+
/// The scope key uses the *template* issuer while the credential's
1305+
/// `.iss` contains the *resolved* issuer. Before the fix,
1306+
/// `get_attributes` used `.iss` directly and the lookup would miss.
1307+
/// `prepare_openid_attributes` (and now `get_attributes`) uses
1308+
/// `config_issuer()` which returns the template, so the key matches.
1309+
#[test]
1310+
fn test_template_scope_resolves_microsoft_credential() {
1311+
setup_microsoft_provider();
1312+
1313+
let mut anchor = Anchor::new(ANCHOR_NUMBER, 0);
1314+
anchor.openid_credentials = vec![microsoft_credential_with_email("user@outlook.com")];
1315+
1316+
// Key the request by the *template* scope — exactly as callers do.
1317+
let mut requested = BTreeMap::from([(
1318+
microsoft_template_scope(),
1319+
BTreeSet::from([AttributeName::Email]),
1320+
)]);
1321+
1322+
let result = anchor.prepare_openid_attributes(&mut requested);
1323+
1324+
// The scope must have been consumed (matched).
1325+
assert!(
1326+
requested.is_empty(),
1327+
"template scope should match the credential via config_issuer()"
1328+
);
1329+
1330+
// Exactly one credential key should be present with one attribute.
1331+
pretty_assert_eq!(result.len(), 1, "one credential should match");
1332+
let attrs = result
1333+
.get(&(
1334+
MICROSOFT_RESOLVED_ISSUER.to_string(),
1335+
"ms-user-456".to_string(),
1336+
))
1337+
.expect("credential key should be present");
1338+
pretty_assert_eq!(attrs.len(), 1, "email attribute should be returned");
1339+
pretty_assert_eq!(String::from_utf8_lossy(&attrs[0].value), "user@outlook.com",);
1340+
}
1341+
}
12231342
}

src/internet_identity/tests/integration/attributes.rs

Lines changed: 182 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,3 +243,185 @@ fn should_get_certified_attributes() {
243243
get_response.expires_at_timestamp_ns,
244244
);
245245
}
246+
247+
/// Regression test: Microsoft credentials use a template issuer with `{tid}` placeholder.
248+
/// `get_attributes` must use the template issuer (from the OpenID config) as the scope key,
249+
/// NOT the resolved issuer (from the JWT `iss` claim with the actual tenant ID).
250+
/// Without the fix, `get_attributes` would look up signatures under the resolved issuer scope
251+
/// but `prepare_attributes` stores them under the template issuer scope, resulting in
252+
/// no certified attributes being returned.
253+
#[test]
254+
fn should_get_certified_attributes_microsoft() {
255+
let env = env();
256+
#[allow(unused_variables)]
257+
let (jwt, salt, claims, test_time, test_principal, test_authn_method) =
258+
crate::openid::one_openid_microsoft_test_data();
259+
260+
let mut init_args = arg_with_wasm_hash(ARCHIVE_WASM.clone()).unwrap();
261+
init_args.openid_configs = Some(vec![OpenIdConfig {
262+
name: "Microsoft".into(),
263+
logo: "logo".into(),
264+
issuer: "https://login.microsoftonline.com/{tid}/v2.0".into(),
265+
client_id: "d948c073-eebd-4ab8-861d-055f7ab49e17".into(),
266+
jwks_uri: "https://login.microsoftonline.com/common/discovery/v2.0/keys".into(),
267+
auth_uri: "https://login.microsoftonline.com/common/oauth2/v2.0/authorize".into(),
268+
auth_scope: vec!["openid".into(), "profile".into(), "email".into()],
269+
fedcm_uri: Some("".into()),
270+
email_verification: None,
271+
}]);
272+
273+
let ii_backend_canister_id = install_ii_canister_with_arg_and_cycles(
274+
&env,
275+
II_WASM.clone(),
276+
Some(init_args),
277+
1_000_000_000_000_000, // 1P cycles for HTTP outcalls
278+
);
279+
// Mock certs response so openid_credential_add can verify the JWT
280+
crate::openid::mock_microsoft_certs_response(&env);
281+
282+
deploy_archive_via_ii(&env, ii_backend_canister_id);
283+
284+
// Create an identity with the required authn method
285+
let identity_number =
286+
crate::v2_api::authn_method_test_helpers::create_identity_with_authn_method(
287+
&env,
288+
ii_backend_canister_id,
289+
&test_authn_method,
290+
);
291+
292+
// Sync time to the JWT iat
293+
let time_to_advance = Duration::from_millis(test_time) - Duration::from_nanos(time(&env));
294+
env.advance_time(time_to_advance);
295+
296+
// Add OpenID credential
297+
api::openid_credential_add(
298+
&env,
299+
ii_backend_canister_id,
300+
test_principal,
301+
identity_number,
302+
&jwt,
303+
&salt,
304+
)
305+
.expect("failed to add openid credential")
306+
.expect("openid_credential_add error");
307+
308+
let origin = "https://some-dapp.com";
309+
310+
// The attribute key MUST use the template issuer with {tid}, not the resolved tenant ID.
311+
let microsoft_template_issuer = "https://login.microsoftonline.com/{tid}/v2.0";
312+
313+
// 1. Prepare attributes
314+
315+
env.advance_time(Duration::from_secs(15));
316+
317+
let prepare_request = PrepareAttributeRequest {
318+
identity_number,
319+
origin: origin.to_string(),
320+
account_number: None,
321+
attribute_keys: vec![format!("openid:{microsoft_template_issuer}:name")],
322+
};
323+
324+
let prepare_response = api::prepare_attributes(
325+
&env,
326+
ii_backend_canister_id,
327+
test_principal,
328+
prepare_request,
329+
)
330+
.expect("failed to call prepare_attributes")
331+
.expect("prepare_attributes error");
332+
333+
assert_eq!(prepare_response.attributes.len(), 1);
334+
335+
// 2. Get attributes
336+
env.advance_time(Duration::from_secs(5));
337+
338+
let get_request = GetAttributesRequest {
339+
identity_number,
340+
origin: origin.to_string(),
341+
account_number: None,
342+
issued_at_timestamp_ns: prepare_response.issued_at_timestamp_ns,
343+
attributes: prepare_response.attributes.clone(),
344+
};
345+
346+
let get_response =
347+
api::get_attributes(&env, ii_backend_canister_id, test_principal, get_request)
348+
.expect("failed to call get_attributes")
349+
.expect("get_attributes error");
350+
351+
// Check that we actually got a certified attribute back (the regression would return 0)
352+
assert_eq!(
353+
get_response.certified_attributes.len(),
354+
1,
355+
"get_attributes should return 1 certified attribute for Microsoft name; \
356+
if 0, get_attributes is likely using the resolved issuer instead of the template issuer"
357+
);
358+
359+
// Verify the CBOR signature prefix (smoke test)
360+
assert!(get_response.certified_attributes[0]
361+
.signature
362+
.starts_with(&[217, 217, 247, 162, 107, 99, 101, 114]));
363+
364+
// Redact signatures for comparison
365+
let mut redacted_response = get_response.clone();
366+
redacted_response
367+
.certified_attributes
368+
.iter_mut()
369+
.for_each(|attr| attr.signature = vec![]);
370+
371+
assert_eq!(
372+
redacted_response,
373+
CertifiedAttributes {
374+
certified_attributes: vec![CertifiedAttribute {
375+
key: format!("openid:{microsoft_template_issuer}:name"),
376+
value: "Llorenç Muntaner Perello".as_bytes().to_vec(),
377+
signature: vec![], // redacted
378+
}],
379+
expires_at_timestamp_ns: prepare_response.issued_at_timestamp_ns
380+
+ Duration::from_secs(30 * 60).as_nanos() as u64,
381+
}
382+
);
383+
384+
// Verify the signatures; this relies on delegation verification for the same (origin, user).
385+
386+
let session_public_key = ByteBuf::from("session public key");
387+
388+
env.advance_time(Duration::from_secs(35));
389+
390+
let (canister_sig_key, expiration) = api::prepare_delegation(
391+
&env,
392+
ii_backend_canister_id,
393+
test_principal,
394+
identity_number,
395+
origin,
396+
&session_public_key,
397+
None,
398+
)
399+
.unwrap();
400+
401+
env.advance_time(Duration::from_secs(5));
402+
403+
let signed_delegation = match api::get_delegation(
404+
&env,
405+
ii_backend_canister_id,
406+
test_principal,
407+
identity_number,
408+
origin,
409+
&session_public_key,
410+
expiration,
411+
)
412+
.unwrap()
413+
{
414+
GetDelegationResponse::SignedDelegation(delegation) => delegation,
415+
GetDelegationResponse::NoSuchDelegation => panic!("failed to get delegation"),
416+
};
417+
418+
verify_delegation_and_attribute_signatures(
419+
&env,
420+
session_public_key,
421+
canister_sig_key,
422+
ii_backend_canister_id,
423+
signed_delegation,
424+
get_response.certified_attributes,
425+
get_response.expires_at_timestamp_ns,
426+
);
427+
}

src/internet_identity/tests/integration/openid.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -619,7 +619,7 @@ pub fn mock_google_certs_response(env: &PocketIc) {
619619
mock_certs_response(env, url, mock_certs);
620620
}
621621

622-
fn mock_microsoft_certs_response(env: &PocketIc) {
622+
pub fn mock_microsoft_certs_response(env: &PocketIc) {
623623
// This is the URL that the canister will fetch the Microsoft certificates
624624
let url = "https://login.microsoftonline.com/common/discovery/v2.0/keys";
625625
// These are the certificates at the time of the related Microsoft JWT mocked data was created.
@@ -785,7 +785,8 @@ fn second_openid_google_test_data() -> (String, [u8; 32], Claims, u64, Principal
785785
)
786786
}
787787

788-
fn one_openid_microsoft_test_data() -> (String, [u8; 32], Claims, u64, Principal, AuthnMethodData) {
788+
pub fn one_openid_microsoft_test_data(
789+
) -> (String, [u8; 32], Claims, u64, Principal, AuthnMethodData) {
789790
let jwt = "eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6IkpZaEFjVFBNWl9MWDZEQmxPV1E3SG4wTmVYRSJ9.eyJhdWQiOiJkOTQ4YzA3My1lZWJkLTRhYjgtODYxZC0wNTVmN2FiNDllMTciLCJpc3MiOiJodHRwczovL2xvZ2luLm1pY3Jvc29mdG9ubGluZS5jb20vNGE0MzVjNWUtNjQ1MS00YzFhLWE4MWYtYWI5NjY2YjZkZThmL3YyLjAiLCJpYXQiOjE3NTY4MDgzMjQsIm5iZiI6MTc1NjgwODMyNCwiZXhwIjoxNzU2ODEyMjI0LCJhaW8iOiJBVlFBcS84WkFBQUExQjhrYVdEVWp6V0xnSUxVT2hIQ1pWVndhbk1wNGVnVzdURzZwTytnVSsyYzdKRVRJckV5VHlySkxQQ0h1VkZINkUrbzRlMzhCQjZ6dmlWSU9kTzkxNVRHVDhEaUR3bkhCazYxTSt2bTdJaz0iLCJjX2hhc2giOiJGUzJsWllUYTIwcWozZVl1enczUXBnIiwibmFtZSI6Ikxsb3JlbsOnIE11bnRhbmVyIFBlcmVsbG8iLCJub25jZSI6ImN1UmM4VlNEN1ZkQU9ISmpsX1UxbkNWdlpvamQtMGJoUE81X0lGbTc0N2MiLCJvaWQiOiIxYjI2NDVmNy04YjdmLTQyMTAtYjQxYy01MDM1MmQ1OWYyZTgiLCJwcmVmZXJyZWRfdXNlcm5hbWUiOiJsbG9yZW5jLm11bnRhbmVyQGRmaW5pdHkub3JnIiwicmgiOiIxLkFTNEFYbHhEU2xGa0dreW9INnVXWnJiZWozUEFTTm05N3JoS2hoMEZYM3EwbmhlNUFPd3VBQS4iLCJzaWQiOiIwMDdkZWE3OS0yNjY5LWZjNTItMzQwOS01Y2NjZDkxOTAzMjEiLCJzdWIiOiJydkF0eGluNk1TblRsN1RnUlg4RlhYQ0tQbEVlTklmUHI0bHdQT1lfd293IiwidGlkIjoiNGE0MzVjNWUtNjQ1MS00YzFhLWE4MWYtYWI5NjY2YjZkZThmIiwidXRpIjoiU3pLR0k3cG44MC1ZdnRmMmxuZ0RBUSIsInZlciI6IjIuMCJ9.kS8C8IlRoMaYoFyru-D06WzdeS8mHA3LupXyrOqXwwb4AIMMUDETlJEznAQ6iZxK4iAhAPAqAnC9TS_j0sacRCTBA3Rks-tkuwV2sA3XdwDsoFOnJdBs-N5GEXJNv45TzQ0jQANnXBJwwgH9hS-ledFZiutvzaTfDGpAymxx58qj7VDG5fTMxpiPMNCr42sNidw7B8ifUJgcfcxt_8wsTN_mui4Q6wtWRQvPnbesyTvRaOg2S6LMG3m8RBNYtHvXlICwD1kaKS5wUiYcrN3gg6wqOXCI3w57S5yfnGNo1tF4sWCfR0ZkfyHfVzdXK_6BwCty7rt4udp-NFsCAVXNRQ";
790791
let salt: [u8; 32] = [
791792
196, 116, 153, 227, 8, 104, 231, 67, 202, 28, 156, 132, 101, 84, 170, 111, 86, 233, 29, 54,

src/internet_identity_interface/src/internet_identity/types/attributes.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,7 @@ impl std::fmt::Display for AttributeKey {
212212
}
213213
}
214214

215-
#[derive(CandidType, Deserialize)]
215+
#[derive(CandidType, Debug, Deserialize)]
216216
pub struct PrepareAttributeRequest {
217217
pub identity_number: AnchorNumber,
218218
pub origin: FrontendHostname,
@@ -302,7 +302,7 @@ pub enum PrepareAttributeError {
302302
GetAccountError(GetAccountError),
303303
}
304304

305-
#[derive(CandidType, Deserialize)]
305+
#[derive(CandidType, Debug, Deserialize)]
306306
pub struct GetAttributesRequest {
307307
pub identity_number: AnchorNumber,
308308
pub origin: FrontendHostname,

0 commit comments

Comments
 (0)