[Https] Fix for issue getting private key after having previously exported it - #21734
[Https] Fix for issue getting private key after having previously exported it#21734javiercn wants to merge 1 commit into
Conversation
Pilchie
added
the
area-commandlinetools
|
👀 |
dougbu
left a comment
dougbu
left a comment
There was a problem hiding this comment.
If we get dotnet/runtime#36287 into preview5 (not sure why that currently targets 'master'), would this change be needed?
| // Cache to store the result of validating whether a key is exportable or not. | ||
| // This avoid requesting the key multiple times and avoids an issue when we try to access | ||
| // the key after it has been exported. | ||
| private Dictionary<string, bool> _areKeysExportable = new Dictionary<string, bool>(); |
There was a problem hiding this comment.
What prevents this dictionary from unbounded growth?
There was a problem hiding this comment.
If we get dotnet/runtime#36287 into preview5 (not sure why that currently targets 'master'), would this change be needed?
From our last tactics discussion dotnet/runtime were planning to do this after preview5. Given that this seems to be happening in preview5, we can pause on this PR.
There was a problem hiding this comment.
I just confirmed with the runtime team - they'll be taking the fix later in 5.0. So this is still needed!
There was a problem hiding this comment.
And it seems I've been overruled by the higher powers and the runtime fix will be brought into 5.0-preview5!
| // Cache to store the result of validating whether a key is exportable or not. | ||
| // This avoid requesting the key multiple times and avoids an issue when we try to access | ||
| // the key after it has been exported. | ||
| private Dictionary<string, bool> _areKeysExportable = new Dictionary<string, bool>(); |
There was a problem hiding this comment.
Does this need to be OrdinalIgnoreCase \ Ordinal?
| rsaPrivateKey.CspKeyContainerInfo.Exportable) || | ||
| (c.GetRSAPrivateKey() is RSACng cngPrivateKey && | ||
| cngPrivateKey.Key.ExportPolicy == CngExportPolicies.AllowExport); | ||
| if (_areKeysExportable.TryGetValue(c.Thumbprint,out var result)) |
There was a problem hiding this comment.
| if (_areKeysExportable.TryGetValue(c.Thumbprint,out var result)) | |
| if (_areKeysExportable.TryGetValue(c.Thumbprint, out var result)) |
mkArtakMSFT
added
the
* NO MERGE *
5 participants
Fixes #21733
The fix here is to cache the results for each certificate we check for exportable keys the first time so as to avoid running into the issue mentioned in .NET Core.
This is a workaround for an underlying runtime issue, which will be fixed in .NET 5.0: dotnet/runtime#36273