-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: add throw exception for rsa salt #5132
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
base: master
Are you sure you want to change the base?
fix: add throw exception for rsa salt #5132
Conversation
AgnesToulet
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.
Hi! Sorry for the delay and thanks a lot for your contribution!
This looks good (just left some wording suggestions) and works great but we probably want to update the tests in this case to expect the error, could you update your PR with updated tests please?
|
hi @AgnesToulet , thankyou for the feedback! I'll add that later. |
Co-authored-by: Agnès Toulet <[email protected]>
Co-authored-by: Agnès Toulet <[email protected]>
|
hi @AgnesToulet could you elaborate, which file that contain test that need to be updated? |
|
hi @AgnesToulet , I see, thankyou for that. sorry, I think I need more time to check & edit that within two weeks, is it okay? |
|
Sure, thanks for taking care of this! Feel free to reach out if you have any question/need help. |
|
hi @AgnesToulet could you help to run the workflow? thankyou |
|
@vincentandreas looks like the build is failing. Can you double check that the code compiles and build into a binary? |
|
hi @ankur22 @AgnesToulet , thanks for noticing, here's the updated version, sorry for that. kindly help to retrigger the workflow, thanks |
|
hi @AgnesToulet @ankur22 for the gofmt I already update. but for this error, I need more time to understand this. |
I'm taking a look. I believe these are flakey tests, so we might just be able to rerun then until they pass. I've just merged in master which contains some changes that might help too 🤞 |
|
Yeah, the pipeline was failing due to flakey tests, which we're hoping to resolve soon 🤞 I'm happy with the current situation with the pipeline/ |
|
hi @ankur22 I just update the branch with master, could you retrigger again? |
AgnesToulet
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.
LGTM, have you looked into checking that the functions throw an error instead of just skipping the tests?


What?
Implement throw exception in RSA PSS, when given salt length = 0, in Sign / Verify function
Why?
It improves UX and makes implementation predictable.
Checklist
make check) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)
Issues #4265
Previous PR #4659