Skip to content

[Fleet] Implement secrets in APM package policy editor #225956

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

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

criamico
Copy link
Contributor

@criamico criamico commented Jul 1, 2025

Closes #224063

Summary

Implement secrets in APM policy editor. This implementation mirrors what we currently have in Integrations' packagePolicyEditor.
❗ In order to work as expected, a new version of APM where secret_token has secret: true needs to be released

In Fleet:

  • Refactored package_policy_input_var_field in Fleet so the components are now decoupled and SecretInputField can be exported to be used by other plugins
  • Exported a new function useSecretsStorage that checks if the requirements to enable secrets are available: Fleet server needs to be available and to have at least version 8.0.0 - below that version fleet server doesn't have the capability to read secrets in package policies.

In APM:

  • Marked the secret_token key as secret: true and added a new type 'secret' to be able to correctly display any secret fields. I also had to pass down an additional property "packageInfo" to be able to check that an input var is marked as "secret" in the package manifest. This aligns with the general functionality in Integrations
  • Imported Fleet component above to render the secret components
  • Saving the var as secret works out of the box since the form "submit" is uses Fleet's apis

UI

With secret: true enabled and the correct version of Fleet server installed:
Screenshot 2025-07-01 at 10 20 19
Screenshot 2025-07-01 at 10 20 14

Fallback UI - displays a password field
Screenshot 2025-07-02 at 12 41 10

View policy:
Screenshot 2025-07-01 at 15 49 36

Testing

Checklist

@criamico criamico self-assigned this Jul 1, 2025
@@ -57,7 +57,7 @@ export function getAgentAuthorizationSettings(): SettingsRow[] {
],
},
{
type: 'text',
type: 'secret',
Copy link
Contributor Author

@criamico criamico Jul 2, 2025

Choose a reason for hiding this comment

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

Changing this value immediately has the effect of using the "fallback" component that renders a EUiFieldPassword, but it doesn't affect the functionality - secret_token keeps being saved as plain text.

I think that this is acceptable but let me know if it's better to change it later when the new APM version with secret: true will be released.

@criamico criamico added Team:Fleet Team label for Observability Data Collection Fleet team release_note:skip Skip the PR/issue when compiling release notes v9.2.0 backport:skip This commit does not require backporting labels Jul 2, 2025
@criamico
Copy link
Contributor Author

criamico commented Jul 2, 2025

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #104 / alerting api integration security and spaces enabled - Group 2 Alerts update space_1_all at space1 should handle updates for a long running alert type without failing the underlying tasks due to invalidated ApiKey

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
fleet 1366 1369 +3

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
fleet 1431 1426 -5

Any counts in public APIs

Total count of every any typed public API. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats any for more detailed information.

id before after diff
fleet 5 3 -2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
apm 2.6MB 2.6MB +1.6KB
fleet 2.1MB 2.1MB -3.0KB
total -1.3KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
fleet 97 99 +2

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
fleet 166.7KB 169.7KB +3.1KB

History

cc @criamico

@criamico criamico marked this pull request as ready for review July 2, 2025 15:31
@criamico criamico requested review from a team as code owners July 2, 2025 15:31
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@criamico
Copy link
Contributor Author

criamico commented Jul 3, 2025

@elasticmachine merge upstream

@botelastic botelastic bot added the Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team label Jul 3, 2025
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

@criamico
Copy link
Contributor Author

criamico commented Jul 3, 2025

When the secret token is saved as secret, it breaks the ESS onboarding. What's the preferred way of addressing it? Replacing it with a "create api key" button like in serverless?

Current onboarding:
Screenshot 2025-07-03 at 11 26 50

Comment on lines 196 to 204
if (
packageInfo &&
packageInfo.policy_templates &&
packageInfo.policy_templates.length > 0 &&
'inputs' in packageInfo.policy_templates[0] &&
Array.isArray(packageInfo.policy_templates[0].inputs) &&
packageInfo.policy_templates[0].inputs.length > 0 &&
packageInfo.policy_templates[0].inputs[0].vars
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this work instead?

Suggested change
if (
packageInfo &&
packageInfo.policy_templates &&
packageInfo.policy_templates.length > 0 &&
'inputs' in packageInfo.policy_templates[0] &&
Array.isArray(packageInfo.policy_templates[0].inputs) &&
packageInfo.policy_templates[0].inputs.length > 0 &&
packageInfo.policy_templates[0].inputs[0].vars
) {
if (
packageInfo?.policy_templates?.[0]?.inputs?.[0]?.vars
) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it works per se if I shorten it but the linter is not happy. This looks a bit funky and I don't like it either, but the alternative is

  if (
    packageInfo &&
    packageInfo.policy_templates &&
    packageInfo.policy_templates.length > 0 &&
    packageInfo.policy_templates[0].inputs.length > 0 &&
    packageInfo.policy_templates[0].inputs[0].vars
  )

and some ts-ignore lines here and there. The reason is that inputs exists only in one of the types of this union type:
https://github.com/criamico/kibana/blob/a60bde8d65ce41f14aced142fb273457ef928e30/x-pack/platform/plugins/shared/fleet/common/types/models/epm.ts#L273-L275

so I had to add the type guard as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could break the condition? Just brainstorming ideas to try to simplify the condition.

const { policy_templates = [] } = packageInfo || {};
const policyTemplate = policy_templates[0];
const inputs = 'inputs' in policy_templates[0] ? policy_templates[0].inputs : undefined;

if (inputs && Array.isArray(inputs) && inputs.length > 0 && inputs[0].vars) {
}

I don't want to block the PR because of this though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I broke down the condition as suggested :)

@simitt
Copy link
Contributor

simitt commented Jul 3, 2025

IMO showing a Create API Key button and nudging users to API Key makes sense for the onboarding flow.
However, there also needs to be a way to see the secret-token, otherwise they cannot configure it - how can that be retrieved after this change? Is this by querying directly from Elasticsearch?
cc @akhileshpok as the PM for APM Server for further input.

@criamico
Copy link
Contributor Author

criamico commented Jul 3, 2025

@simitt

how can that be retrieved after this change? Is this by querying directly from Elasticsearch?

I need to check if there's a way to retrieve the plain text value. Currently we don't do it anywhere in Fleet, we just send the value to the policy and fleet server will read it. The value that is returned in the package policy is like follows, we just see an id that is then internally used to retrieve the value:

 "secret_token": {
                        "value": {
                            "id": "dMlZy5cB3Iz3RJj917hN",
                            "isSecretRef": true
                        },
                        "type": "text"
                    }

Would it be an option to add a warning to the user that they need to write down this value when they enter it - similar to what we do in other areas of the product? I believe it's like that for the main token displayed in serverless onboarding.

@kpollich
Copy link
Member

kpollich commented Jul 3, 2025

However, there also needs to be a way to see the secret-token, otherwise they cannot configure it - how can that be retrieved after this change? Is this by querying directly from Elasticsearch?

Kibana cannot retrieve Fleet secrets by design - only Fleet Server can read these secret values. If there's a requirement that users can see the secret token, is just rendering these as an input with type=password enough? Fleet secrets guarantee that kibana_system or any other non-Fleet-Server-users cannot read back the secret once written, so the value of the secret cannot be leaked in dev tools or anywhere else in Kibana.

@simitt
Copy link
Contributor

simitt commented Jul 3, 2025

Yeah allowing them to read it back kind of defeats the purpose of making this a secret in the first place. Since this is going out in a minor release version, I just want to ensure that we are not breaking existing workflows.
There is only one secret-token for the apm-server, if we force users to configure a new secret-token, it would mean they have to reconfigure all their apps currently using it. IMO if they can fetch it directly from ES (as long as they have privileges for it) it should be good enough. But PM input would definitely be valued here as this change is pretty user facing.

@kpollich
Copy link
Member

kpollich commented Jul 3, 2025

There is only one secret-token for the apm-server, if we force users to configure a new secret-token, it would mean they have to reconfigure all their apps currently using it.

I wonder if we can do the same thing we did when introducing Fleet Secrets initially here and have a "first time secret" flow where the current, plain-text value of the token is rendered with a warning that it will automatically be converted to a secret and will no longer be readable, so the user should copy it somewhere safe to maintain access to it. IIRC we have some code in Fleet to detect this with the policy saved objects, @criamico.

This might be tough with APM, though, since we automatically upgrade the package behind the scenes today. We want to introduce a user-facing step here where the user must consent to their token being "taken away" and turned into a secret. They need a chance to persist the value.

@criamico
Copy link
Contributor Author

criamico commented Jul 7, 2025

@elasticmachine merge upstream

@criamico criamico marked this pull request as draft July 7, 2025 07:43
@elasticmachine
Copy link
Contributor

🤖 Jobs for this PR can be triggered through checkboxes. 🚧

ℹ️ To trigger the CI, please tick the checkbox below 👇

  • Click to trigger kibana-pull-request for this PR!
  • Click to trigger kibana-deploy-project-from-pr for this PR!
  • Click to trigger kibana-deploy-cloud-from-pr for this PR!

@akhileshpok
Copy link

@simitt - Agree that showing a Create API Key button and nudging users to API Key should be incorporated as part of the onboarding flow.
@kpollich - We should not force existing APM users to configure a new secret-token for the apm-server as part of this change. Instead, we should ask for user consent (with accompanying docs) before forcing this change.

@criamico
Copy link
Contributor Author

criamico commented Jul 7, 2025

@kpollich @akhileshpok @simitt I did a local test where I buit the apm version with secret_token marked as secret: true and marked the changelog entry with breaking_changes.
Running the package registry locally I then verified that we have logic in place to detect this type of changes when trying to upgrade the integration:

Screenshot 2025-07-07 at 14 46 40 Screenshot 2025-07-07 at 14 47 04

So I think that this can be a starting point to warn the user when the upgrade is presented.

In addition, I'm checking how to build some logic based on this comment:

I wonder if we can do the same thing we did when introducing Fleet Secrets initially here and have a "first time secret" flow where the current, plain-text value of the token is rendered with a warning that it will automatically be converted to a secret and will no longer be readable, so the user should copy it somewhere safe to maintain access to it.

@kpollich
Copy link
Member

kpollich commented Jul 7, 2025

So I think that this can be a starting point to warn the user when the upgrade is presented.

+1 from me - I'm glad the work we did to add generic breaking change handling can be reused at least to some extent here. Since APM is auto-upgraded, I wonder if we could enhance this to flow to notify the user via a toast when the auto-upgrade failed due to a breaking change and direct them to the view you've screenshotted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team v9.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fleet] Implement package policy secrets for APM secret-token
6 participants