Add secret option to input vars

[joshdover]

What does this PR do?

This adds a new secret option to input vars to support securely stored values. These values can only be referenced via the upcoming secret provider in Elastic Agent. See https://github.com/elastic/ingest-dev/issues/999 for more context.

Why is it important?

We need to securely store, transmit, and redact (eg. in a diagnostic) sensitive fields in agent policies.

Checklist

• I have added test packages to test/packages that prove my change is effective.
• I have added an entry in versions/N/changelog.yml.

[elasticmachine]

💚 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-20T14:05:46.238+0000

• Duration: 8 min 46 sec

Test stats 🧪

Test	Results
Failed	0
Passed	876
Skipped	0
Total	876

🤖 GitHub comments

Expand to view the GitHub comments

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

• /test : Re-trigger the build.

[joshdover]

Skipped a step and went straight to a PR since this was a simple change. One question I'm sure that we'll get: we want to separate how the field is displayed from whether or not it's a secret that should be treated differently. This way we can have the appropriate input display in the UI. So we don't want to just assume all password fields are secret or not allow other input types to be secret, such as inputs for CA certificates private keys.

[mtojek]

Please remember about updating the changelog file to include this PR :)

[ph]

This look to me, using a question as a description is strange at first but this is the norm when looking at the other boolean flags defined in the package spec.

I assume this is only additiona logic and we can communicate that change with the integrations teams and they can start using it.

@joshdover maybe create an issue to audit for common secrets fields:

• password
• key
• token

[ph]

Skipped a step and went straight to a PR since this was a simple change. One question I'm sure that we'll get: we want to separate how the field is displayed from whether or not it's a secret that should be treated differently. This way we can have the appropriate input display in the UI. So we don't want to just assume all password fields are secret or not allow other input types to be secret, such as inputs for CA certificates private keys.

There might be some UI definition that needs to happen, when you are editing an integration that has already a private key defined, you want to see that we have something set for the field but not the actual value.

[joshdover]

There might be some UI definition that needs to happen, when you are editing an integration that has already a private key defined, you want to see that we have something set for the field but not the actual value.

I will put together a design doc for this end-to-end secret storage including UX around this.

[jlind23]

@pierrehilbert @jen-huang we should let @joshdover work on this design doc first but then lets make sure that Control Plane and Fleet UI are aligned on this feature.

[elasticmachine]

🌐 Coverage report

Name	Metrics % (covered/total)	Diff
Packages	100.0% (8/8)	💚
Files	73.333% (22/30)	👍
Classes	76.19% (32/42)	👍
Methods	59.091% (78/132)	👍
Lines	45.201% (730/1615)	👍
Conditionals	100.0% (0/0)	💚

[mrodm]

Missign a new changelog entry for this feature.

One doubt about this feature, what is the behaviour in Kibana for those versions that do not support this secret ? Should we just allow this secret field from 2.9.0 or from certain kibana version ?
cc @jsoriano

[hop-dev]

@mrodm regarding backwards compatibility, the field will be ignored by older kibana versions and the package will not be affected, so I don't think a version constraint is necessary.

Added examples + changelog in a888ac4

[mrodm]

@mrodm regarding backwards compatibility, the field will be ignored by older kibana versions and the package will not be affected, so I don't think a version constraint is necessary.

My concern is about that users/developers setting a field as secret but Kibana does not manage it as a secret and it could be shown afterwards. Should we disallow that?
I would like to have other opinions about this, please @jsoriano if you can chime in here 🙏

[mrodm]

This release was already published, please update your branch with main, there should be 2.8.1-next in the changelog.
https://github.com/elastic/package-spec/blob/e5b5ef533002a09fa1c0de1abd8ed6a467935402/spec/changelog.yml

[hop-dev]

ah 🤦 Thanks!

[mrodm]

Not sure about removing this field in previous versions using a JSON Patch.
If we do so, probably it should be for 2.9.0 (and it would be needed to update changelog too) ? Would it be better to keep for all versions for security reasons ? @jsoriano WDYT ?

versions:
  - before: 2.9.0 # other version 2.0.0 ? 
    patch:
      - op: remove
        path: /definitions/vars/items/properties/secret

[jsoriano]

+1 to add this to encourage updating the spec (and to update the version in the changelog).

[hop-dev]

@juliaElastic Mario and I have had a chat about this, I think we will need to restrict it so that a package developer can only set a var as secret if they have a kibana version condition of 8.9 or greater.

we have function similar to this e.g for checking runtime fields https://github.com/elastic/package-spec/blob/main/code/go/internal/validator/semantic/validate_minimum_kibana_version.go#L68

However we will need one to iterate over all of the vars in a package, including package level, input level and stream level vars and see if any is set to secret. I don't think I am going to have time to do this before I go I want to focus on getting the other PRs merged. @mrodm might be able to help with this next sprint if Jill feels she has enough on her plate?

[juliaElastic]

Thanks for the update. Do we consider the condition as a requirement for this pr or should we raise a separate issue?
If Mario can help out, that would be great. cc @jlind23

[jlind23]

@juliaElastic Thanks for the ping, as Mark is out @mrodm is the only one that can help us here.

[jsoriano]

@mrodm regarding backwards compatibility, the field will be ignored by older kibana versions and the package will not be affected, so I don't think a version constraint is necessary.

My concern is about that users/developers setting a field as secret but Kibana does not manage it as a secret and it could be shown afterwards. Should we disallow that?

Mario and I have had a chat about this, I think we will need to restrict it so that a package developer can only set a var as secret if they have a kibana version condition of 8.9 or greater.

In general I share the concern with Mario, it may be misleading for package developers to have an option silently ignored.

But thinking about this specific case, it may be better to go on without the version check. If we force package developers to update the kibana version to use secret, they may need to maintain another package version if they want to keep supporting older versions of kibana, what can discourage them about using this secure setting. And at the end the result is going to be similar.

[mrodm]

In general I share the concern with Mario, it may be misleading for package developers to have an option silently ignored.

But thinking about this specific case, it may be better to go on without the version check. If we force package developers to update the kibana version to use secret, they may need to maintain another package version if they want to keep supporting older versions of kibana, what can discourage them about using this secure setting. And at the end the result is going to be similar.

Thanks @jsoriano for your comments!
Let's do that then, let's add this new option/field without adding a new validation for kibana versions. But let's force to update to 2.9.0 version of the package-spec as mentioned here #339 (comment) @hop-dev @juliaElastic

[jlind23]

@mrodm @jsoriano could you please re-review it?

[jillguyonnet]

Hi @mrodm - I've pushed the changes and fixed the conflicts - can you have another look please?

[jillguyonnet]

@jsoriano Are you OK with merging this?

[jsoriano]

LGTM! 🚀