Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/8.0-staging] Re-try loading ENGINE keys with a non-NULL UI_METHOD #109782

Open
wants to merge 2 commits into
base: release/8.0-staging
Choose a base branch
from

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 13, 2024

Backport of #109706 to release/8.0-staging

/cc @vcsjones

Customer Impact

  • Customer reported
  • Found internally

Reported by a customer in #109243. Customers that use certain OpenSSL engines may encounter issues when using SafeEvpPKeyHandle.OpenPrivateKeyFromEngine or SafeEvpPKeyHandle.OpenPublicKeyFromEngine. These methods use ENGINE_load_private_key or ENGINE_load_public_key, respectively. One of the parameters to these methods is called ui_method. This method is used to inform an engine how it can prompt for user input or other interactivity, if needed, when loading a key.

Currently, NULL is passed as this parameter because we do not expect to show a user interface from this API, or any other kind of interactivity.

However, at least one engine, tpm2tss, will outright reject a NULL ui_method and fail to load the key.

This change accommodates engines that do not permit a null ui_method by trying again to open the key if it failed to open. The ui_method that we pass in is a stub one that does not perform any interactivity.

Customers are unable to work around this issue on their own.

Regression

  • Yes
  • No

Testing

This was manually validated. The issue is reproducible, and with the change, the error no longer occurs. Unit testing this is not feasible - it requires a particular native dependency set up and be installed (tpm2tss) as well as the corresponding OpenSSL engine. The machine must also have a functional TPM, which is not present in CI.

Risk

Low. This adds a fall-back mechanism. The way we were originally opening the key from an OpenSSL engine is still present. If it fails, we try again a different way to get it to load. Keys that were loading successfully already will continue to do so as the fallback path will never be taken. If the keys were failing to load, the worse case is they fail to load twice now.

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

@vcsjones
Copy link
Member

vcsjones commented Nov 20, 2024

I am going to close this back port request for now as we do not believe it will resolve a customer's scenario. If we later receive confirmation from someone that this unblocks a scenario for them, we can open it back up.

@vcsjones vcsjones closed this Nov 20, 2024
@vcsjones
Copy link
Member

Customer confirmed this resolved their scenario, re-opening for consideration.

@vcsjones vcsjones reopened this Nov 26, 2024
@jeffhandley jeffhandley added Servicing-consider Issue for next servicing release review Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Dec 3, 2024
@jeffhandley
Copy link
Member

This was approved for the February 2025 servicing release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Security Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants