Tamper protected Endpoint integration removal #2747

aleksmaus · 2023-05-30T21:52:09Z

What does this PR do?

Implements the Tamper protected Endpoint integration removal
as described in the first part of https://github.com/elastic/ingest-dev/issues/1882

I discussed this approach with @blakerouse last week. The idea is to not introduce any agent state dependency on a particular order of the policy updates, the agent can still configure itself from any policy revision. The trust is maintained between Kibana and Endpoint through the signed policy payload, that's why the Endpoint would have to know more about the original structure of the policy, because that's what Kibana knows about and that's what it signs.
The surface of the change is limited to to the service runtime teardown implementation, which currently affects only Endpoint.

Here is what's done here:

Change the runtime manager Update signature in order to pass the component.Model instead, thus allowing to pass the optional Signed struct as discussed with @blakerouse.
Change the Teardown to pass Signed
Change the teardown sequence for the service runtime, which affects only Endpoint at the moment.
The teardown is now performed in two steps, first the new policy Signed object is passed down to Endpoint with the component update.

The signed data now has all the inputs included with some minimal set of properties that allows Endpoint
to validate the signature and unprotect itself if the there is no corresponding endpoint input in the policy.
Here is and example decoded from base64 encoded signed.data payload:

{
  "id": "6314e110-f8c2-11ed-ab07-d3e4064cb6ee",
  "agent": {
    "protection": {
      "enabled": false,
      "uninstall_token_hash": "tx/Vht5/TVZ7gE8LcuX0NiV9azGfpX3BkrF0/wRjFsY=",
      "signing_key": "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAENxwK08uYPM8D5yTmoA/emoxK5s5lQFyt/6E4LpMmpxbukMBN3/rNfjodisQBPyVQmE/ENuTbE42G5K8iXXvBRA=="
    }
  },
  "inputs": [
    {
      "id": "5be5a402-b3cb-456a-92e7-5ca5ac9204af",
      "name": "EP",
      "revision": 1,
      "type": "endpoint"
    }
  ]
}

The agent expects Endpoint to checkin after that or uses the teardown timeout, and proceeds with stopping and uninstalling the service.

This was tested against Kibana changes on @joeypoon branch
https://github.com/joeypoon/kibana/pull/new/feature/sign-unenroll

This will need to be tested and possibly adjusted once Endpoint implements this functionality, will work with @intxgo on validating it end-to-end once the Endpoint is ready.

Why is it important?

Completes the requirements for the tamper protection the 8.10

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in ./changelog/fragments using the changelog tool

Related issues

Relates https://github.com/elastic/ingest-dev/issues/1882

elasticmachine · 2023-05-31T14:23:32Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-06-07T16:05:44.494+0000
Duration: 21 min 22 sec

Test stats 🧪

Test	Results
Failed	0
Passed	6039
Skipped	19
Total	6058

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages.
run integration tests : Run the Elastic Agent Integration tests.
run end-to-end tests : Generate the packages and run the E2E Tests.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2023-05-31T15:08:19Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	98.667% (`74/75`)	👍
Files	68.992% (`178/258`)	👍 0.388
Classes	67.695% (`329/486`)	👍 0.273
Methods	53.801% (`1005/1868`)	👍 0.099
Lines	39.624% (`11515/29061`)	👍 0.048
Conditionals	100.0% (`0/0`)	💚

aleksmaus · 2023-05-31T15:23:17Z

hmm, the unit test failed in CI and works fine locally

FAIL: pkg/component/runtime TestManager_FakeInput_GoodUnitToBad (1.68s)

confirmed with the agent team that it's known to be flaky

cmacknz · 2023-06-02T19:45:07Z

@blakerouse is the integration & E2E test framework at a point where Aleks could write a test to verify this with a real instance of Elastic Defend?

blakerouse · 2023-06-05T13:04:10Z

@cmacknz Yes, minus actually having Elastic Defend in the Elastic Agent components. But we support adding a component out of band, so if it was fetched for the tests then yes it could be tested.

blakerouse

Really like how this turned out. Think we should get it fully tested with the integration testing framework before merging.

blakerouse · 2023-06-05T13:06:06Z

pkg/component/component.go

+
+var ErrNotFound = errors.New("not found")
+
+func SignedFromPolicy(policy map[string]interface{}) (*Signed, error) {


Please add comments to each exposed struct and function. In proper format of // {name} {description}

added comments

blakerouse · 2023-06-05T13:06:28Z

pkg/component/component.go

+func SignedFromPolicy(policy map[string]interface{}) (*Signed, error) {
+	v, ok := policy["signed"]
+	if !ok {
+		return nil, fmt.Errorf("policy is not signed: %w", ErrNotFound)


Should this maybe be a defined error? ErrNotSigned?

from general use case point of view, it's the same meaning, we care if we have data or not, same for the tests.
The fmt.Errorf adds additional context to ErrNotFound, which can be logged or propagated up.
It might be more convenient if we could wrap multierror probably like it's possible with Go 1.20
https://tip.golang.org/doc/go1.20#errors

aleksmaus · 2023-06-05T13:28:38Z

@cmacknz Yes, minus actually having Elastic Defend in the Elastic Agent components. But we support adding a component out of band, so if it was fetched for the tests then yes it could be tested.

Would have to wait until Endpoint is ready. There was an idea to merge the changes shortly after FF once we manually test it with Endpoint, so we can have the full release window (more time) to test with the real Endpoint and could add E2E right after. Given the scope of the change affects Endpoint/service runtime only.

…nd variables

mergify · 2023-06-08T17:19:28Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b feature/service_integration_removal_signed upstream/feature/service_integration_removal_signed
git merge upstream/main
git push upstream feature/service_integration_removal_signed

aleksmaus · 2023-06-12T18:56:37Z

Closing in favor of combined PR #2781

Tamper protected Endpoint integration removal

f4cf21e

aleksmaus added enhancement New feature or request backport-skip labels May 30, 2023

aleksmaus requested review from joshdover, cmacknz, blakerouse, joeypoon, intxgo and kevinlog May 30, 2023 21:52

aleksmaus requested a review from a team as a code owner May 30, 2023 21:52

mergify bot assigned aleksmaus May 30, 2023

pierrehilbert added the Team:Elastic-Agent Label for the Agent team label May 31, 2023

aleksmaus added 2 commits May 31, 2023 10:18

Make linter happy

392864f

Add changelog fragment

cfcc30d

aleksmaus added 2 commits May 31, 2023 10:29

Make linter happier

84b8b58

Fix unit tests

c1c9335

blakerouse reviewed Jun 5, 2023

View reviewed changes

aleksmaus mentioned this pull request Jun 5, 2023

Tamper protected Endpoint uninstall - combined PR #2781

Merged

6 tasks

Address code review, add missing comments for the public structures a…

a02f6da

…nd variables

aleksmaus closed this Jun 12, 2023

cmacknz mentioned this pull request Jun 13, 2023

Add automated test coverage for service based components #1357

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tamper protected Endpoint integration removal #2747

Tamper protected Endpoint integration removal #2747

aleksmaus commented May 30, 2023

elasticmachine commented May 31, 2023 •

edited

Loading

Build stats

Test stats 🧪

elasticmachine commented May 31, 2023 •

edited

Loading

aleksmaus commented May 31, 2023

cmacknz commented Jun 2, 2023

blakerouse commented Jun 5, 2023

blakerouse left a comment

blakerouse Jun 5, 2023

aleksmaus Jun 7, 2023

blakerouse Jun 5, 2023

aleksmaus Jun 7, 2023 •

edited

Loading

aleksmaus commented Jun 5, 2023 •

edited

Loading

mergify bot commented Jun 8, 2023

aleksmaus commented Jun 12, 2023


		var ErrNotFound = errors.New("not found")

		func SignedFromPolicy(policy map[string]interface{}) (*Signed, error) {

Tamper protected Endpoint integration removal #2747

Tamper protected Endpoint integration removal #2747

Conversation

aleksmaus commented May 30, 2023

What does this PR do?

Why is it important?

Checklist

Related issues

elasticmachine commented May 31, 2023 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

elasticmachine commented May 31, 2023 • edited Loading

🌐 Coverage report

aleksmaus commented May 31, 2023

cmacknz commented Jun 2, 2023

blakerouse commented Jun 5, 2023

blakerouse left a comment

Choose a reason for hiding this comment

blakerouse Jun 5, 2023

Choose a reason for hiding this comment

aleksmaus Jun 7, 2023

Choose a reason for hiding this comment

blakerouse Jun 5, 2023

Choose a reason for hiding this comment

aleksmaus Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

aleksmaus commented Jun 5, 2023 • edited Loading

mergify bot commented Jun 8, 2023

aleksmaus commented Jun 12, 2023

elasticmachine commented May 31, 2023 •

edited

Loading

elasticmachine commented May 31, 2023 •

edited

Loading

aleksmaus Jun 7, 2023 •

edited

Loading

aleksmaus commented Jun 5, 2023 •

edited

Loading