Skip to content

feat(test): add signatures to oci integration tests [NR-520796]#2212

Draft
sigilioso wants to merge 1 commit intofreeze-developfrom
feat/signed-packages-test
Draft

feat(test): add signatures to oci integration tests [NR-520796]#2212
sigilioso wants to merge 1 commit intofreeze-developfrom
feat/signed-packages-test

Conversation

@sigilioso
Copy link
Contributor

🚧 WIP

Signatures aren't in place yet ❗

Summary

This PR adds signatures to OCI integration tests.

  • Updates the current integration test: now the artifacts are signed and the corresponding public_key is included in the agent-type. Therefore signature verification should take place.
  • Adds an additional integration test to check that the verification fails as expected: the agent-type includes the public key but the package isn't signed, therefore the corresponding remote configuration should fail.


retry(60, Duration::from_secs(1), || {
// Remote config status should fail because the package is unsigned
check_latest_remote_config_status_is_expected(
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this is how we generally test this cases , but would make sense to specify some expected "partial" log ? i guess that in the future when the error get's typed we will be able to check the types 🤞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I don't like that much that we could have a failed remote config because other reasons. I might be able to check the error message in the remote-config-status. I'll look into it!

// TODO: this should pass with signatures in place
#[test]
#[ignore = "needs oci registry (use *with_oci_registry suffix), needs elevated privileges on Windows"]
fn test_unsigned_artifact_makes_remote_config_fail_with_oci_registry() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you thought about having a test where the verification is disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I'm not sure if they should be at this level (there are too many things moving around). Maybe we can assure that the enable/disable behavior is well covered with unit tests (or integration tests using components).

It is a good point anyway, we can see if we are happy covering it with unit tests with the implementation in place. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments