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.

Sign up for GitHub

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

Jump to bottom

Interop\Windows\BCrypt\Cng.cs does not follow the Interop guidelines #14588

Closed
bartonjs opened this issue May 13, 2015 · 5 comments
Closed

Interop\Windows\BCrypt\Cng.cs does not follow the Interop guidelines #14588

bartonjs opened this issue May 13, 2015 · 5 comments
Assignees
@robert-matusewicz
Labels
area-System.Security enhancement Product code improvement that does NOT require public API changes/additions
Milestone
Future

Comments

@bartonjs
Copy link
Member

Cng.cs was ported as-was from closed source into open. It should be broken up into multiple Interop.Method.cs and/or Interop.MethodCollection.cs files, and the SafeHandle types extracted into files of their own right.
@bartonjs bartonjs self-assigned this May 13, 2015
stephentoub referenced this issue in stephentoub/corefx Jun 7, 2015
@stephentoub
Separate BCryptGenRandom out of Cng.cs
879182b 
As part of #1739, this commit begins to refactor Cng.cs, starting by pulling out the functionality needed for RandomNumberGenerator.  The assembly was being filled with all of the Cng code, when in reality it only needed a very small amount of it.  And since the assembly doesn't have any string resources, I also modified the .csproj to suppress the buildtools inclusion of the common resources support.

As a result of the removals, this change boosts the code coverage number of System.Security.Cryptography.RandomNumberGenerator.dll from 13% to 96%.

(Note that there are still copies of the code in Interop.NTSTATUS.cs in Cng.cs, due to other dependencies in Cng.cs that I didn't want to change.  As we refactor Cng.cs further, we can remove those copies.)
@joshfree joshfree assigned ghost and unassigned bartonjs May 2, 2016
@bartonjs bartonjs unassigned ghost Oct 3, 2016
@robert-matusewicz
Copy link
Contributor

@karelz @bartonjs - is there still a need to implement that tickete? If yes, I would be interested to do it.

Just to confirm that I understand that correctly - that the file that needs to adapt to guidelines is corefx/src/Common/src/Interop/Windows/BCrypt/Cng.cs

The work that needs to be done is:

  • extract BCrypt* methods to separate files, so BCryptImportKey should be extracted to Interop.BCryptImportKey.cd, BCryptDecrypt to Interop.BCryptDecrypt, etc?
  • handles should be extracted to their own files, so SafeAlgorithmHandle into SafeAlgorithmHandle .cs, SafeKeyHandle into SafeKeyHandle.cs

I'm not sure what do you mean by Interop.MethodCollection.cs - any example of how that should be done?

Cheers

@karelz
Copy link
Member

karelz commented Oct 6, 2019

@krwq @GrabYourPitchforks can you provide some guidance while Jeremy is on vacation?

@bartonjs
Copy link
Member Author

bartonjs commented Oct 7, 2019

While "one thing per file" can work, we've been accepting "one file per logical group".

  • BCRYPT_KEY_DATA_BLOB_HEADER is only used with BCryptImportKey, so they could both be in one file (probably Interop.BCryptImportKey.cs).
  • BCryptEncrypt and BCryptDecrypt are almost always needed together, so leave them in one file (Interop.BCryptEncryptDecrypt.cs?)
  • Put the helper methods for the P/Invokes in the same file as the P/Invokes
  • I'd probably put the extension methods SetCipherMode and SetEffectiveKeyLength with all of the SetProperty helpers and P/Invokes as Interop.BCryptSetProperty.cs. If that requires an assembly to pull in SafeAlgorithmHandle when it otherwise didn't need to, then I'd come up with something else.

@robert-matusewicz
Copy link
Contributor

awesome, thank you for explaining that to me:) @karelz can you assign me to that ticket - I will be slowly working on it.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@bartonjs bartonjs removed the untriaged New issue has not been triaged by the area owner label Feb 25, 2020
@bartonjs
Copy link
Member Author

bartonjs commented Mar 2, 2022

This is now being tracked under more active issues, such as #51564. Closing this redundant copy.

@bartonjs bartonjs closed this as completed Mar 2, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Apr 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@robert-matusewicz robert-matusewicz

Labels
area-System.Security enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Milestone
Future
Development

No branches or pull requests
5 participants
@robert-matusewicz @karelz @maryamariyan @msftgits @bartonjs