-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat: cert verification, support multiple certs #50
Conversation
Allow multiple certificates to be specified when doing cert-based verification. All the certs are put in `AND` condition. Fixes: #49 Signed-off-by: Flavio Castelli <fcastelli@suse.com>
454f875
to
8cde3db
Compare
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, modulo the missing s/certificate/certificates/
in validate() of settings.rs.
Maybe worth it to change that labeled 'signatures
loop into a fails-closed that starts with the error, as done with the Response on this PR.
Rename the `mock_sdk` namespace to `mock_verification_sdk`. This allows for more host callbacks namespaces to be mocked. Signed-off-by: Flavio Castelli <fcastelli@suse.com>
On the `main` branch a set of changes landed that make it possible to have real settings validation for certificate based signatures. This commit fixes the build errors and organizes the settins code in a more structured way. These are the changes introduced: * Have each type of Signature implement the `Display` trait. This is used when propagating errors * Add a `validate` method to the `Signature` class, then leave each enum to implement it. This IMHO makes the validate_settings code more maintainable * Introduce usage of mocks to stub the crypto host callback functions About the latter one, the previous settings validation tests were simulating a failure of certificate verification. The failure was happening, but not because it was triggered by a mock. Since there was no mocking in place, the failure was caused by the waPC runtime. This is now addressed. Signed-off-by: Flavio Castelli <fcastelli@suse.com>
I just pushed some commit. This is the relevant one:
|
Once this gets merged I would like to open another PR to split the settings class into smaller files and maybe add more unit tests. Right now, that would feel too much to me. This PR is already big enough |
I broke the e2e tests... I'm looking into that |
Interesting... we do not run the e2e tests on PR, at least not with Rust policies. I caught the error locally, seems like something we have to change |
I've created kubewarden/rust-policy-template#27 to keep track of that |
I didn't break the e2e tests, I'm just running with an old version of kwctl that doesn't have the |
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.
This refactor is great, approving!
Allow multiple certificates to be specified when doing cert-based verification.
All the certs are put in
AND
condition.Fixes: #49