Broker Secret Key Rotation, Fixes AB#2956404#3048
Conversation
…46 (#3023) Foundational immutable data class for tracking symmetric encryption key versions, enabling SDL-compliant key rotation in the Android Broker. Each key instance captures its version ID, creation timestamp, algorithm, key size, and deprecation status (decrypt-only flag). ## Changes ### `common4j/.../crypto/KeyMetadata.java` - Immutable `final` class using Lombok `@Getter` and `@Builder` annotations to eliminate boilerplate - Fields: `versionId`, `createdAtMillis`, `algorithm` (default `AES/CBC/PKCS5Padding`), `keySize` (default 256), `deprecated` - Fail-fast validation in constructor: blank `versionId` throws `IllegalStateException`; non-positive `keySize` throws `IllegalArgumentException` - `toJson()` / `fromJson(String)` via `org.json` for persistence by the KeyVersionRegistry - `fromJson()` uses `opt*` APIs for optional fields (`algorithm`, `keySize`, `deprecated`) so deserialization is backward-compatible with older persisted JSON ```java KeyMetadata key = KeyMetadata.builder() .versionId("K001") .createdAtMillis(System.currentTimeMillis()) .deprecated(false) .build(); String json = key.toJson(); KeyMetadata restored = KeyMetadata.fromJson(json); ``` ### `common4j/.../crypto/KeyMetadataTest.java` Unit tests covering builder defaults, field validation, JSON round-trip, deprecated flag semantics, backward-compatible deserialization of JSON missing optional fields, and malformed JSON error handling. <!-- START COPILOT CODING AGENT SUFFIX --> <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > ## [Symmetric Key Rotation] Add KeyMetadata data class > > Fixes [AB#3546146](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3546146) > > ### Objective > > Add a KeyMetadata data class to common4j that stores version information for symmetric encryption keys. This is the foundational data structure for the symmetric key rotation feature. > > ### Context > > The Android Broker needs to support multiple symmetric key versions to enable SDL-compliant key rotation. Each key needs metadata including version ID, creation timestamp, algorithm, and deprecation status. This data class will be used by the KeyVersionRegistry (in common) and the encryption managers (in broker). > > ### Technical Requirements > > - Create new file common4j/src/main/com/microsoft/identity/common/java/crypto/KeyMetadata.java > - Implement as an immutable data class with the following fields: > - versionId (String): Key identifier like K001, K002 > - createdAtMillis (long): Unix timestamp of key creation > - algorithm (String): Encryption algorithm, default AES/CBC/PKCS5Padding > - keySize (int): Key size in bits, default 256 > - isDeprecated (boolean): If true, key can only decrypt (not encrypt) > - Add JSON serialization/deserialization methods (toJson(), fromJson(String)) using org.json > - Follow existing patterns in common4j for data classes > - Add appropriate JavaDoc comments > - Add copyright header to the new file > > ### Scope > > In scope: KeyMetadata data class with all fields, JSON serialization/deserialization, Builder pattern for construction. > Out of scope: Persistence logic (handled by KeyVersionRegistry), Key generation logic (handled elsewhere). > > ### Files to Create > > - common4j/src/main/com/microsoft/identity/common/java/crypto/KeyMetadata.java > > ### Acceptance Criteria > > - KeyMetadata class exists with all required fields > - toJson() produces valid JSON representation > - fromJson(String) reconstructs the object correctly > - Class is immutable (final fields, no setters) > - JavaDoc comments document each field > - Compile check passes: ./gradlew :common4j:compileJava > > Follow .github/copilot-instructions.md strictly. > </details> <!-- START COPILOT CODING AGENT TIPS --> --- 🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. [Learn more about Advanced Security.](https://gh.io/cca-advanced-security) --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: fadidurah <88730756+fadidurah@users.noreply.github.com>
…agement, Fixes AB#3546148 (#3031) Introduces `KeyVersionRegistry` to manage multiple versioned AES-256 symmetric encryption keys — tracking metadata, generating new keys, marking keys deprecated, and pruning expired ones. Builds on the existing `KeyMetadata` data class and follows `AndroidWrappedKeyProvider` patterns for RSA-wrapping via AndroidKeyStore. ## New: `KeyVersionRegistry` - **Storage**: Metadata in `SharedPreferences` (`brokerks_metadata`); wrapped key blobs in files (`brokerks_K001`, `brokerks_K002`, …) - **Wrapping**: RSA/ECB/PKCS1Padding via AndroidKeyStore alias `brokerks_wrapping_key`; API-level-aware key pair generation (M+ / legacy) - **Version IDs**: Auto-incremented (`K001`, `K002`, …) - **Constants**: `MAX_KEY_AGE_MILLIS` = 3 years, `GRACE_PERIOD_MILLIS` = 90 days - **Pruning**: Removes any non-active key whose total age exceeds `MAX_KEY_AGE_MILLIS + GRACE_PERIOD_MILLIS`, regardless of whether it has been explicitly deprecated; the active key is never pruned - **Telemetry**: `generateNewKey()` emits a `KeyVersionRegistryGenerateKey` span (new entry in `SpanName`) ## Public API ```java KeyVersionRegistry registry = new KeyVersionRegistry(context); KeyMetadata key = registry.generateNewKey(); // K001 — not active yet registry.setActiveKey(key.getVersionId()); // promote to active KeyMetadata active = registry.getActiveKey(); SecretKey secretKey = registry.loadSecretKey("K001"); // unwrap for decryption registry.deprecateKey("K001"); List<KeyMetadata> deprecated = registry.getDeprecatedKeys(); registry.pruneExpiredKeys(); // remove stale keys ``` All public methods are `synchronized`; thread-safety is instance-scoped (not cross-process). ## Tests Added `KeyVersionRegistryTest` as an instrumented test in `common/src/androidTest`, covering all public API methods across 18 test cases: - **`generateNewKey`**: metadata fields, wrapped key file creation, version ID auto-increment (`K001` → `K002`), no auto-promotion to active - **`setActiveKey` / `getActiveKey`**: initially `null`, promotion, throws `IllegalStateException` for unknown version - **`getKeyByVersion`**: found and not-found cases - **`deprecateKey` / `getDeprecatedKeys`**: marks key deprecated, idempotent, throws for unknown version, list filtering - **`loadSecretKey`**: returns unwrapped AES key, same key material on repeated loads, throws for unknown version - **`pruneExpiredKeys`**: recent keys kept, expired non-active key pruned (including key file deletion), active key never pruned even when old, prunes regardless of deprecation status, boundary threshold check > [!WARNING] > > <!-- START COPILOT CODING AGENT SUFFIX --> <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > ## [Symmetric Key Rotation][Common] Implement KeyVersionRegistry > > Fixes [AB#3546148](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3546148) > > ### Objective > > Implement KeyVersionRegistry in common/common to manage multiple encryption key versions. This registry tracks key metadata, generates new keys, marks keys as deprecated, and prunes expired keys. > > ### Context > > The KeyVersionRegistry is the central component for managing symmetric key lifecycle. It stores key metadata in SharedPreferences (encrypted) and actual wrapped keys in files. Old keys are retained for decryption until pruned (3 years + 90-day grace period after deprecation). This builds on the KeyMetadata data class already merged in the codebase at common4j/src/main/com/microsoft/identity/common/java/crypto/KeyMetadata.java. > > ### Technical Requirements > > - Create common/src/main/java/com/microsoft/identity/common/crypto/KeyVersionRegistry.java > - Constructor takes Context for storage access > - Implement key metadata storage using SharedPreferences with filename brokerks_metadata > - Implement the following public methods: > - KeyMetadata getActiveKey() > - KeyMetadata getKeyByVersion(String versionId) > - List<KeyMetadata> getDeprecatedKeys() > - KeyMetadata generateNewKey() - Creates key, wraps with KeyStore, stores > - void deprecateKey(String versionId) - Sets isDeprecated=true > - void setActiveKey(String versionId) - Updates activeVersion pointer > - SecretKey loadSecretKey(String versionId) - Loads/unwraps key material > - void pruneExpiredKeys() - Remove keys older than MAX_AGE + GRACE_PERIOD > - Key version IDs format: K001, K002, etc. (auto-incrementing) > - Use AndroidKeyStore for RSA wrapping of symmetric keys (follow existing StorageCryptoManager patterns) > - Key file naming: brokerks_K001, brokerks_K002, etc. > - Constants: MAX_KEY_AGE_MILLIS = 3 years, GRACE_PERIOD_MILLIS = 90 days > - Add telemetry span for key generation operations > > ### Storage Format (SharedPreferences) > > {activeVersion: K002, keys: [{versionId: K001, createdAtMillis: 1673827200000, algorithm: AES/CBC/PKCS5Padding, keySize: 256, isDeprecated: true}, {versionId: K002, createdAtMillis: 1740787200000, algorithm: AES/CBC/PKCS5Padding, keySize: 256, isDeprecated: false}]} > > ### Scope > > In scope: KeyVersionRegistry class with all public APIs, key generation using AES256SecretKeyGenerator, key wrapping/unwrapping with AndroidKeyStore, metadata persistence in SharedPreferences, key file management. > Out of scope: Rotation orchestration logic (Broker), feature flag integration (Broker), cross-app coordination (Broker). > > ### Files to Create > > - common/src/main/java/com/microsoft/identity/common/crypto/KeyVersionRegistry.java > > ### Acceptance Criteria > > - getActiveKey() returns current active key metadata > - generateNewKey() creates a new key with auto-incremented version ID > - deprecateKey() marks a key as deprecated > - loadSecretKey() correctly unwraps and returns the SecretKey > - pruneExpiredKeys() removes keys older than 3 years + 90 days past deprecation > - Metadata persists across app restarts > - Compile check passes: ./gradlew :common:compileDebugKotlin > > Follow .github/copilot-instructions.md strictly. > </details> <!-- START COPILOT CODING AGENT TIPS --> --- 📍 Connect Copilot coding agent with [Jira](https://gh.io/cca-jira-docs), [Azure Boards](https://gh.io/cca-azure-boards-docs) or [Linear](https://gh.io/cca-linear-docs) to delegate work to Copilot in one click without leaving your project management tool. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: fadidurah <88730756+fadidurah@users.noreply.github.com>
…ersionRegistry, Fixes AB#3546149 (#3034) Adds ECS-controllable key lifetime thresholds for the symmetric key rotation feature. Since `common` cannot import `BrokerFlight`, the two age threshold flights are added to `CommonFlight` with matching string keys — the established broker-override pattern where `AndroidBrokerFlightsManager` injects itself at runtime, making ECS-backed values transparently available through `CommonFlightsManager`. ### CommonFlight (`common4j`) Two new entries following the `ENABLE_PLAYSTORE_URL_LAUNCH` / `ENABLE_WEBVIEW_MULTIPLE_WINDOWS` broker-override pattern: ```java SYMMETRIC_KEY_MAX_AGE_DAYS("SymmetricKeyMaxAgeDays", 1095), // 3yr SDL max SYMMETRIC_KEY_RECOMMENDED_AGE_DAYS("SymmetricKeyRecommendedAgeDays", 365), // 1yr SDL recommendation ``` String keys match BrokerFlight's keys exactly so ECS overrides flow through correctly. ### KeyVersionRegistry (`common`) `pruneExpiredKeys()` now reads `SYMMETRIC_KEY_MAX_AGE_DAYS` dynamically instead of using the hardcoded `MAX_KEY_AGE_MILLIS` constant: ```java int maxAgeDays = CommonFlightsManager.INSTANCE.getFlightsProvider() .getIntValue(CommonFlight.SYMMETRIC_KEY_MAX_AGE_DAYS); if (maxAgeDays <= 0) { // guard against invalid ECS value maxAgeDays = (int) CommonFlight.SYMMETRIC_KEY_MAX_AGE_DAYS.getDefaultValue(); } final long maxAgeMillis = (long) maxAgeDays * 24 * 60 * 60 * 1000; ``` - `MAX_KEY_AGE_MILLIS` constant is retained (existing tests reference it; its value equals the new flight default). - Grace period (90 days) remains hardcoded. <!-- START COPILOT CODING AGENT SUFFIX --> <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > ## [Symmetric Key Rotation][Common] Add key lifetime flights to CommonFlight and wire into KeyVersionRegistry > > Fixes [AB#3546149](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3546149) > > ### Objective > > Add two key lifetime threshold flights to CommonFlight enum in common4j, and update KeyVersionRegistry (on the fadi/secret-key-rotation branch) to read these flights instead of using hardcoded constants. This enables ECS-controlled key lifetime thresholds for the symmetric key rotation feature. > > ### Context > > The symmetric key rotation feature defines 4 flights in BrokerFlight (broker4j). Two of them the age threshold flights are also needed by KeyVersionRegistry in common, which determines when deprecated keys are ready for pruning. Since common cannot import BrokerFlight, these two flights must also be added to CommonFlight with the SAME string keys (this is the established broker-override pattern, used by EnablePlaystoreUrlLaunch, EnableWebViewMultipleWindows, etc.). > > At runtime in broker, AndroidBrokerFlightsManager injects itself into CommonFlightsManager, so CommonFlight lookups go through the ECS-backed provider which uses BrokerFlight's default values (overriding CommonFlight's defaults). > > ### Technical Requirements > > #### 1. Add to CommonFlight enum (common4j) > > File: common/common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java > > Add these two entries before the semicolon at the end of the enum (after ENABLE_WEBVIEW_MULTIPLE_WINDOWS): > > `java > /** > * Maximum key age in days before rotation is required. > * Default: 1095 (3 years per SDL maximum lifetime requirement). > * When running in broker, BrokerFlight overrides this default via ECS. > */ > SYMMETRIC_KEY_MAX_AGE_DAYS(SymmetricKeyMaxAgeDays, 1095), > > /** > * Recommended key age in days before rotation is triggered. > * Default: 365 (1 year per SDL recommendation). > * When running in broker, BrokerFlight overrides this default via ECS. > */ > SYMMETRIC_KEY_RECOMMENDED_AGE_DAYS(SymmetricKeyRecommendedAgeDays, 365), > ` > > #### 2. Update KeyVersionRegistry to use flights > > File: common/common/src/main/java/com/microsoft/identity/common/crypto/KeyVersionRegistry.kt (on fadi/secret-key-rotation branch, merged via PR #3031) > > In pruneExpiredKeys(), replace the hardcoded MAX_KEY_AGE_MILLIS constant with a runtime flight lookup: > > `kotlin > val maxAgeDays = CommonFlightsManager.getFlightsProvider() > .getIntValue(CommonFlight.SYMMETRIC_KEY_MAX_AGE_DAYS) > val maxAgeMillis = maxAgeDays.toLong() * 24 * 60 * 60 * 1000 > ` > > Use this maxAgeMillis + the existing grace period constant when determining which deprecated keys to prune. > > Remove or update the companion object constant MAX_KEY_AGE_MILLIS to derive from the flight default, or remove it entirely and always read the flight value. > > ### Reference Files > > - CommonFlight pattern: common/common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java (see ENABLE_WEBVIEW_MULTIPLE_WINDOWS as example of broker-override pattern) > - CommonFlightsManager: common/common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlightsManager.kt > - IFlightsProvider.getIntValue(): common/common4j/src/main/com/microsoft/identity/common/java/flighting/IFlightsProvider.java > - KeyVersionRegistry: common/common/src/main/java/com/microsoft/identity/common/crypto/KeyVersionRegistry.kt (on fadi/secret-key-rotation branch) > > ### Acceptance Criteria > > - Two new CommonFlight entries added with correct string keys matching BrokerFlight > - KeyVersionRegistry.pruneExpiredKeys() reads SYMMETRIC_KEY_MAX_AGE_DAYS via CommonFlightsManager instead of hardcoded constant > - Grace period (90 days) can remain hardcoded > - JavaDoc on all new enum entries > - Copyright header preserved > - Compile check passes: ./gradlew :common4j:compileJava; ./gradlew :common:compileDebugKotlin > > Follow .github/copilot-instructions.md strictly. > </details> <!-- START COPILOT CODING AGENT TIPS --> --- 💬 Send tasks to Copilot coding agent from [Slack](https://gh.io/cca-slack-docs) and [Teams](https://gh.io/cca-teams-docs) to turn conversations into code. Copilot posts an update in your thread when it's finished. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: fadidurah <88730756+fadidurah@users.noreply.github.com>
…nFlight entry, Fixes AB#3546155 (#3033) Adds comprehensive JVM-based unit tests for `KeyVersionRegistry` (introduced in #3031) using Robolectric, covering the full symmetric key lifecycle. Also removes the mistakenly added `SYMMETRIC_KEY_RECOMMENDED_AGE_DAYS` flight from `CommonFlight` (it belongs solely in `BrokerFlight`, where `RotationOrchestrator` reads it). ## Test coverage - **`generateNewKey`** — metadata fields (`versionId` K001/K002 sequencing, `algorithm`, `keySize`, `isDeprecated=false`), wrapped key file creation, no auto-promotion to active - **`getActiveKey` / `setActiveKey`** — null on empty registry, promotion, throws on unknown version - **`getKeyByVersion`** — hit and miss cases - **`deprecateKey` / `getDeprecatedKeys`** — marks deprecated, does not affect sibling keys, idempotent, throws on unknown version, filtered list - **`loadSecretKey`** — returns AES `SecretKey`, deterministic re-load (same encoded bytes), throws on unknown version - **`pruneExpiredKeys`** — age-based pruning, active key is never pruned, key file deleted on prune, boundary condition (1 s under threshold kept), expired non-deprecated keys are pruned - **Multi-key lifecycle** — generate → deprecate → generate new → assert both accessible with correct active/deprecated state ## Setup - `@RunWith(RobolectricTestRunner::class)` + `@Config(sdk = [Build.VERSION_CODES.P])`, consistent with existing module tests - `overrideKeyCreationTimestamp()` helper manipulates SharedPreferences directly to simulate aged keys without real-time delays - `cleanUp()` runs before/after each test: clears SharedPreferences, removes the AndroidKeyStore wrapping key pair, deletes wrapped key files K001–K010 - `AndroidKeyStoreUtil` static methods are stubbed via `mockkStatic` to avoid Robolectric `KeyStoreException` when generating RSA wrapping key pairs ## CommonFlight cleanup Removed `SYMMETRIC_KEY_RECOMMENDED_AGE_DAYS` from `CommonFlight`. This entry was added by mistake in PR #3034 — it is only consumed by `RotationOrchestrator` in broker (via `BrokerFlight`) and has no callers in the common library. `SYMMETRIC_KEY_MAX_AGE_DAYS` remains as the sole symmetric-key flight in `CommonFlight`, used by `KeyVersionRegistry.pruneExpiredKeys()`. <!-- START COPILOT CODING AGENT SUFFIX --> <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > ## [Symmetric Key Rotation][Common] Add KeyVersionRegistry unit tests > > Fixes [AB#3546155](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3546155) > > ### Objective > > Write comprehensive unit tests for KeyVersionRegistry (merged in PR #3031 on the fadi/secret-key-rotation branch). Cover key generation, deprecation, metadata persistence, pruning, and edge cases. > > ### Context > > KeyVersionRegistry was implemented in PR #3031 and merged into the fadi/secret-key-rotation branch of AzureAD/microsoft-authentication-library-common-for-android. It manages versioned encryption keys with metadata stored in SharedPreferences. Tests should validate the registry's core operations. > > ### Technical Requirements > > 1. Create test file: common/common/src/test/java/com/microsoft/identity/common/crypto/KeyVersionRegistryTest.kt > 2. Use JUnit 4/5 with Robolectric (existing pattern in common module tests) > 3. Test cases to implement: > - generateNewKey creates key with isDeprecated=false and current timestamp > - generateNewKey assigns incrementing version IDs (K001, K002, etc.) > - getActiveKey returns the most recently generated non-deprecated key > - getKeyByVersion returns correct metadata for known version > - getKeyByVersion returns null for unknown version > - deprecateKey marks key as deprecated (isDeprecated=true) > - deprecateKey does not affect other keys > - loadSecretKey returns AES SecretKey for valid version > - loadSecretKey throws for unknown version > - pruneExpiredKeys removes deprecated keys older than retention period > - pruneExpiredKeys does NOT remove non-deprecated (active) keys regardless of age > - pruneExpiredKeys does NOT remove deprecated keys within retention period > - Multiple key lifecycle: generate -> deprecate old -> generate new -> verify both accessible > - getDeprecatedKeys returns only deprecated keys > 4. Use @RunWith(RobolectricTestRunner.class) pattern > 5. Use ApplicationProvider.getApplicationContext() for context > > ### Reference Files > > - KeyVersionRegistry on fadi/secret-key-rotation branch (PR #3031) > - KeyMetadata data class (PR #3023) > - Existing test patterns: common/common/src/test/java/com/microsoft/identity/common/internal/cache/SharedPreferencesFileManagerTest.java > - Design doc test cases: design-docs/[Android] Symmetric Key Rotation/symmetric-key-rotation.md (Testing Strategy section) > > ### Acceptance Criteria > > - Test file created at specified path > - All test cases pass: ./gradlew :common:testDebugUnitTest --tests *.KeyVersionRegistryTest > - Tests cover happy path and edge cases > - Uses Robolectric for Android context > - Copyright header included > - KDoc comments on test class > > Follow .github/copilot-instructions.md strictly. > </details> <!-- START COPILOT CODING AGENT TIPS --> --- 💬 Send tasks to Copilot coding agent from [Slack](https://gh.io/cca-slack-docs) and [Teams](https://gh.io/cca-teams-docs) to turn conversations into code. Copilot posts an update in your thread when it's finished. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: fadidurah <88730756+fadidurah@users.noreply.github.com> Co-authored-by: mohitc1 <22034758+mohitc1@users.noreply.github.com> Co-authored-by: Mohit <mchand@microsoft.com> Co-authored-by: pedro romero vargas <76129899+p3dr0rv@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: fadidurah <fadidurah@microsoft.com>
|
❌ Work item link check failed. Description does not contain AB#{ID}. Click here to Learn more. |
There was a problem hiding this comment.
Pull request overview
This PR introduces infrastructure for symmetric key versioning/rotation and expands telemetry capabilities (EU telemetry routing via TDBR claim + enhanced passkey/WebAuthn telemetry), along with a small WebView silent-flow behavior fix.
Changes:
- Add a new
KeyVersionRegistry(Android) andKeyMetadata(common4j) to support versioned symmetric keys and pruning/rotation policies (flighted viaSYMMETRIC_KEY_MAX_AGE_DAYS). - Add EU telemetry region storage (
EUClaimStorageUtil) keyed by tenant + emittenant_id/new OTel attributes, and enhance passkey telemetry (origin + AAGUID extraction and emission). - Prevent silent embedded WebView flows from registering a back-press interceptor; add tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| common4j/src/test/com/microsoft/identity/common/java/opentelemetry/EUClaimStorageUtilTest.kt | Unit tests for storing TDBR claim by tenant. |
| common4j/src/test/com/microsoft/identity/common/java/crypto/KeyMetadataTest.java | Unit tests for KeyMetadata builder + JSON roundtrip/validation. |
| common4j/src/main/com/microsoft/identity/common/java/providers/microsoft/azureactivedirectory/ClientInfo.java | Add xms_tdbr claim parsing + set tenant_id attribute when present. |
| common4j/src/main/com/microsoft/identity/common/java/opentelemetry/SpanName.java | Add span name for key generation in KeyVersionRegistry. |
| common4j/src/main/com/microsoft/identity/common/java/opentelemetry/EUClaimStorageUtil.kt | Store TDBR claim by tenant and attach tenant to current span. |
| common4j/src/main/com/microsoft/identity/common/java/opentelemetry/AttributeName.java | Add correlation_id_v2, passkey_origin, passkey_aaguid. |
| common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java | Add SYMMETRIC_KEY_MAX_AGE_DAYS flight for key pruning/rotation. |
| common4j/src/main/com/microsoft/identity/common/java/crypto/KeyMetadata.java | New immutable metadata model + JSON serialization for key versions. |
| common4j/src/main/com/microsoft/identity/common/java/constants/FidoConstants.kt | Add WebAuthn registration + authenticator-data parsing constants. |
| common4j/src/main/com/microsoft/identity/common/java/constants/CborConstants.kt | Add CBOR parsing constants used for attestation parsing. |
| common/src/test/java/com/microsoft/identity/common/internal/providers/oauth2/SilentWebViewAuthorizationFragmentTest.kt | Robolectric test ensuring silent fragment doesn’t register back callback. |
| common/src/test/java/com/microsoft/identity/common/internal/fido/WebAuthnJsonUtilTest.kt | Tests for extracting origin + AAGUID from registration responses/attestation. |
| common/src/test/java/com/microsoft/identity/common/crypto/KeyVersionRegistryTest.java | Robolectric unit tests for KeyVersionRegistry behavior. |
| common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/SilentWebViewAuthorizationFragment.kt | Skip back-press registration by not calling parent onViewCreated(). |
| common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/PasskeyWebListener.kt | Ensure CredentialManager calls are launched on main; thread model doc; pass OTel context. |
| common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/PasskeyReplyChannel.kt | Record additional passkey telemetry (origin/AAGUID) asynchronously; propagate baggage/OTel context. |
| common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/AuthorizationActivity.java | Add explicit getOtelContext() accessor. |
| common/src/main/java/com/microsoft/identity/common/internal/fido/WebAuthnJsonUtil.kt | Add origin and AAGUID extraction logic; restructure util as object. |
| common/src/main/java/com/microsoft/identity/common/internal/fido/LegacyFidoActivityResultContract.kt | Update import due to WebAuthnJsonUtil companion/object refactor. |
| common/src/main/java/com/microsoft/identity/common/crypto/KeyVersionRegistry.java | New registry for generating/wrapping/pruning versioned symmetric keys with telemetry. |
| common/src/androidTest/java/com/microsoft/identity/common/crypto/KeyVersionRegistryTest.java | Instrumented tests exercising AndroidKeyStore + file/prefs persistence. |
| common/build.gradle | Add opentelemetry-extension-kotlin dependency for coroutine context propagation. |
| changelog.txt | Add vNext entries for EU TDBR routing, silent WebView back behavior, and passkey telemetry. |
| // Wrap and persist key material. | ||
| // RSA does not require an IV/AlgorithmParameters, so null is correct here. | ||
| final KeyPair keyPair = getOrCreateWrappingKeyPair(); | ||
| final byte[] wrappedKey = AndroidKeyStoreUtil.wrap(secretKey, keyPair, WRAP_ALGORITHM, null); | ||
| FileUtil.writeDataToFile(wrappedKey, getKeyFile(newVersionId)); | ||
|
|
||
| // Build and persist metadata. Explicitly set algorithm and keySize to match | ||
| // the AES-256 key generated above. | ||
| final KeyMetadata metadata = KeyMetadata.builder() | ||
| .versionId(newVersionId) | ||
| .createdAtMillis(System.currentTimeMillis()) | ||
| .algorithm(KeyMetadata.DEFAULT_ALGORITHM) | ||
| .keySize(KeyMetadata.DEFAULT_KEY_SIZE) | ||
| .build(); | ||
|
|
||
| state.keys.add(metadata); | ||
| saveState(state); | ||
|
|
There was a problem hiding this comment.
In generateNewKey(), the wrapped key file is written to disk before metadata is persisted. If saveState(state) throws (e.g., JSON/SharedPreferences failure), the key file can be left orphaned on disk without a corresponding metadata entry. Impact: leaked key blobs and inconsistent registry state that may accumulate over time. Recommendation: make the operation more atomic (e.g., write metadata first, or delete the newly written key file in the catch path when state persistence fails).
There was a problem hiding this comment.
@copilot is it better that a key be orphaned on disk, or the alternative of key ketadata is persisted first, but then key file write fails, in which case our persisted data has a noisy failure?
There was a problem hiding this comment.
@copilot apply changes based on this feedback
…um (#3049) Broker's `RotationOrchestrator` (PR #119) requires two span names for symmetric key rotation telemetry that are currently hardcoded. This adds them centrally to `SpanName` in common4j. ## Changes - **`SpanName.java`** — appended two new entries after `KeyVersionRegistryGenerateKey`: - `SymmetricKeyRotationCheck` — span for key rotation eligibility check on broker startup - `SymmetricKeyRotation` — span for key rotation execution ```java /** * Span name for symmetric key rotation check on broker startup. */ SymmetricKeyRotationCheck, /** * Span name for symmetric key rotation execution. */ SymmetricKeyRotation ``` > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `www.puppycrawl.com` > - Triggering command: `/opt/hostedtoolcache/CodeQL/2.24.3/x64/codeql/tools/linux64/java/bin/java /opt/hostedtoolcache/CodeQL/2.24.3/x64/codeql/tools/linux64/java/bin/java -jar /opt/hostedtoolcache/CodeQL/2.24.3/x64/codeql/xml/tools/xml-extractor.jar --fileList=/tmp/codeql-scratch-b2678d0398b00160/dbs/java/working/files-to-index778550329887067804.list --sourceArchiveDir=/tmp/codeql-scratch-b2678d0398b00160/dbs/java/src --outputDir=/tmp/codeql-scratch-b2678d0398b00160/dbs/java/trap/java` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/AzureAD/microsoft-authentication-library-common-for-android/settings/copilot/coding_agent) (admins only) > > </details> <!-- START COPILOT CODING AGENT SUFFIX --> <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > ## [Symmetric Key Rotation][Common] Add key rotation span names to SpanName enum > > ### Objective > > Add two span name entries to the SpanName enum in common4j for symmetric key rotation telemetry. These are needed by RotationOrchestrator in broker (PR #119) which currently hardcodes them. > > ### Technical Requirements > > File: common/common4j/src/main/com/microsoft/identity/common/java/opentelemetry/SpanName.java > > Add these two entries after KeyVersionRegistryGenerateKey (note: need to add a comma after KeyVersionRegistryGenerateKey first): > > `java > /** > * Span name for symmetric key rotation check on broker startup. > */ > SymmetricKeyRotationCheck, > /** > * Span name for symmetric key rotation execution. > */ > SymmetricKeyRotation > ` > > ### Acceptance Criteria > > - Two new SpanName enum values added with JavaDoc > - Comma added after existing KeyVersionRegistryGenerateKey entry > - Copyright header preserved > - Compile check passes: ./gradlew :common4j:compileJava > > Follow .github/copilot-instructions.md strictly. > </details> <!-- START COPILOT CODING AGENT TIPS --> --- 📍 Connect Copilot coding agent with [Jira](https://gh.io/cca-jira-docs), [Azure Boards](https://gh.io/cca-azure-boards-docs) or [Linear](https://gh.io/cca-linear-docs) to delegate work to Copilot in one click without leaving your project management tool. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: fadidurah <88730756+fadidurah@users.noreply.github.com>
|
✅ Work item link check complete. Description contains link AB#2956404 to an Azure Boards work item. |
…tion-library-common-for-android into fadi/secret-key-rotation
| private KeyPair getOrCreateWrappingKeyPair() throws ClientException { | ||
| final String methodTag = TAG + ":getOrCreateWrappingKeyPair"; | ||
| KeyPair keyPair = AndroidKeyStoreUtil.readKey(WRAPPING_KEY_ALIAS); | ||
| if (keyPair == null) { | ||
| Logger.info(methodTag, "No existing wrapping key pair, generating a new one."); | ||
| keyPair = generateNewWrappingKeyPair(); | ||
| } | ||
| return keyPair; | ||
| } |
There was a problem hiding this comment.
Severity: High – Wrapping key pair recovery can permanently orphan existing wrapped keys. getOrCreateWrappingKeyPair() generates a new RSA key pair whenever AndroidKeyStoreUtil.readKey(WRAPPING_KEY_ALIAS) returns null; if brokerks_metadata already contains existing key versions, those wrapped key blobs will no longer be unwrap-able, breaking decryption of existing encrypted data and leaving the registry in an inconsistent state. Recommendation: if the wrapping key pair is missing while any key versions exist, fail fast (throw a ClientException that the caller can handle by wiping caches) or explicitly wipe all wrapped key files + metadata before generating a new wrapping key pair, so the system does not silently keep undecryptable historical keys.
There was a problem hiding this comment.
@copilot realistically, when would AndroidKeyStoreUtil.readKey(WRAPPING_KEY_ALIAS) ever return null if brokerks_metadata already contains some existing key that were wrapped by it?
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…yMetadata.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@fadidurah I've opened a new pull request, #3054, to work on those changes. Once the pull request is ready, I'll request review from you. |
…yMetadata.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…_AGE_DAYS returns 0 or negative (#3054) The `pruneExpiredKeys()` fallback path for invalid ECS values (`maxAgeDays <= 0`) had no test coverage, meaning regressions (e.g., pruning all keys when the flight returns 0) would go unnoticed. ## Changes - **Two new tests** in `KeyVersionRegistryTest`: - `pruneExpiredKeys_usesDefaultWhenFlightReturnsZero` — asserts a recently-created key survives when the flight returns `0` - `pruneExpiredKeys_usesDefaultWhenFlightReturnsNegative` — same for `-1` - **`buildFlightsManagerWithMaxAgeDays(int)`** helper — concrete `IFlightsManager`/`IFlightsProvider` implementation that returns the specified value for `SYMMETRIC_KEY_MAX_AGE_DAYS` and each flight's own default otherwise; injected via `CommonFlightsManager.INSTANCE.initializeCommonFlightsManager(...)` - **`tearDown()`** now calls `CommonFlightsManager.INSTANCE.resetFlightsManager()` to prevent test pollution ```java @test public void pruneExpiredKeys_usesDefaultWhenFlightReturnsZero() throws ClientException { registry.generateNewKey(); CommonFlightsManager.INSTANCE.initializeCommonFlightsManager(buildFlightsManagerWithMaxAgeDays(0)); registry.pruneExpiredKeys(); assertNotNull("Key should not be pruned when flight returns 0; default should be used", registry.getKeyByVersion("K001")); } ``` <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: fadidurah <88730756+fadidurah@users.noreply.github.com>
…/microsoft-authentication-library-common-for-android into fadi/secret-key-rotation
|
@copilot please respond to my two recent comment replies that tagged you |
Symmetric Key Rotation Common Module Changes
Overview
This PR merges the
fadi/secret-key-rotationfeature branch intodev, introducing the common module components for the Symmetric Key Rotation feature.The Symmetric Key Rotation feature addresses SDL compliance gaps in the Android Broker's AES-256 symmetric key management. Currently, the broker uses a single, never-rotated symmetric key for encrypting local caches (tokens, accounts, credentials). This feature introduces a multi-key-version architecture that supports key rotation without requiring data migration new data is encrypted with the active key, while old data remains decryptable using any previous key version.
Design Document
PRs Included
The following PRs were merged into the feature branch and are included in this rollup:
@Builderdata class for symmetric key metadata (versionId, createdAtMillis, algorithm, keySize, isDeprecated)SYMMETRIC_KEY_MAX_AGE_DAYSflight to CommonFlight enum for key expiry configurationSymmetricKeyRotationCheckandSymmetricKeyRotationto SpanName enum for OpenTelemetry instrumentationCompanion PR
AB#2956404