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

Consider memoizing more CNG properties on CngKey that can't change after the key is finalized #89821

Closed
vcsjones opened this issue Aug 1, 2023 · 2 comments · Fixed by #99053
Assignees
Milestone

Comments

@vcsjones
Copy link
Member

vcsjones commented Aug 1, 2023

#89599 shows that accessing some CNG properties are relatively expensive to the cryptographic operation itself. That pull request memoized the result for KeySize so that reading the KeySize property doesn't incur the overhead of reading ncrypt properties, which requires an RPC call.

We should consider what other properties can be memoized for performance. #89819 showed that AlgorithmGroup is likely another candidate.

@vcsjones vcsjones added this to the Future milestone Aug 1, 2023
@ghost
Copy link

ghost commented Aug 1, 2023

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

#89599 shows that accessing some CNG properties are relatively expensive to the cryptographic operation itself. That pull request memoized the result for KeySize so that reading the KeySize property doesn't incur the overhead of reading ncrypt properties, which requires an RPC call.

We should consider what other properties can be memoized for performance. #89819 showed that AlgorithmGroup is likely another candidate.

Author: vcsjones
Assignees: -
Labels:

area-System.Security

Milestone: Future

@bartonjs
Copy link
Member

bartonjs commented Aug 1, 2023

I think there are times that we can see a non-"finalized" key, e.g. between PFX import and the key first use (which is why the export policy can be changed then). So the remembering part might need to be itself guarded by a semi-memoized state check on key finalization (remember yes, don't remember no).

@stephentoub stephentoub modified the milestones: Future, 9.0.0 Aug 17, 2023
@vcsjones vcsjones self-assigned this Sep 5, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 28, 2024
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 6, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Apr 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants