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.

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

Init RSA PSS SignatureAlgorithm Implementation #867

Merged
merged 2 commits into from
May 16, 2024

Conversation

MattiLE
Copy link
Contributor

@MattiLE MattiLE commented Apr 3, 2024

Summary

As far as I know, the existing RSACryptoServiceProvider implementation does not offer the option of specifying the PSS padding. My online research has also shown that this is not possible either.

Therefore, I would like to add my own implementation of the "SignatureAlgorithm" interface based on the "System.Security.Cryptography.RSA" class.
For my use case, only a function that signs the data is sufficient. However, I think that the interface implementation is more similar to the styleguid.

Work Item(s)

Fixes #866

Fixes AB#524188

@MattiLE MattiLE requested review from a team as code owners April 3, 2024 09:50
@github-actions github-actions bot added AL: System Application From Fork Pull request is coming from a fork labels Apr 3, 2024
@MattiLE MattiLE changed the title Init RSA PSS Init RSA PSS SignatureAlgorithm Implementation Apr 3, 2024
@JesperSchulz
Copy link
Contributor

Let's give this a CI run 🙂

@MattiLE
Copy link
Contributor Author

MattiLE commented Apr 4, 2024 via email

Copy link
Contributor

@darjoo darjoo left a comment

Choose a reason for hiding this comment

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

Things need to be cleaned up and tests need to be added.
This is just a preliminary review thus far.

@github-actions github-actions bot added this to the Version 25.0 milestone Apr 11, 2024
@JesperSchulz
Copy link
Contributor

We need @darjoo to complete the review. Unfortunately he is on a short break, but we'll get back to this PR as soon as he's back 🙂

@JesperSchulz JesperSchulz added the Integration GitHub request for Integration area label Apr 22, 2024
@github-actions github-actions bot added the Linked Issue is linked to a Azure Boards work item label May 7, 2024
@duichwer
Copy link

duichwer commented May 8, 2024

Do we really need all the the NonDebuggable properties?
I think they should be only neede in procedures that unwrap a SecretText..

@MattiLE
Copy link
Contributor Author

MattiLE commented May 14, 2024

Do we really need all the the NonDebuggable properties? I think they should be only neede in procedures that unwrap a SecretText..

I have compared the new objects with the existing ones and removed some NonDebuggable tags.

@MattiLE
Copy link
Contributor Author

MattiLE commented May 14, 2024

@darjoo @JesperSchulz
What is the status of the PR? Is there anything for me to do at the moment? Have you already been able to check if I can get the modification for 23 or at least 24?

@JesperSchulz
Copy link
Contributor

We'll give it a 3rd round of review. If approved, we can merge the code and backport for release with 24.2.

Copy link
Contributor

@darjoo darjoo left a comment

Choose a reason for hiding this comment

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

Almost there.

darjoo
darjoo previously approved these changes May 15, 2024
JesperSchulz
JesperSchulz previously approved these changes May 15, 2024
@JesperSchulz JesperSchulz enabled auto-merge (squash) May 15, 2024 12:01
@JesperSchulz
Copy link
Contributor

JesperSchulz commented May 15, 2024

@MattiLE, could you use IDs codeunit 1475 and codeunit 1476 (remember to add those to the app.json of the module.
Change range to:
{
"from": 1473,
"to": 1476
}

Also, you seem to have some unused variables, upon which build fails. Once fixed, I think we're ready to merge.

auto-merge was automatically disabled May 15, 2024 13:04

Head branch was pushed to by a user without write access

JesperSchulz
JesperSchulz previously approved these changes May 15, 2024
darjoo
darjoo previously approved these changes May 15, 2024
@JesperSchulz
Copy link
Contributor

JesperSchulz commented May 16, 2024

Looks like we have the same ID problem with the new test. @MattiLE, if you could re-ID the test to "codeunit 132617" and expand the range in the app.json, I think we're finally ready to rock 🙂

                 {
                     "from":  132611,
                     "to":  132617 <- CHANGE FROM 132615
                 }

@MattiLE MattiLE dismissed stale reviews from darjoo and JesperSchulz via 7e73140 May 16, 2024 09:16
@JesperSchulz JesperSchulz enabled auto-merge (squash) May 16, 2024 11:12
@JesperSchulz JesperSchulz merged commit fe02f53 into microsoft:main May 16, 2024
23 checks passed
@MattiLE
Copy link
Contributor Author

MattiLE commented Aug 12, 2024

@darjoo @JesperSchulz What is the status of the PR? Is there anything for me to do at the moment? Have you already been able to check if I can get the modification for 23 or at least 24?

Hi @JesperSchulz, the change is not yet included in the current BC24 version. Could you add it?

@MattiLE
Copy link
Contributor Author

MattiLE commented Aug 19, 2024

Hi @darjoo and @JesperSchulz, can you take the adjustments into the next BC24 CU Update?
We urgently need the adjustment for our project.

Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AL: System Application From Fork Pull request is coming from a fork Integration GitHub request for Integration area Linked Issue is linked to a Azure Boards work item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BC Idea]: Extend BC System APP with RSASSA-PSS algorithm
4 participants