-
Notifications
You must be signed in to change notification settings - Fork 109
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
lints: enforce Mozilla PKI policy RSASSA-PSS encoding requirements #377
Conversation
@mtgag If you wouldn't mind could you resolve the conflicts on this branch? I'll start reviewing it afterwards. Thanks! |
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.
Thanks @mtgag. Here's some initial feedback.
v2/lints/mozilla/lint_mp_pss_parameters_encoding_correct_test.go
Outdated
Show resolved
Hide resolved
Co-Authored-By: Daniel McCarney <daniel@binaryparadox.net>
Co-Authored-By: Daniel McCarney <daniel@binaryparadox.net>
Co-Authored-By: Daniel McCarney <daniel@binaryparadox.net>
Co-Authored-By: Daniel McCarney <daniel@binaryparadox.net>
Thanks for the iteration @mtgag! I'll be travelling over the next few days but will endeavour to give the code & your replies my attention next week. |
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.
Thanks @mtgag, I appreciate your patience through the review process.
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.
These look good to me, so I'm not doing "Request Changes", but there's one possible easy improvement and one possibly quite difficult (and thus better not in this change, but worth noting).
ExpectedResult: lint.Pass, | ||
}, | ||
{ | ||
Name: "Standard RSASSA-PSS with SHA512 but the hash parameters are empty instead of NULL", |
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.
Suggestion: These variations all only exercise a single mis-encoding (empty hash parameters). Would it be useful to include explicit hash parameters that are more esoteric, such as a different mask length?
I'm not sure if I'm underestimating the difficulty, so I thought I'd check here.
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.
Added a test case for irregular (maybe invalid or unsupported is a better name for this?) salt length. Other test cases (e.g. irregular trailerField) are indeed more work to implement also because the crypto libraries do not support it out-of-the-box.
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.
@mtgag Thanks for adding another test case. I think the coverage in-branch is sufficient to see the lint merged. It would be nice to have more malformed test cases but it sounds like the difficulty would mean holding up the PR longer and I'd prefer to see it merged and iterated on as time allows.
Since @sleevi gave this branch a +1 without requesting the test case change as blocking feedback I'm going to go ahead and merge. @mtgag If you think you'd be up to adding the more difficult test cases would you mind filing an issue to act as a marker/discussion point?
Thanks all!
Implementation of #356