-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Fleet][Tests] Package install signature verification API tests #136947
Conversation
abd4ad2
to
d5dc7d6
Compare
@@ -33,7 +33,7 @@ const INGEST_API_PACKAGE_POLICIES = `${INGEST_API_ROOT}/package_policies`; | |||
const INGEST_API_PACKAGE_POLICIES_DELETE = `${INGEST_API_PACKAGE_POLICIES}/delete`; | |||
const INGEST_API_EPM_PACKAGES = `${INGEST_API_ROOT}/epm/packages`; | |||
|
|||
const SECURITY_PACKAGES_ROUTE = `${INGEST_API_EPM_PACKAGES}?category=security`; | |||
const SECURITY_PACKAGES_ROUTE = `${INGEST_API_EPM_PACKAGES}?category=security&experimental=true`; |
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.
I was having the issue where 8.4.0-dev of the endpoint package was being installed, but this function was returning 8.3.0 as the latest version, adding ?experimental=true fixes this, and it made sense that we would always want to test with the latest package, but do we only ever want to install GA packages for these tests?
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.
cc @paul-tavares is installing experimental (prereleases) endpoint package a good idea for these tests?
somewhat unlikely that a prerelease package is going to break tests since it's mostly mappings, but transform stuff is there too.
And the prereleases are going to be more unstable now with automatic deploys.
on the face of it it doesn't seem great, but I'm having trouble thinking through the implications of what this affects
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.
@pzl ,
I think running against the latest package here is ok. Finding and addressing failures as soon as possible rather than to wait until a release package is available would be best. I'm actually a bit surprised - have we been testing against an older package all along?
@hop-dev ,
What does experimental
do from a fleet API standpoint?
Our tests all run with a custom package registry running in docker (we get the config for the FTR tests from here, which uses the Docker image defined via the fleet API integration config. Just want to ensure that your change here will not cause the tests to become flaky, especially if somehow this flag is causing the API to reach out to the "real" registry (we have had problems in the past with CI test runs making external network calls to the real registry (especially network timeout or connection issues)).
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 @paul-tavares, adding experimental
means EPR returns the latest version of a package it has whether it is GA or not, e.g it will return 8.4.0-dev instead of 8.3.0 of the endpoint package. It doesnt change where we go to get the packages.
As part of this PR I have moved to v2 of the EPR docker image which contains dev packages which it seems the previous v1 docker image we were using didn't. When the security tests start we install the latest version of the package here
the previous issue was that we were installing 8.4.0-dev but then the EPR call was returning 8.3.0 as the latest version, adding experimental solved this.
If it is too risky to use the latest version I could look at a way of only specifying the latest GA release of the package on startup
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.
Just a note, the version of the package would only change if we update the docker image so I wouldnt expect this to add flakiness. Only the PR changing the image would fail
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.
^ got it. So this is still pulling a particular docker image to test from, and the query here is being sent to that point-in-time image. Sounds like there will be at least some minimal checking that it isn't a completely broken package during a PR that would update the image. Seems ok to me
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 @hop-dev . It seems ok with me as well, so I'll 👍 it.
7ce8835
to
fd18a23
Compare
@@ -54,6 +54,7 @@ export default function (providerContext: FtrProviderContext) { | |||
}, | |||
id: `${getSyntheticsPolicy(agentFullPolicy)?.streams?.[0]?.id}`, | |||
name, | |||
origin: 'ui', |
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.
I am not sure why this value appeared when moving to the v2 registry but I can see that there are other tests which expect the origin field to be set so it seemed acceptable to add.
Pinging @elastic/fleet (Team:Fleet) |
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.
APM changes LGTM
Pinging @elastic/apm-ui (Team:apm) |
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, great work!
e097159
to
ac5637e
Compare
@elasticmachine merge upstream |
186ae35
to
8901c07
Compare
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @hop-dev |
💔 All backports failed
Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…tic#136947) * add valid signature test package * add test signatures and readme for signature generation * mount zip packages as part of tests * amend README * Verified package test working * Rename valid to verified * Add test for unverified content * add test for package verified with wrong key * Check error types in 400 response * Check saved object keys as part of tests * Remove wrong_ keys * use release docker image * update package path for v2 registry * force install endpoint package * fix package policy upgrade on setup test * formatting * move back to production registry * Update all registry configs to use new package directory * use specific docker image not tag * fix agent policy tests * Get latest experimental endpoint version * skip impossible fleet test * update synthetics to use same registry image as fleet * fix telemetry tests * remove experimental flag from test config * add force install confirm to synthetics tests * add origin to expected policy data * add test subj to force install modal * Install latest fleet_server package not fixed version * install latest system pkg * fix types * fix deprecated API calls (cherry picked from commit 9f8a2c6) # Conflicts: # x-pack/test/fleet_api_integration/config.ts # x-pack/test/functional_synthetics/config.js
) (#138027) * add valid signature test package * add test signatures and readme for signature generation * mount zip packages as part of tests * amend README * Verified package test working * Rename valid to verified * Add test for unverified content * add test for package verified with wrong key * Check error types in 400 response * Check saved object keys as part of tests * Remove wrong_ keys * use release docker image * update package path for v2 registry * force install endpoint package * fix package policy upgrade on setup test * formatting * move back to production registry * Update all registry configs to use new package directory * use specific docker image not tag * fix agent policy tests * Get latest experimental endpoint version * skip impossible fleet test * update synthetics to use same registry image as fleet * fix telemetry tests * remove experimental flag from test config * add force install confirm to synthetics tests * add origin to expected policy data * add test subj to force install modal * Install latest fleet_server package not fixed version * install latest system pkg * fix types * fix deprecated API calls (cherry picked from commit 9f8a2c6) # Conflicts: # x-pack/test/fleet_api_integration/config.ts # x-pack/test/functional_synthetics/config.js
Non Fleet team reviewers 👋 : I have had to change your package registry config to point to a new directory. In Fleet we have moved to using the v2 registry docker image for integration testing which stores packages in this different directory 👍
Summary
Part of #133822.
This PR isn't as big as it seems! :) Files under fixtures/package_verification/packages/src are just test packages.
Add tests for package verification on the install package route. As part of these tests I have had to include a public and private key pair for testing purposes, the private key is needed to generate the signed zip files.
Fleet Devs: This means that in integration tests any non-test fixture package from the registry will fail signature verification and must be force installed. In general we should be using test packages for our integration tests to avoid this, bu I understand there are situations where using a real package can't be avoided.
This PR was starting to get a bit big, things I intend to do in further PRs: