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.

Sign up for GitHub

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

Jump to bottom

feat: cert verification, support multiple certs #50

Merged
merged 3 commits into from
Dec 1, 2022
Merged

feat: cert verification, support multiple certs #50

merged 3 commits into from
Dec 1, 2022

Conversation

flavio
Copy link
Member

@flavio flavio commented Dec 1, 2022

Allow multiple certificates to be specified when doing cert-based verification.

All the certs are put in AND condition.

Fixes: #49

@flavio flavio requested a review from a team as a code owner December 1, 2022 07:55
@flavio
feat: cert verification, support multiple certs
8cde3db 
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>
@flavio flavio force-pushed the support-multiple-certs-at-verification-time branch from 454f875 to 8cde3db Compare December 1, 2022 07:56
viccuad
viccuad approved these changes Dec 1, 2022
View reviewed changes
Copy link
Member

@viccuad viccuad left a 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.

@flavio
refactor: rename mock namespace
a82205e 
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>
@flavio
refactor: improve settings validation
f7eb1e9 
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>
@flavio
Copy link
Member Author

flavio commented Dec 1, 2022 •
edited
Loading

I just pushed some commit.

This is the relevant one:

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 settings 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.

@flavio
Copy link
Member Author

flavio commented Dec 1, 2022

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

@flavio
Copy link
Member Author

flavio commented Dec 1, 2022

I broke the e2e tests... I'm looking into that

@flavio
Copy link
Member Author

flavio commented Dec 1, 2022

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

@flavio
Copy link
Member Author

flavio commented Dec 1, 2022

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

@flavio
Copy link
Member Author

flavio commented Dec 1, 2022

I didn't break the e2e tests, I'm just running with an old version of kwctl that doesn't have the crypto namespace yet 😅

viccuad
viccuad approved these changes Dec 1, 2022
View reviewed changes
Copy link
Member

@viccuad viccuad left a 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!

@flavio flavio merged commit 155311d into main Dec 1, 2022
@flavio flavio deleted the support-multiple-certs-at-verification-time branch December 1, 2022 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvanz jvanz jvanz approved these changes

@viccuad viccuad viccuad approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Certificate verification: allow multiple certificates to be added to the same signature
3 participants
@flavio @jvanz @viccuad