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

Improve RSAPSS documentation #425

Merged
merged 2 commits into from
Sep 27, 2023

Conversation

ma-ilsi
Copy link
Contributor

@ma-ilsi ma-ilsi commented Sep 9, 2023

Summary of Changes

The documentation for RSAPSS receives additional clarity:

  • Usage example extended to show error handling should be present to deal with failure when loading keys.
  • Return value of Hacl_RSAPSS_new_rsapss_load_pkey and Hacl_RSAPSS_new_rsapss_load_skey explains that NULL is returned on failure.
  • Supported hash algorithms are listed in every applicable function and rendered in proper bullet point form, now.
  • Endianness/Byte-order addressed for key part parameters.
  • Additionally, I decided to explicitly mention the disjoint memory pre-condition only for the sgnt parameter in applicable functions in the case a client, for some odd reason, decides to overwrite other arguments with the produced signature.

Issues Resolved

Resolves #346.

Local Testing

./mach build --test: all passed.
./mach doc: finished, RSAPSS documentation page renders correctly.

@ma-ilsi ma-ilsi requested a review from a team as a code owner September 9, 2023 20:05
@cla-bot
Copy link

cla-bot bot commented Sep 9, 2023

We require contributors to sign our Contributor License Agreement https://github.com/cryspen/hacl/blob/main/CLA.md ensuring that the contribution can be licensed under Apache 2.0 and MIT. In order for us to review and merge your code, please mention @cryspen/core in a comment below to get yourself added.

@ma-ilsi
Copy link
Contributor Author

ma-ilsi commented Sep 9, 2023

@cryspen/core

include/Hacl_RSAPSS.h Outdated Show resolved Hide resolved
@franziskuskiefer franziskuskiefer self-requested a review September 12, 2023 12:46
Copy link
Member

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Hey, thanks for looking into this!

This code is all generated from F*. So we can't just change it here.
The code is coming from here and here etc.
Would you mind moving you changes over there?

include/Hacl_RSAPSS.h Outdated Show resolved Hide resolved
@ma-ilsi
Copy link
Contributor Author

ma-ilsi commented Sep 18, 2023

Would you mind moving you changes over there?

No problem. Once we finish/resolve our above conversation, I'll head over there.

tests/rsapss.cc Outdated Show resolved Hide resolved
Case analysis of invlaid key arguments.
@cla-bot
Copy link

cla-bot bot commented Sep 20, 2023

We require contributors to sign our Contributor License Agreement https://github.com/cryspen/hacl/blob/main/CLA.md ensuring that the contribution can be licensed under Apache 2.0 and MIT. In order for us to review and merge your code, please mention @cryspen/core in a comment below to get yourself added.

@ma-ilsi
Copy link
Contributor Author

ma-ilsi commented Sep 20, 2023

Here's the latest:

  • Branch updated to include .cpp test update only. So you can merge for that change only.

  • Original message of this PR updated to resolve Extend RSA-PSS examples. #346, only.

  • hacl-star PR: Improve RSAPSS documentation hacl-star/hacl-star#858 to increase clarity, endianness, return values.

  • Memory disjoint condition has been left out and can be addressed separately, later. (Verification conditions not checked on behalf of the client in the generated C code seems like a problem for this repo. You could add the memory note to the top of docs/reference/hacl/signature/rsapss.md file - anyways that's another issue).

That's about it. I think we're done here.

Copy link
Member

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Thanks

@franziskuskiefer
Copy link
Member

Can you update your branch? Then I can merge it.

@cla-bot cla-bot bot added the cla-signed label Sep 27, 2023
@ma-ilsi
Copy link
Contributor Author

ma-ilsi commented Sep 27, 2023

Gotcha

@franziskuskiefer franziskuskiefer enabled auto-merge (squash) September 27, 2023 09:42
@franziskuskiefer franziskuskiefer merged commit 27f40b2 into cryspen:main Sep 27, 2023
mamonet pushed a commit to mamonet/hacl-packages that referenced this pull request Dec 1, 2023
Case analysis of invlaid key arguments.
@ma-ilsi ma-ilsi deleted the rsapss_docs_update branch January 10, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Extend RSA-PSS examples.
2 participants