[aws] Declare required permissions with provider_permissions#19405
[aws] Declare required permissions with provider_permissions#19405jeniawhite wants to merge 4 commits into
Conversation
✅ Elastic Docs Style Checker (Vale)No issues found on modified lines! The Vale linter checks documentation changes against the Elastic Docs style guide. To use Vale locally or report issues, refer to Elastic style guide for Vale. |
This comment has been minimized.
This comment has been minimized.
andrewkroh
added
Team:Obs-InfraObs
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
andrewkroh
left a comment
andrewkroh
left a comment
There was a problem hiding this comment.
The permission sets look good to me, but I only did a few spot checks on the ones I know offhand. It would be good if the SMEs for the individual inputs used could take a closer look.
| @@ -22,6 +22,12 @@ conditions: | |||
| subscription: basic | |||
| kibana: | |||
| version: "^8.19.4 || ^9.2.1" | |||
There was a problem hiding this comment.
With the package-spec move 3.4->3.6 the package becomes incompatible with 8.19 as per format_version.md. I think we should bump the major version of the package so that we can continue maintenance of the 6.x version of the package be able to support format_version 3.4 and Elastic Stack 8.19 on a branch until Jan 2027 (~6 months). Or merge this after #19828 which does the major bump.
There was a problem hiding this comment.
Thanks for catching this. Went with the second option: this PR now depends on #19828 rather than bumping the major version independently.
Rebased on top of #19828's commit (cherry-picked onto current main for now, since that PR is still open) — package version is 7.1.0, format_version 3.6.4, kibana/agent ^9.4.0, consistent with the compatibility table in format_version.md.
Marked this PR as Draft and blocked on #19828 in the description. The diff currently includes #19828's changes as a side effect of the cherry-pick — once that PR merges, I'll rebase this branch onto main and that content will drop out of the diff on its own.
This comment has been minimized.
This comment has been minimized.
|
👀 I have started reviewing the PR |
TL;DR
Remediation
Investigation detailsRoot CauseThis is a configuration/spec-validation failure, not a runtime test assertion failure. The PR changes
Evidence
The log also reports the same Verification
Follow-upThis is different from the earlier detective comments about the CI toolchain missing spec What is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
![vera-review-bot[bot]](https://avatars.githubusercontent.com/in/3297022?s=60&v=4){kind=small}
| permissions: | ||
| - name: ce:GetCostAndUsage | ||
| description: Retrieve cost and usage amounts from AWS Cost Explorer. | ||
| - name: cur:DescribeReportDefinitions |
There was a problem hiding this comment.
🟡 MEDIUM data_stream/billing/manifest.yml:9
Billing declares unused cur and budgets permissions
This PR's stated goal is to scope AWS permissions with least privilege, but the billing data stream declares two permissions for AWS services it does not call: cur:DescribeReportDefinitions (Cost and Usage Reports) and budgets:ViewBudget (Budgets). The billing data stream collects only from the Cost Explorer API and CloudWatch — docs/billing.md states "Metrics are gathered with the AWS Cost Explorer API", and the block's own description (line 5) says only "Read cost data from Cost Explorer alongside the CloudWatch billing metrics." Neither the Cost and Usage Report API nor the Budgets API is invoked by this data stream. Declaring them over-grants IAM scope, which contradicts the least-privilege intent and would lead operators to attach permissions the integration never uses.
Recommendation:
Drop the two unused permissions, keeping only the Cost Explorer call the billing data stream actually makes (CloudWatch is already covered at the policy-template input level):
provider_permissions:
- provider: aws
description: Read cost data from Cost Explorer alongside the CloudWatch billing metrics.
permissions:
- name: ce:GetCostAndUsage
description: Retrieve cost and usage amounts from AWS Cost Explorer.🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
…permissions with matrix fallback (#274425) ## Summary Open question(s): - [ ] I spoke a bit with @juliaElastic on this but want to discuss more with the team: This implementation creates a server in the ingest_hub plugin and a new endpoint, which is probably fine. However, Julia raised a good point that we could probably instead have added the endpoint directly to the Fleet plugin and had the benefit of the existing FTR infrastructure. Another benefit of adding it to Fleet would be if any other plugin would need to consume this endpoint (or any future additions), but its not clear is this would be a real use case. However, on the contrary keeping it in `ingest_hub` is more self-contained whereas moving it to fleet means it becomes more primitive - and ingest_hub still owns some of the resolution logic so its split into two places, endpoint in fleet and logic in `ingest_hub`. --- Closes elastic/ingest-dev#8084 PR #7927 shipped AWS onboarding V1 with IAM permissions hardcoded in a client-side service matrix (`aws_provider_permissions.ts`). Now that [package-spec PR #1180](elastic/package-spec#1180) (spec 3.7.0) adds a `provider_permissions` field to package manifests, this PR wires Kibana up to read permissions from the installed package when declared and fall back to the hardcoded matrix for packages not yet upgraded. To do this cleanly on the server, `ingest_hub` needed a server for the first time (`"server": false` → `"server": true`). A new internal endpoint `GET /internal/onboarding/iam_permissions?services=a,b,c` fetches `PackageInfo` via Fleet's `packageService`, maps `provider_permissions` across all four manifest levels (package → policy_template → input → data_stream), and returns `{ merged, byService }`. The UI's connect step now calls this endpoint instead of reading the matrix directly; no hardcoded IAM logic remains on the client. Fleet type changes (`RegistryProviderPermissions`, enum extensions, `parse.ts` allowlist) are scoped to the shared `fleet/common` types so any other consumer can pick up the field without further changes. ### Testing 1. Boot Kibana against an Elasticsearch cluster. 2. Navigate to **Ingest Hub → AWS onboarding** and advance to the "Connect" step with one or more services selected. 3. Confirm the "Required IAM permissions" JSON block appears and reflects the correct actions for each selected service. 4. Toggle the per-service dropdown (e.g. "All integrations" → "CloudTrail") and verify the policy switches without making a second network request (check the Network tab). 5. To verify the endpoint directly: ``` curl -s -u elastic:<password> \ "http://localhost:5601/internal/onboarding/iam_permissions?services=cloudtrail,vpcflow" \ | jq '{merged_action_count: (.merged.Statement[0].Action | length), services: (.byService | keys)}' ``` Expected: `merged` is a deduped union of both services' actions; `byService` has a key per requested service. 6. Install a package that declares `provider_permissions` (once [integrations#19405](elastic/integrations#19405) lands) and confirm the endpoint returns manifest values rather than the matrix fallback. ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks - **Fleet type surface** (low): adding `provider_permissions` fields to the Fleet enums and interfaces is additive. Existing consumers are unaffected; the archive parser's strict allowlists are explicitly extended. - **New ingest_hub server** (low): `ingest_hub` previously had no server-side code. The new server only registers a single internal read-only endpoint — no saved objects, no config keys, no tasks. - **Matrix fallback dependency** (low): if Fleet's `getLatestPackageInfo` throws (package not installed, registry unreachable), the endpoint silently falls back to the hardcoded matrix and still returns a valid response. Degradation is graceful. - [ ] [See some risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Julia Bardi <90178898+juliaElastic@users.noreply.github.com>
…permissions with matrix fallback (elastic#274425) ## Summary Open question(s): - [ ] I spoke a bit with @juliaElastic on this but want to discuss more with the team: This implementation creates a server in the ingest_hub plugin and a new endpoint, which is probably fine. However, Julia raised a good point that we could probably instead have added the endpoint directly to the Fleet plugin and had the benefit of the existing FTR infrastructure. Another benefit of adding it to Fleet would be if any other plugin would need to consume this endpoint (or any future additions), but its not clear is this would be a real use case. However, on the contrary keeping it in `ingest_hub` is more self-contained whereas moving it to fleet means it becomes more primitive - and ingest_hub still owns some of the resolution logic so its split into two places, endpoint in fleet and logic in `ingest_hub`. --- Closes elastic/ingest-dev#8084 PR elastic#7927 shipped AWS onboarding V1 with IAM permissions hardcoded in a client-side service matrix (`aws_provider_permissions.ts`). Now that [package-spec PR elastic#1180](elastic/package-spec#1180) (spec 3.7.0) adds a `provider_permissions` field to package manifests, this PR wires Kibana up to read permissions from the installed package when declared and fall back to the hardcoded matrix for packages not yet upgraded. To do this cleanly on the server, `ingest_hub` needed a server for the first time (`"server": false` → `"server": true`). A new internal endpoint `GET /internal/onboarding/iam_permissions?services=a,b,c` fetches `PackageInfo` via Fleet's `packageService`, maps `provider_permissions` across all four manifest levels (package → policy_template → input → data_stream), and returns `{ merged, byService }`. The UI's connect step now calls this endpoint instead of reading the matrix directly; no hardcoded IAM logic remains on the client. Fleet type changes (`RegistryProviderPermissions`, enum extensions, `parse.ts` allowlist) are scoped to the shared `fleet/common` types so any other consumer can pick up the field without further changes. ### Testing 1. Boot Kibana against an Elasticsearch cluster. 2. Navigate to **Ingest Hub → AWS onboarding** and advance to the "Connect" step with one or more services selected. 3. Confirm the "Required IAM permissions" JSON block appears and reflects the correct actions for each selected service. 4. Toggle the per-service dropdown (e.g. "All integrations" → "CloudTrail") and verify the policy switches without making a second network request (check the Network tab). 5. To verify the endpoint directly: ``` curl -s -u elastic:<password> \ "http://localhost:5601/internal/onboarding/iam_permissions?services=cloudtrail,vpcflow" \ | jq '{merged_action_count: (.merged.Statement[0].Action | length), services: (.byService | keys)}' ``` Expected: `merged` is a deduped union of both services' actions; `byService` has a key per requested service. 6. Install a package that declares `provider_permissions` (once [integrations#19405](elastic/integrations#19405) lands) and confirm the endpoint returns manifest values rather than the matrix fallback. ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks - **Fleet type surface** (low): adding `provider_permissions` fields to the Fleet enums and interfaces is additive. Existing consumers are unaffected; the archive parser's strict allowlists are explicitly extended. - **New ingest_hub server** (low): `ingest_hub` previously had no server-side code. The new server only registers a single internal read-only endpoint — no saved objects, no config keys, no tasks. - **Matrix fallback dependency** (low): if Fleet's `getLatestPackageInfo` throws (package not installed, registry unreachable), the endpoint silently falls back to the hardcoded matrix and still returns a valid response. Degradation is graceful. - [ ] [See some risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Julia Bardi <90178898+juliaElastic@users.noreply.github.com>
…ration Bump format_version to 3.6.0 and version to 7.0.0. Reorganize AWS credential configuration into a `Setup Access` var_groups selector with six options: Identity Federation, Direct Access Keys, Temporary Access Keys, Assume Role, Assume Role with External ID, and Shared Credentials. Key changes: - format_version: 3.4.0 → 3.6.0 - version: 6.20.3 → 7.0.0 - kibana.version: "^8.19.4 || ^9.2.1" → "^9.4.0" - agent.version: "^9.4.0" - var_groups: credential_type selector with 6 options - external_id is now secret: true - New vars: assume_role_duration, assume_role_expiry_window, supports_identity_federation - hide_in_var_group_options for 13 inputs across services that don't support Identity Federation - GuardDuty httpjson stream: switch to auth.aws: block and add Identity Federation policy tests - Add conditions.agent.version: ^9.4.0 because guardduty now requires it. Source: #19278 (Omolola-Akinleye/integrations)
…issions from billing data stream The billing data stream only calls the Cost Explorer API (ce:GetCostAndUsage); Cost and Usage Reports and Budgets are never invoked.
c1300d1 to
086eb1d
Compare
|
Changelog link mismatch — expected
Tip If expected, add the |
💔 Build Failed
Failed CI StepsHistory
|
🚀 Benchmarks reportPackage
|
| Data stream | Previous EPS | New EPS | Diff (%) | Result |
|---|---|---|---|---|
guardduty |
1715.27 | 1390.82 | -324.45 (-18.92%) | 💔 |
securityhub_findings |
1976.28 | 1628.66 | -347.62 (-17.59%) | 💔 |
To see the full report comment with /test benchmark fullreport
| changes: | ||
| - description: Reorganize AWS credentials configuration into a `Setup Access` selector with Identity Federation, Direct Access Keys, Temporary Access Keys, Assume Role, Assume Role with External ID, and Shared Credentials options. Existing access keys, role ARN, and shared credential settings continue to function and are auto-mapped to the corresponding option. | ||
| type: enhancement | ||
| link: https://github.com/elastic/integrations/pull/19828 |
There was a problem hiding this comment.
Severity: 🟡 Medium confidence: high path: packages/aws/changelog.yml:11
Changelog links a different PR number
Details
This changelog entry's link: points at pull/19828, but it was added in PR #19405. It is likely a leftover template placeholder or a copy from another PR.
Recommendation:
Point the changelog link at this PR:
link: https://github.com/elastic/integrations/pull/19405🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| link: https://github.com/elastic/integrations/pull/19828 | ||
| - description: Switch the GuardDuty httpjson stream from manual SigV4 HMAC signing to the native `auth.aws` block. | ||
| type: enhancement | ||
| link: https://github.com/elastic/integrations/pull/19828 |
There was a problem hiding this comment.
Severity: 🟡 Medium confidence: high path: packages/aws/changelog.yml:14
Changelog links a different PR number
Details
This changelog entry's link: points at pull/19828, but it was added in PR #19405. It is likely a leftover template placeholder or a copy from another PR.
Recommendation:
Point the changelog link at this PR:
link: https://github.com/elastic/integrations/pull/19405🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| link: https://github.com/elastic/integrations/pull/19828 | ||
| - description: Store the `external_id` variable as a secret. | ||
| type: breaking-change | ||
| link: https://github.com/elastic/integrations/pull/19828 |
There was a problem hiding this comment.
Severity: 🟡 Medium confidence: high path: packages/aws/changelog.yml:17
Changelog links a different PR number
Details
This changelog entry's link: points at pull/19828, but it was added in PR #19405. It is likely a leftover template placeholder or a copy from another PR.
Recommendation:
Point the changelog link at this PR:
link: https://github.com/elastic/integrations/pull/19405🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| link: https://github.com/elastic/integrations/pull/19828 | ||
| - description: Add `assume_role_duration`, `assume_role_expiry_window`, and `supports_identity_federation` variables. | ||
| type: enhancement | ||
| link: https://github.com/elastic/integrations/pull/19828 |
There was a problem hiding this comment.
Severity: 🟡 Medium confidence: high path: packages/aws/changelog.yml:20
Changelog links a different PR number
Details
This changelog entry's link: points at pull/19828, but it was added in PR #19405. It is likely a leftover template placeholder or a copy from another PR.
Recommendation:
Point the changelog link at this PR:
link: https://github.com/elastic/integrations/pull/19405🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| link: https://github.com/elastic/integrations/pull/19828 | ||
| - description: Require Kibana and Elastic Agent ^9.4.0 (drop support for Kibana 8.x, Kibana 9.x below 9.4.0, and Elastic Agent below 9.4.0). The var_groups manifest feature and `auth.aws` runtime require this minimum. | ||
| type: breaking-change | ||
| link: https://github.com/elastic/integrations/pull/19828 |
There was a problem hiding this comment.
Severity: 🟡 Medium confidence: high path: packages/aws/changelog.yml:23
Changelog links a different PR number
Details
This changelog entry's link: points at pull/19828, but it was added in PR #19405. It is likely a leftover template placeholder or a copy from another PR.
Recommendation:
Point the changelog link at this PR:
link: https://github.com/elastic/integrations/pull/19405🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
Review summary
🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
|
Cross-checked every inline action against the provider_permissions declarations in elastic/integrations#19405 (the AWS package's per-datastream permission manifest) and added the missing actions the agentless data streams require at runtime: - ec2:DescribeInstanceStatus (ec2_metrics) - ecs:DescribeClusters (ecs_metrics) - elasticloadbalancing:DescribeTargetHealth (elb_metrics) - lambda:GetFunction (lambda) - rds:DescribeDBClusters (rds) - health:DescribeAffectedEntities (awshealth) - securityhub:BatchGetSecurityControls, GetInsightResults (securityhub_findings_full_posture, securityhub_insights) securityhub:ListInsights, declared by #19405, is NOT added: cfn-lint confirms no such IAM action exists — listing insights is securityhub:GetInsights, which was already granted. Also corrects the Config grant: the aws.config data stream polls rule compliance (DescribeConfigRules, DescribeComplianceByConfigRule, GetComplianceDetailsByConfigRule), not resource inventory; the inventory-style Config reads moved to the SecurityPosturePolicy with the rest of the Asset Inventory permission set. Actions not declared by #19405 are retained when another primary source documents them (e.g. iam:ListAccountAliases in the package README); services whose policy templates are not agentless-enabled (apigateway, natgateway, vpn, emr, kafka, kinesis, redshift, s3_storage_lens) stay out of scope. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Proposed commit message
Adopt the new
provider_permissionsfield (package-spec,format_version3.6.4) so the AWS integration declares the IAM permissions it actually needs in a machine-readable way, instead of leaving them in prose docs and expecting users to attach a broad managed policy (e.g.SecurityAudit/ReadOnlyAccess).awsintegrationformat_version3.4.0 → 3.6.4 and packageversionto7.1.0(on top of [aws] Add var_groups for credential type selection with Identity Federation #19828's7.0.0major bump, which raises the Kibana/Agent requirement to^9.4.0— required forformat_version3.6.4); add achangelog.ymlentry.sts:GetCallerIdentityonly; the single call every credentialed AWS collector makes.aws/metrics→ec2:DescribeRegions,cloudwatch:ListMetrics,cloudwatch:GetMetricData,tag:GetResources;aws-s3→s3:GetObject/s3:ListBucket+ SQS notification actions;aws-cloudwatch→logs:DescribeLogGroups/logs:FilterLogEvents;cel/httpjson→ the AWS Config / Inspector / GuardDuty API read calls.ec2:DescribeInstancesonec2_metrics,rds:Describe*onrds, and the disjoint Security HubGetFindingsvsGetInsights/ListInsights/GetInsightResultsreads).Permission requirements currently exist only in documentation, so they drift over time and cannot be consumed by tooling. Declaring them with
provider_permissionsversions them alongside the package and lets tooling generate appropriately scoped infrastructure-as-code rather than a static kitchen-sink policy.The placement follows the spec's accumulate-and-deduplicate semantics together with least privilege: each permission is declared exactly once, at the broadest layer whose scope matches everything that needs it. This keeps minimal grants when only some data streams are enabled. Concretely:
cloudwatch:*(those live on theaws/metricsinput);httpjsoninput rather than theguarddutydata stream;No data streams, ingest pipelines, dashboards, or collection behavior change; this is metadata only.
Checklist
changelog.ymlfile.Author's Checklist
provider_permissionsare placed at the correct layer (package / input / data_stream) with no cross-layer duplication.How to test this PR locally
Related issues
Screenshots