Add Simple AES Class

[AlirezaP]

Description

Motivation and Context

How Has This Been Tested?

Screenshots

Types of changes

• Improvement (non-breaking change that improves a feature, code or algorithm)
• Bug fix (non-breaking change which fixes an issue with code or algorithm)
• New feature (non-breaking change which adds functionality to code)
• Breaking change (fix or feature that would cause existing functionality to change)
• Config and build (change in the configuration and build system, has no impact on code or features)
• Dependencies (update dependencies and changes associated, has no impact on code or features)
• Unit Tests (add new Unit Test(s) or improved existing one(s), has no impact on code or features)
• Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist:

• My code follows the code style of this project (only if there are changes in source code).
• My changes require an update to the documentation (there are changes that require the docs website to be updated).
• I have updated the documentation accordingly (the changes require an update on the docs in this repo).
• I have read the CONTRIBUTING document.
• I have tested everything locally and all new and existing tests passed (only if there are changes in source code).
• I have added new tests to cover my changes.

[Ellerbach]

Can you please add a .gitignore in the root of the project? So it will be easier to review and won't bring the binaries in the commits?

[Ellerbach]

Thanks, couple of remarks to start, once with a gitignore, it's going to be more clear :-)
Also, you're missing all the nuget and related elements including the pipeline. Checkout the docs: https://docs.nanoframework.net/content/maintainers/creating-a-new-repo.html#prepare-the-initial-commit

[josesimoes]

This is a bit early in the process. The "base" project is still pending review and merge. Only after that it makes sense to start adding new features. 😉

[AlirezaP]

so am I supposed to continue or close the Pull request ?

[josesimoes]

so am I supposed to continue or close the Pull request ?

Please hold until the inital version is there. Then you can merge on your working branch and take it from there.
BTW, we're following the .NET API on these. Please make sure to follow AES API from here.

[AlirezaP]

sure. I would be happy if I could participate in this work

[AlirezaP]

Can you please add a .gitignore in the root of the project? So it will be easier to review and won't bring the binaries in the commits?

sure. i will apply all comments. i will try :)

[josesimoes]

@AlirezaP initial version merged into main. Please merge here and take it from there.

[Ellerbach]

@AlirezaP can you please make sure that you don't commit the .vs folder and other non needed folders? Once done, it will be better and easier to review the code. Thanks

[josesimoes]

@AlirezaP just took a quick look at this. Can you please point me to the .NET system.security.cryptography API that has this AES class? The policy is to follow the API from the full framework whenever possible.

[AlirezaP]

.Net system.security.cryptography (link) has AES class, but it works differently. It uses ICryptoTransform and CryptoStream to encrypt and decrypt data. But here, I use the simple method with the byte array in input and output.
If your policy forces to implement SymmetricAlgorithm, therefore, I should try to make it close to SymmetricAlgorithm. I hope I correctly understood your query.

[AlirezaP]

@AlirezaP can you please make sure that you don't commit the .vs folder and other non needed folders? Once done, it will be better and easier to review the code. Thanks

@Ellerbach I obeyed the mentioned note. As a test, I made a change in the .vs folder (I added a text file); it doesn't included in the git change.

[Ellerbach]

@AlirezaP can you please make sure that you don't commit the .vs folder and other non needed folders? Once done, it will be better and easier to review the code. Thanks

@Ellerbach Laurent Ellerbach FTE I obeyed the mentioned note. As a test, I made a change in the .vs folder (I added a text file); it doesn't included in the git change.

You have to get rid of all the files I4ve highlighted because they are part of your PR:

There are 2 options:

• make sure you remove them from your next commit
• Copy the AES.cs and nfproj somewhere else on your hard drive, close the PR, start clean from the main banch, clean the folders, create a brnahc bring back the 2 files and raise a new PR

[josesimoes]

The main goal here is to rely on the native provider for all these. For AES the full .NET version is going back and fort simply because it seems that AES has shown to have some security issues... or at least some versions(implementations) of it...

I understand (and agree) that going through the MemoryStream and CryptoStream is a bit too much for embedded systems scenarios. This seems to be pointing to a need to have a lightweight approach to this API as we've done a few times.
Which will happen (most likely) with other algorithms.

This may sound a blocker, still: we should come up with a list of algorithms that we want to have implemented and find out a common denominator for the methods and API.

[Ellerbach]

This may sound a blocker, still: we should come up with a list of algorithms that we want to have implemented and find out a common denominator for the methods and API.

I agree and on the simplification as well. We've been added some of the crypto algo first purely on a managed way because we mainly we needed them to connect to cloud providers like Azure and AWS.

We've always been also very pragmatic with all this. And we never were afraid to refactor some elements when we were able to cluster those. So, we should not neither be too afraid of bringing couple of those at first and refactor a bit later.

[AlirezaP]

The main goal here is to rely on the native provider for all these. For AES the full .NET version is going back and fort simply because it seems that AES has shown to have some security issues... or at least some versions(implementations) of it...

I understand (and agree) that going through the MemoryStream and CryptoStream is a bit too much for embedded systems scenarios. This seems to be pointing to a need to have a lightweight approach to this API as we've done a few times. Which will happen (most likely) with other algorithms.

This may sound a blocker, still: we should come up with a list of algorithms that we want to have implemented and find out a common denominator for the methods and API.

you are right. however, it is worth noting that the existence of a basic implementation of this algorithm in the Nanoframeworks security library can be useful; it can be customed and optimized proportional to the embedded devices in the future.

[AlirezaP]

@AlirezaP can you please make sure that you don't commit the .vs folder and other non needed folders? Once done, it will be better and easier to review the code. Thanks

@Ellerbach Laurent Ellerbach FTE I obeyed the mentioned note. As a test, I made a change in the .vs folder (I added a text file); it doesn't included in the git change.

You have to get rid of all the files I4ve highlighted because they are part of your PR:

There are 2 options:

• make sure you remove them from your next commit
• Copy the AES.cs and nfproj somewhere else on your hard drive, close the PR, start clean from the main banch, clean the folders, create a brnahc bring back the 2 files and raise a new PR

Ok, I will remove them from the next commit.

[AlirezaP]

@AlirezaP can you please make sure that you don't commit the .vs folder and other non needed folders? Once done, it will be better and easier to review the code. Thanks

@Ellerbach Laurent Ellerbach FTE I obeyed the mentioned note. As a test, I made a change in the .vs folder (I added a text file); it doesn't included in the git change.

You have to get rid of all the files I4ve highlighted because they are part of your PR:
There are 2 options:

• make sure you remove them from your next commit
• Copy the AES.cs and nfproj somewhere else on your hard drive, close the PR, start clean from the main banch, clean the folders, create a brnahc bring back the 2 files and raise a new PR

Ok, I will remove them from the next commit.

Actually, I clone the branch in a new project, and everything seems to be ok.

[Ellerbach]

Actually, I clone the branch in a new project, and everything seems to be ok.

It is not. Can you please either remove all the files, either start clean?
I'm not going to merge a PR containing non necessary files. You have the list of files in the PR, github shows it.
Thank you.

[AlirezaP]

@Ellerbach excuse me I didn't understand before. :)
it's done

[Ellerbach]

Thanks, much better without the non needed files :-) Couple of comments. What's important is to have both the API and the intellisense comments aligned with the full .NET, so it makes things easier when sharing code

[Ellerbach]

looks good all up. Can you add an example of usage in the main README?

[AlirezaP]

looks good all up. Can you add an example of usage in the main README?

sure. Done

[Ellerbach]

Now we are good with the code, would be perfect to add few tests. You can get inspiration from the existing ones. This will allow to ensure it's still working fine. Few cases should be enough.

[Ellerbach]

2 little extra lines. Then good for me. @josesimoes does this PR change the native signature?

[sonarqubecloud]

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud