-
-
Notifications
You must be signed in to change notification settings - Fork 3
Initial version #1
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
Conversation
f345499 to
208fa41
Compare
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
208fa41 to
1dfd47a
Compare
1dfd47a to
4044916
Compare
4044916 to
adcfef8
Compare
Ellerbach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks real good. Just open question to clarify on elements related to native and requirement on the device side. Just to make sure we can have it on any firmware to allow the modem managed implementations to use those security elements
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.InternalCall)] | ||
| extern private static byte[] HashCore(byte[] key, byte[] source); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this always present in all the firmware? Otherwise we should provide as well a pure managed version for modem implementation where we want to be able to run this on any system, not just those have System.Net enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System.Net is not a requirement to have this.
Just like any other assembly the availability on the fw image is dependent on a build option.
We can bring in this independent of it.
If the target doesn't have support for it, VS won't deploy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but then, we are loosing the capability to have this done on a pure managed way. I would prefer to have then a native pure software implementation when it's not implemented with some native platform functions.
And somehow have those few API always available on the native side. So regardless, we're sure we can count on them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having cryptography always available in all targets adds a bunch of code that's most likely not going to be used.
Particularly impactful for devices with small flash.
Of course we can offer a pure managed version of all these. The code is already available. It's a matter of moving it here.
If you ask me: my gut feeling is that 99.99% of projects using cryptography are also using network, therefore will benefit from the native availability of the images we're enabling this.
For the few others, that will be a simple matter of doing a local build and enabling this feature. Just as it happens with several other build options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we just added modem support with purely managed classes. So then, we would have to provide as well, like it is today and only for the purely managed the HMACCSHA256 (and SHA256) as purely managed. So we will have this purely managed implementation.
And so, then, ok to have this one not on all devices and on build. The path then should be clear:
-> wifi/ethnet => the Azure lib + all this new nuget
-> modem => managed Azure lib only (it will come with managed crypto)
I think I'm fine with this! (not CRC32 is a different discussion ;-))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually see 3 paths here... each with benefits and pitfalls...
-
in the managed class, if an exception is thrown (because the feature is not available) call a managed class.
-
implement the "native" software emulated implementation.
-
throw a warning to "suggest" using an independent lib that includes it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a forth...
Have 2 nugets available, one purely managed and one using the native implementation... We kind of do that for some other libs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a project referencing Sys.Sec.Cryptography, if this is not available in the target device, the deployment won't even happen, so 1. above is technically impossible.
All these cryptography implementations are supplied through Mbed TLS. They are all software based. On some platforms there are hardware accelerators for some algorithms, but this is a just a detail on the implementation.
I'm convinced that enabling cryptography for all targets it's a mistake because those that do not have network enabled (and please keep in mind that this is independent of it, just mentioning it for the sake of this conversation) are meant to be slim and have no use for it. This is exposed as a build option and one can always build this if needed.
We can also split the distribution of this library by the various features, but maybe that would be overengineering this a bit too much and getting into a granularity level that's not reasonable...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be treated similarly to what we have with Math library: support for DP and "full" double type, or standard build/distribution with the lightweight one.
We come up with what are the classes that are the "basic" cryptography (seems to be HMACCSHA256 + SHA256) and have those available as the "default" in native implementation. The remaining ones are available in the C# API and either have their implementation provided as build option, or simply throw an NotAvailableException.
Tests/System.Security.CryptographyTests/Properties/AssemblyInfo.cs
Outdated
Show resolved
Hide resolved
- Add license header. - Add description.
Co-authored-by: Laurent Ellerbach <laurelle@microsoft.com>
Ellerbach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! And as always, bonus points for the tests ;-)
Yes! It's really awesome that we can rely on this tool. |
|
Kudos, SonarCloud Quality Gate passed!
|








Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist: