Skip to content

Conversation

@p3dr0rv
Copy link
Contributor

@p3dr0rv p3dr0rv commented Jun 16, 2025

Summary

This PR has two main goals: to seamlessly update the encryption padding used in the Android KeyStore, and to lay the groundwork for integrating future cryptographic requirements with minimal changes.


Phase 1: Replace AndroidWrappedKeyProvider with KeyStoreBackedSecretKeyProvider

  • Controller Flag: ENABLE_KEYSTORE_BACKED_SECRET_KEY_PROVIDER

  • Behavior When Enabled:

    • The factory will return KeyStoreBackedSecretKeyProvider instead of AndroidWrappedKeyProvider.
    • The new class behaves similarly to the previous one but introduces the ability to:
      • Retrieve the paddings supported by the KeyStore KeyPair
    • Maintain a list of available ciphers (oaepCipherSpec and pkcs1CipherSpec)
    • Select a compatible cipher for encryption/decryption based on what the KeyPair supports.
      • In this case, if the KeyPair only supports PKCS1, then all keys will be encrypted and decrypted using PKCS1.
  • Goal:

    • Validate that the new provider functions equivalently to the existing one.

    Note: No encryption changes are introduced in this phase.

Tests

  • AndroidWrappedKeyProviderTest has been parameterized to confirm both providers behave identically.
  • KeyStoreBackedSecretKeyProviderRolloutTest.java includes test to validate migration and rollback

Phase 2: Enable New Wrapped Secret Key Format with Metadata Support

Before introducing new key types, we need to ensure that all new keys include additional metadata that specifies how they were encrypted.

  • Controller Flag: ENABLE_NEW_WRAPPED_SECRET_KEY_FORMAT

  • Behavior When Enabled:

    • WrappedSecretKey will use an enhanced binary format with the following structure:
      • MAGIC BYTES (4 bytes)
      • SERIALIZER ID (4bytes)
      • Metadata length (4 bytes)
      • Metadata (format is decided by the serializer)
      • Raw encrypted key bytes
  • Goal:

    • Future-proof the wrapped key by including metadata that specifies:
      • Key size
      • Algorithm
      • Transformation used for encryption
      • Key version
    • This allows the system to handle multiple key types if formats change in the future.
    • Validate WrappedSecretKey can read both new and old keys.

Tests

  • WrappedSecretKeyTest

Phase 3: Add OAEP encryption padding.

  • Controller Flag: ENABLE_OAEP_WITH_SHA_AND_MGF1_PADDING

  • Behavior When Enabled:

    • CryptoParameterSpecFactory will now return two paddings: ENCRYPTION_PADDING_RSA_PKCS1 and ENCRYPTION_PADDING_RSA_OAEP for the KeyGen specifications.
    • KeyStoreBackedSecretKeyProvider will generate a KeyPair that supports ENCRYPTION_PADDING_RSA_OAEP.
    • As a result, when a SecretKey needs to be wrapped, the provider will select OAEP, since the KeyPair now supports this padding and OAEP cipher support was already introduced in Phase 1.
  • Goal:

    • All new keys will be encrypted /decrypted with OAEP
    • Existing keys will still be encrypted /decrypted with PKCS1

Tests

  • KeyStoreBackedSecretKeyProviderRolloutTest.java includes test to validate migration and rollback

Telemetry

span_name key_pair_gen_successful_method elapsed_time_keypair_generation secret_key_wrapping_cipher secret_key_wrapping_operation
SecretKeyWrapping null CipherSpec(transformation='RSA/ECB/PKCS1Padding') KeyStoreBackedSecretKeyProvider:unwrapSecretKey
KeyPairGeneration KeyGenSpec(description='modern_spec_with_wrap_key', algorithm='RSA', encryptionPaddings='[PKCS1Padding]') 581
KeyPairGeneration KeyGenSpec(description='modern_spec_with_wrap_key', algorithm='RSA', encryptionPaddings='[PKCS1Padding]') 294

AB#3274176
https://github.com/AzureAD/ad-accounts-for-android/pull/3166

Base automatically changed from pedroro/interface-for-secretkeyloader to dev July 4, 2025 05:16
@github-actions
Copy link

github-actions bot commented Jul 7, 2025

❌ Work item link check failed. Description does not contain AB#{ID}.

Click here to Learn more.

p3dr0rv added 14 commits July 7, 2025 12:48
…tants and documentation; refine StorageEncryptionManager logic; clean up PredefinedKeyProvider and MockAES256KeyProvider imports.
…move CipherSpec and KeyGenSpec classes; enhance CryptoParameterSpecFactory with updated parameter specifications.
…lasses; update unwrapping logic, enhance cipher specifications, and improve key retrieval methods.
…edKeyProvider

- Added AndroidWrappedKeyProviderFactory to create wrapped key loaders based on feature flags.
- Introduced OAEPAndroidWrappedKeyProvider for handling key wrapping and unwrapping using OAEP padding.
- Updated CryptoParameterSpecFactory to support new key generation specifications and improved documentation.
- Refactored CryptoSpecs to enhance clarity and maintainability.
- Removed deprecated methods from AndroidKeyStoreUtil to streamline key management.
- Enhanced unit tests for CryptoParameterSpecFactory to cover new functionality and edge cases.
- Cleaned up FileUtil by removing unused methods related to string file operations.
…ons; refactor NewAndroidWrappedKeyProvider to utilize new specs and remove legacy key generation methods.
…eyProvider in tests; add KeyStoreBackedSecretKeyProvider and factory for key management.
- Introduced `KeyStoreBackedSecretKeyProvider` to replace the legacy `AndroidWrappedKeyProvider`, improving security with enhanced encryption paddings.
- Updated `CryptoParameterSpecFactory` to include detailed initialization logging and improved cipher specification handling.
- Refactored `KeyGenSpec` and `LegacyKeyGenSpec` for better clarity and added documentation.
- Enhanced `AndroidKeyStoreUtil` to provide clearer methods for retrieving encryption paddings from key pairs.
- Updated tests to reflect changes in method names and ensure compatibility with the new key provider.
- Added telemetry attributes for secret key wrapping operations to improve observability.
- Modified feature flags to control the use of the new key provider implementation.
…rmats

- Added WrappedSecretKeyLegacySerializer for backward compatibility with legacy key formats.
- Implemented WrappedSecretKeySerializerManager to manage serialization formats and version detection.
- Updated WrappedSecretKeyTest to cover serialization and deserialization for both legacy and new formats.
- Enhanced version detection logic to handle different serialization formats.
- Modified CommonFlight to control the WrappedSecretKey serializer version instead of a simple enable/disable flag.
@p3dr0rv
Copy link
Contributor Author

p3dr0rv commented Sep 4, 2025

Is there any reason why Phase 2 and phase 3 should not be done together?

There’s no limitation in the code; I chose this approach so that in case of a rollback, it’s easier to move from one state to another. It also provides more granularity in the release, allowing us to ensure that each step of the new changes works as expected.

Copy link
Contributor

@mohitc1 mohitc1 left a comment

Choose a reason for hiding this comment

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

:shipit:

@p3dr0rv p3dr0rv merged commit 3f151f1 into dev Sep 26, 2025
24 of 25 checks passed
@p3dr0rv p3dr0rv deleted the pedroro/prototype branch September 26, 2025 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants