Skip to content

Conversation

@somalaya
Copy link
Contributor

@somalaya somalaya commented Aug 25, 2025

  • 1 of the SDL assessments has a requirement that we should avoid using multiple crypto purposes with the same key.
  • There are multiple places where we are using multiple purposes and device pop scenario is one of them.
  • In Device pop scenario, we do not use encrypt/decrypt methods anywhere in our code. Only signing is used. Hence removing the ununsed purposes encrypt, decrypt from the key gen spec.
  • Also, encrypt and decrypt methods added in IDevicePopManager.java are probably for maintaining all the crypto operations but I have deprecated them as they are not getting used.
  • Added telemetry in mintSignedHttpRequest method.

Note : The flight to remove encryption paddings in initialize method of AndroidDevicePopManager is not going to be enabled until we observe telemetry added in this PR first. Idea is that the telemetry should prove that encrypt/decrypt methods are not getting used. Only then, we can enable the flight! So, this is going to be enabled in Jan/Feb next year.

@github-actions
Copy link

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

Click here to Learn more.

@somalaya somalaya marked this pull request as ready for review August 25, 2025 22:43
Copilot AI review requested due to automatic review settings August 25, 2025 22:43
@somalaya somalaya requested a review from a team as a code owner August 25, 2025 22:43
Copy link
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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@somalaya somalaya changed the title Fix for SDL violation in device pop scenarios Fix for SDL violation in device pop scenarios, Fixes AB#3284510 Aug 27, 2025
@github-actions
Copy link

✅ Work item link check complete. Description contains link AB#3284510 to an Azure Boards work item.

@mohitc1
Copy link
Contributor

mohitc1 commented Aug 27, 2025

Is this needed if purpose does not have encrypt/decrypt/wrap?

If not, then this method is identical to initialize23, since caller decides if wrap key is to be set or not.


Refers to: common/src/main/java/com/microsoft/identity/common/internal/platform/AndroidDevicePopManager.java:470 in 42bf650. [](commit_id = 42bf650, deletion_comment = True)

@somalaya
Copy link
Contributor Author

Is this needed if purpose does not have encrypt/decrypt/wrap?

If not, then this method is identical to initialize23, since caller decides if wrap key is to be set or not.

Refers to: common/src/main/java/com/microsoft/identity/common/internal/platform/AndroidDevicePopManager.java:470 in 42bf650. [](commit_id = 42bf650, deletion_comment = True)

If you meant the encryption padding, then yes it is not needed when purpose is not encrypt/decrypt/wrap. I have skipped adding this padding when the flight is ON in 5c852db . Once the flight is completely ON, we can remove initialize23 and initalize28 and just add a check for Build version to use strongbox in API >=28.

@somalaya somalaya force-pushed the somalaya/devicePopSdlFix branch from 1c62817 to dca128e Compare September 12, 2025 01:54
@somalaya somalaya changed the title Fix for SDL violation in device pop scenarios, Fixes AB#3284510 Fix for SDL violation in device pop scenarios Oct 30, 2025
@somalaya somalaya force-pushed the somalaya/devicePopSdlFix branch from 1738709 to 76549a8 Compare October 30, 2025 04:09
@github-actions
Copy link

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

Click here to Learn more.

final boolean unnecessaryCryptoPurposesDisabled =
CommonFlightsManager.INSTANCE.getFlightsProvider().isFlightEnabled(CommonFlight.DISABLE_UNNECESSARY_CRYPTO_PURPOSES_FROM_DEVICE_POP_MANAGER);

int purposes = KeyProperties.PURPOSE_SIGN | KeyProperties.PURPOSE_VERIFY;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Consider moving the definition of purposes inside each initializeX method — I believe it makes things a little easier to read.

final boolean enableImport,
final boolean trySetAttestationChallenge) throws InvalidAlgorithmParameterException {
final boolean unnecessaryCryptoPurposesDisabled =
CommonFlightsManager.INSTANCE.getFlightsProvider().isFlightEnabled(CommonFlight.DISABLE_UNNECESSARY_CRYPTO_PURPOSES_FROM_DEVICE_POP_MANAGER);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: ask copilot for a better name :p

/**
* Flight to disable the unnecessary crypto operation purposes in device pop manager like encrypt, decrypt and wrap.
*/
DISABLE_UNNECESSARY_CRYPTO_PURPOSES_FROM_DEVICE_POP_MANAGER ("DisableUnnecessaryCryptoPurposes", false);
Copy link
Contributor

Choose a reason for hiding this comment

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

DisableUnnecessaryCryptoPurposes

Update this string as well?

@mohitc1
Copy link
Contributor

mohitc1 commented Nov 13, 2025

}

this sets it to null. Do we really need this method? if we delete this method, initialize23 and initialize28 can be simplified.

initialize23() also does not need to worry about Strongbox. If strong box is needed call initialize28


Refers to: common/src/main/java/com/microsoft/identity/common/internal/platform/AndroidDevicePopManager.java:424 in 9691151. [](commit_id = 9691151, deletion_comment = False)

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:

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