fix(storage): authorize MGF1 digest to prevent INCOMPATIBLE_DEVICE on…4.x - #1049
Open
utkrishtsahu wants to merge 1 commit into
Open
fix(storage): authorize MGF1 digest to prevent INCOMPATIBLE_DEVICE on…4.x#1049utkrishtsahu wants to merge 1 commit into
utkrishtsahu wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughChangesRSA cryptography updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
auth0/src/test/java/com/auth0/android/authentication/storage/CryptoUtilTest.java (1)
746-779: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the
CryptoExceptionmessage explicitly.
Assert.assertThrows(String, Class, ThrowingRunnable)uses the first argument as the assertion failure message. Capture the returnedCryptoExceptionand assertgetMessage()in both tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth0/src/test/java/com/auth0/android/authentication/storage/CryptoUtilTest.java` around lines 746 - 779, Update both RSADecrypt tests, shouldRecreateKeysAndThrowCryptoExceptionOnInvalidKeyExceptionWhenTryingToRSADecrypt and shouldRecreateKeysAndThrowCryptoExceptionOnInvalidAlgorithmParameterExceptionWhenTryingToRSADecrypt, to capture the CryptoException returned by Assert.assertThrows instead of passing the expected text as its assertion message. Explicitly assert that the captured exception’s getMessage() equals the documented message in each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@auth0/src/test/java/com/auth0/android/authentication/storage/CryptoUtilTest.java`:
- Around line 251-285: Rename
shouldNotAuthorizeMgf1DigestWhenCreatingRSAKeyPairBelowAPI33 to reflect the API
35 boundary, and retain it as the pre-API-35 coverage. Add a separate
`@Config`(sdk = 35) test around cryptoUtil.getRSAKeyEntry that captures the
KeyGenParameterSpec and asserts getMgf1Digests contains SHA-1 and SHA-256.
Ensure the new coverage exercises both successful authorization and the existing
failure/error path for MGF1 configuration.
---
Outside diff comments:
In
`@auth0/src/test/java/com/auth0/android/authentication/storage/CryptoUtilTest.java`:
- Around line 746-779: Update both RSADecrypt tests,
shouldRecreateKeysAndThrowCryptoExceptionOnInvalidKeyExceptionWhenTryingToRSADecrypt
and
shouldRecreateKeysAndThrowCryptoExceptionOnInvalidAlgorithmParameterExceptionWhenTryingToRSADecrypt,
to capture the CryptoException returned by Assert.assertThrows instead of
passing the expected text as its assertion message. Explicitly assert that the
captured exception’s getMessage() equals the documented message in each test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4823915d-18cf-429e-92c9-96c872497bfe
📒 Files selected for processing (2)
auth0/src/main/java/com/auth0/android/authentication/storage/CryptoUtil.javaauth0/src/test/java/com/auth0/android/authentication/storage/CryptoUtilTest.java
Comment on lines
+251
to
+285
| @Test | ||
| @Config(sdk = 30) | ||
| public void shouldNotAuthorizeMgf1DigestWhenCreatingRSAKeyPairBelowAPI33() throws Exception { | ||
| // Below API 33 setMgf1Digests does not exist; the key must be generated without it (the | ||
| // pre-existing behavior), relying on the SHA-1 default that older Keystore uses. | ||
| Mockito.when(keyStore.containsAlias(KEY_ALIAS)).thenReturn(false); | ||
| KeyStore.PrivateKeyEntry expectedEntry = Mockito.mock(KeyStore.PrivateKeyEntry.class); | ||
| Mockito.when(keyStore.getEntry(KEY_ALIAS, null)).thenReturn(expectedEntry); | ||
|
|
||
| ArgumentCaptor<AlgorithmParameterSpec> specCaptor = ArgumentCaptor.forClass(AlgorithmParameterSpec.class); | ||
|
|
||
| cryptoUtil.getRSAKeyEntry(); | ||
|
|
||
| Mockito.verify(keyPairGenerator).initialize(specCaptor.capture()); | ||
| Mockito.verify(keyPairGenerator).generateKeyPair(); | ||
| KeyGenParameterSpec spec = (KeyGenParameterSpec) specCaptor.getValue(); | ||
|
|
||
| // The rest of the spec is unchanged from the pre-fix behavior. | ||
| assertThat(spec.getEncryptionPaddings(), is(new String[]{KeyProperties.ENCRYPTION_PADDING_RSA_OAEP})); | ||
| assertThat(spec.getDigests(), is(new String[]{KeyProperties.DIGEST_SHA1, KeyProperties.DIGEST_SHA256})); | ||
|
|
||
| String[] mgf1Digests = readMgf1Digests(spec); | ||
| // No MGF1 digest was authorized (null/empty), matching the pre-fix key spec. | ||
| assertThat(mgf1Digests == null || mgf1Digests.length == 0, is(true)); | ||
| } | ||
|
|
||
|
|
||
| private static String[] readMgf1Digests(KeyGenParameterSpec spec) { | ||
| try { | ||
| java.lang.reflect.Method m = spec.getClass().getMethod("getMgf1Digests"); | ||
| Object result = m.invoke(spec); | ||
| return (String[]) result; | ||
| } catch (Throwable t) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add API 35 coverage for MGF1 authorization.
This test only verifies the API 30 path. It does not execute setMgf1Digests. Add an @Config(sdk = 35) test that asserts SHA-1 and SHA-256 are authorized. Rename this test because the production boundary is API 35, not API 33.
As per coding guidelines, test both success and failure paths for behavior changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@auth0/src/test/java/com/auth0/android/authentication/storage/CryptoUtilTest.java`
around lines 251 - 285, Rename
shouldNotAuthorizeMgf1DigestWhenCreatingRSAKeyPairBelowAPI33 to reflect the API
35 boundary, and retain it as the pre-API-35 coverage. Add a separate
`@Config`(sdk = 35) test around cryptoUtil.getRSAKeyEntry that captures the
KeyGenParameterSpec and asserts getMgf1Digests contains SHA-1 and SHA-256.
Ensure the new coverage exercises both successful authorization and the existing
failure/error path for MGF1 configuration.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
What & why: On newer Android Keystore2 hardware,
SecureCredentialsManagercan fail immediately after a successful login withCredentialsManagerException.INCOMPATIBLE_DEVICE("This device is not compatible with the ... class"), blocking token persistence even though authentication succeeded. Reported on Pixel 10 / Galaxy S26 (Android 16+).Root cause:
CryptoUtilencrypts stored credentials with an RSA-OAEP key whose cipher uses the MGF1/SHA-1 digest, but the key was generated without explicitly authorizing an MGF1 digest. Older KeyMint defaulted the MGF1 digest to SHA-1, so it matched. Newer Keystore2 firmware strictly enforces the authorized MGF1 set and its default is vendor-dependent (some default to SHA-256), so the private-key decrypt is rejected withINCOMPATIBLE_MGF_DIGEST("Incompatible padding mode") and surfaces asINCOMPATIBLE_DEVICE.Changes (both in
CryptoUtil.java, no public API change):getRSAKeyEntry()— when generating a new RSA key, explicitly authorize the MGF1 digests (SHA-1andSHA-256) viaKeyGenParameterSpec.Builder#setMgf1Digests, guarded toBuild.VERSION_CODES.VANILLA_ICE_CREAM(API 35+, where the API exists). The key no longer depends on the vendor's MGF1 default.RSADecrypt()—InvalidKeyException/InvalidAlgorithmParameterException(the MGF1-authorization rejection on an existing, pre-fix key) are now treated as recoverable: delete the stale RSA + AES keys and throwCryptoExceptionso the caller regenerates a correctly-authorized key, instead of the terminalIncompatibleDeviceException.NoSuchAlgorithmException/NoSuchPaddingExceptionremain terminal (genuine device incompatibility). This self-heals users already stuck (one re-login).No endpoints, classes, or public methods added/removed/deprecated.
CryptoUtilis internal; behavior change only.Testing
CryptoUtilcannot exercise the real Android Keystore under unit tests (Robolectric mocks it), so the hardware behavior was validated on physical devices and the exception-handling logic via unit tests.Updated the existing RSADecrypt
InvalidKeyExceptiontest to assert the new recovery behavior (throwsCryptoException, deletes RSA + AES keys) and added an equivalent test forInvalidAlgorithmParameterException.:auth0:testReleaseUnitTest(CryptoUtilTest) and:auth0:lintReleasepass (JDK 17).On-device verification (Pixel 10 / Android 17 and Galaxy S26 Ultra / Android 16): confirmed an MGF1 digest mismatch reproduces the exact
INCOMPATIBLE_MGF_DIGESTfailure, and that generating the key withsetMgf1Digests(SHA-1, SHA-256)+ the SDK's MGF1/SHA-1 cipher round-trips successfully.Not reproduced end-to-end on hardware: a device whose KeyMint defaults MGF1 to SHA-256 (the failing-on-default case) — our test devices default to SHA-1, so the failure was reproduced by forcing the mismatch. The fix removes reliance on the vendor default in all cases.
This change adds unit test coverage
This change adds integration test coverage
This change has been tested on the latest version of the platform/language or why not
Checklist
I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors
Summary by CodeRabbit