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.

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

Indexing permissions as part of the Elastic Agent policy #169

Merged
merged 3 commits into from
Mar 29, 2021

Conversation

aleksmaus
Copy link
Member

What does this PR do?

Implements the Indexing permissions as part of the Elastic Agent policy
#101

  1. Adds a new field policy_output_permissions_hash into the Agent document that stores the hash of output_permissions from the policy.
  2. Retrieves the policy from .fleet-policies upon the agent enrollment and uses the output_permissions default namespace in order to create the role descriptors for the output API key generation.
  3. Checks the output_permissions upon every checking. If the hash doesn't match with the stored in the agent document the new key is generated.

This PR:

  1. doesn't invalidate the old API key see the comment for the next iteration [Discuss] Indexing permissions as part of the Elastic Agent policy #101 (comment)
  2. is not fully validated end-to-end with kibana for the output_permissions changes, pending on further kibana changes.

Should only be merged after the corresponding kibana changes:
elastic/kibana#94591

Why is it important?

Addresses: #101

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

Related issues

Screenshots

Screen Shot 2021-03-24 at 6 40 40 PM

@aleksmaus aleksmaus added the enhancement New feature or request label Mar 25, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Mar 25, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #169 updated

  • Start Time: 2021-03-29T13:25:33.463+0000

  • Duration: 4 min 11 sec

  • Commit: f625155

Test stats 🧪

Test Results
Failed 0
Passed 66
Skipped 0
Total 66

Trends 🧪

Image of Build Times

Image of Tests

}

m := make(smap.Map)
m["index"] = idx
Copy link
Member

Choose a reason for hiding this comment

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

I hope we can get rid of this bit as it is planned that Kibana will add the index part to the output so we don't need to do the additional processing here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, Alejandro reached out about the upcoming change. Might add more things here then.

Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

Overall this change looks good. See my inline comment about fetching the policy from elasticsearch.

@@ -79,3 +90,18 @@ func CreatePolicy(ctx context.Context, bulker bulk.Bulk, policy model.Policy, op
}
return bulker.Create(ctx, o.indexName, "", data)
}

// FindPolicyByID find policy by ID
func FindPolicyByID(ctx context.Context, bulker bulk.Bulk, policyID string) (policy model.Policy, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The policy.Monitor keeps the latest revision of each policy in memory. It would be better to fetch the policy from there instead of going to elasticsearch, as fleet-server already has it cached in memory.

@aleksmaus
Copy link
Member Author

aleksmaus commented Mar 25, 2021

Removed the output key generation upon enrollment. Now this is driven by the policy update pushes only, one code path.
Updated to the latest kibana changes, removed output permissions to the role descriptors transformation.

Screen Shot 2021-03-25 at 9 36 11 AM

@mergify
Copy link
Contributor

mergify bot commented Mar 29, 2021

This pull request is now in conflicts. Could you fix it @aleksmaus? 🙏
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/indexing_permissions upstream/feature/indexing_permissions
git merge upstream/master
git push upstream feature/indexing_permissions

@aleksmaus aleksmaus merged commit 81c6d16 into elastic:master Mar 29, 2021
@ruflin
Copy link
Member

ruflin commented Mar 29, 2021

@aleksmaus I see this got merged but elastic/kibana#94591 is still open. How is the current behaviour?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants