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

remove bouncy castle dependency from edgeHub #6019

Merged
merged 19 commits into from
Feb 1, 2022
Merged

remove bouncy castle dependency from edgeHub #6019

merged 19 commits into from
Feb 1, 2022

Conversation

vipeller
Copy link
Contributor

@vipeller vipeller commented Jan 20, 2022 •
edited
Loading

A quick solution to see how bouncy castle could be removed. What missing are:
- error handling
- Import apparently does not like PKCS-8

Now it contains the full solution

@vipeller vipeller added the do-not-merge Used to indicate that this PR must not be merged yet label Jan 20, 2022
varunpuranik
varunpuranik reviewed Jan 20, 2022
View reviewed changes
Copy link
Contributor

@varunpuranik varunpuranik left a comment

Choose a reason for hiding this comment

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

Changes look good..

  • Did you try removing the dependency as well?
  • Did you get a chance to test it as well? Does it work?
  • Is this all that we need to do?

@vipeller
Copy link
Contributor Author

vipeller commented Jan 20, 2022 •
edited
Loading

I have not removed the dependency to the package yet, just the 'using'-s.
I started it from visual studio with an rsa cerrtificate, that worked.
I need to test it with EC, and also with pkcs8 keys (that I realized that those would not work with the ImportRSAPrivateKey() method, but from Martin's code it turned out that there is a separate ImportPkcs8PrivateKey() or similar call to read those)
The unit tests fail because those use pkcs8 keys. Once I add to import to pkcs8, they should start working.
The code will be a little-bit more complex with the pkcs8 part and with the error handling, but not so much.
I think it is about 1-2 hours to finish with the code, then I need to test it with 3 more key types (EC pkcs1/pkcs8 and rsa pkcs8 (I've tested rsa pkcs1 because that is what I have with my setup)

I've put together this code quickly only just to see the magnitude of the work, but it was easier than I thought.

@vipeller vipeller removed the do-not-merge Used to indicate that this PR must not be merged yet label Jan 21, 2022
@vipeller vipeller marked this pull request as ready for review January 21, 2022 01:54
varunpuranik
varunpuranik reviewed Jan 23, 2022
View reviewed changes
edge-util/src/Microsoft.Azure.Devices.Edge.Util/CertificateHelper.cs Outdated

public static class CertificateHelper
{
// The private-key import on windows randomly seems failing, however according to the tests, the second time
// after a failure it usually works. The number below is just a "big enough" number randomly chosen for
// self-healing, but gives a limit to avoid endless try.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to combine the 2 fixes? I would rather keep them separate... unless we need to do this as part of BC removal, for some reason.

varunpuranik
varunpuranik reviewed Jan 23, 2022
View reviewed changes
edge-util/src/Microsoft.Azure.Devices.Edge.Util/CertificateHelper.cs Outdated
// self-healing, but gives a limit to avoid endless try.
const int MaxCertImportRetryCount = 10;

private static Oid oidRsaEncryption = Oid.FromFriendlyName("RSA", OidGroup.All);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: remove private qualifier

varunpuranik
varunpuranik reviewed Jan 23, 2022
View reviewed changes
edge-util/src/Microsoft.Azure.Devices.Edge.Util/CertificateHelper.cs
string ecLabel = "EC PRIVATE KEY";

var keyAlgorithm = certificate.GetKeyAlgorithm();

Copy link
Contributor

@varunpuranik varunpuranik Jan 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: remove empty lines..

varunpuranik
varunpuranik reviewed Jan 23, 2022
View reviewed changes
edge-util/src/Microsoft.Azure.Devices.Edge.Util/CertificateHelper.cs Outdated

try
{
// On Windows the certificate in 'result' gives an error when used with kestrel: "No credentials are available in the security"
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is Windows only, maybe we can keep it to release/1.1 only?

varunpuranik
varunpuranik reviewed Jan 23, 2022
View reviewed changes
edge-util/src/Microsoft.Azure.Devices.Edge.Util/CertificateHelper.cs Outdated
static X509Certificate2 AttachPrivateKey(X509Certificate2 certificate, string pemEncodedKey)
{
int retryCount = 0;
while (retryCount++ < MaxCertImportRetryCount)
Copy link
Contributor

Choose a reason for hiding this comment

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

If this code is Windows specific.. then maybe we don't need it in master?

@varunpuranik
Copy link
Contributor

What missing are:

  • error handling
  • Import apparently does not like PKCS-8

Is this still applicable? I do see PKCS-8 support.
Otherwise changes look good.

varunpuranik
varunpuranik previously approved these changes Jan 27, 2022
View reviewed changes
@darobs
Copy link
Contributor

darobs commented Jan 28, 2022

#6051 should fix the E2E checkin build failure.

@darobs
Copy link
Contributor

darobs commented Jan 28, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@kodiakhq kodiakhq bot merged commit aa22379 into Azure:master Feb 1, 2022
vipeller added a commit to vipeller/iotedge that referenced this pull request Feb 1, 2022
@vipeller
cherrypick: remove bouncy castle dependency from edgeHub Azure#6019
9e377ae
vipeller added a commit to vipeller/iotedge that referenced this pull request Feb 1, 2022
@vipeller
cherry pick: removing bouncy castle Azure#6019
38cf0fc
kodiakhq bot pushed a commit that referenced this pull request Feb 2, 2022
@vipeller
cherrypick: remove bouncy castle dependency from edgeHub #6019 (#6062)
403ca87
kodiakhq bot pushed a commit that referenced this pull request Feb 4, 2022
@vipeller
cherry pick: removing bouncy castle #6019 (#6065)
f18f057
onalante-msft pushed a commit to onalante-msft/iotedge that referenced this pull request Feb 7, 2022
@vipeller @onalante-msft
remove bouncy castle dependency from edgeHub (Azure#6019)
87f73cf 
A quick solution to see how bouncy castle could be removed. <del>What missing are:
<del>- error handling
<del>- Import apparently does not like PKCS-8

Now it contains the full solution
damonbarry pushed a commit to damonbarry/iotedge that referenced this pull request Apr 15, 2022
@vipeller @damonbarry
remove bouncy castle dependency from edgeHub (Azure#6019)
bf909a4 
A quick solution to see how bouncy castle could be removed. <del>What missing are:
<del>- error handling
<del>- Import apparently does not like PKCS-8

Now it contains the full solution
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@varunpuranik varunpuranik varunpuranik approved these changes

@ancaantochi ancaantochi Awaiting requested review from ancaantochi

Assignees
No one assigned
Labels
ready-to-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vipeller @varunpuranik @darobs