elastic / package-spec

EPR package specifications
Other
15 stars 70 forks source link

[Change Proposal] Add special `tls` variable types #761

Open kpollich opened 1 month ago

kpollich commented 1 month ago

Ref @andrewkroh's comment here: https://github.com/elastic/integrations/issues/8610#issuecomment-2082564625

Today, there is duplication across integrations that include TLS related fields. In an effort to remove this duplication, we should provide special tls variable types that are "expanded" by Fleet UI to render + provide boilerplate TLS variables without integration maintainers needing to copy/paste TLS configuration across many integrations.

Providing this abstraction will also allow Fleet UI to build a better, more tailored UX around TLS fields that makes use of secrets as appropriate and provides something better than a few text inputs and a YAML textarea.

There are two distinct types of TLS variables exposed in integrations:

TODO: Add specific examples of each type of variable in an existing integration, clarify exactly which variables should be included in which type.

Rather than define these variables directly, an integration maintainer should be able to define a flag at any level for which vars might exist, e.g.

# my_integration/manifest.yml
policy_templates:
  - name: my_integration
    title: My integration
    include_tls_server_vars: true
    vars:
      - name: bar
        type: text
        ...

Only one of include_tls_server_vars and include_tls_client_vars should be defined at a time in a given manifest.

kpollich commented 1 month ago

TODO: Add specific examples of each type of variable in an existing integration, clarify exactly which variables should be included in which type.

@andrewkroh - Can you help with this?

Once this is a bit more defined I'll create a Kibana issue as well to capture the need to support this, and maybe do a quick UX mock to see what the baseline set of TLS fields would look like w/ secret support as well in the UI.

jsoriano commented 1 month ago

Rather than define these variables directly, an integration maintainer should be able to define a flag at any level for which vars might exist

What are the benefits of defining this as a single boolean flag?

Defining it as a normal var has the benefits of allowing to reuse any logic that we have for variables, for example:

We could define it with var types, so variables are defined something like these:

          - name: ssl
            type: tls_client_options
            title: SSL Configuration
            required: false
          - name: ssl
            type: tls_server_options
            title: SSL Configuration

And these variables could be referenced in templates as members of an object, for example ssl.certificate_authorities.

Also, TLS options are a extense topic, there may be many advanced options that not all packages support. Having this as an object would also allow to decide what fields to show, taking into account what is supported by the package. For example something like this to show only fields for key, password, certificate and verification mode, but not for CAs:

          - name: ssl
            type: tls_client_options
            title: SSL Configuration
            tls_fields:
              - key
              - key_passphrase
              - certificate
              - verification_mode
kpollich commented 1 month ago

What are the benefits of defining this as a single boolean flag?

Probably none 😆 - I just hadn't thought through what this would look like in detail yet and that seemed like a decent starting point.

I like the idea of using a var type instead of the flag for the reasons you've listed. One thing I'm not sure about is whether the name field of the variable should be enforced statically. e.g. should this variable always have to be named ssl if it declares a type of tls_client/server_options.

jsoriano commented 1 month ago

One thing I'm not sure about is whether the name field of the variable should be enforced statically. e.g. should this variable always have to be named ssl if it declares a type of tls_client/server_options.

I wouldn't enforce it, this would add an special case, at the moment we expect variables to have names defined in the manifests files.

It would also limit this variable type to appear only once per package or data stream. I don't know of any package now that would need two ssl config blocks, but not sure, and this would place a limitation on that.

Btw, we also need to think how this variable will work in the API. Will the API accept an object with the subfields? Something like the following?

          "vars": {
            "ssl": {
              "key": "somekey...",
              "key_password": "somepassword",
              "cert": "somecert...",
            },
andrewkroh commented 1 month ago

TODO: Add specific examples of each type of variable in an existing integration, clarify exactly which variables should be included in which type.

@andrewkroh - Can you help with this?

I have reviewed the code^1 associated with Beats, and created a schema that includes a distinct types for client and server usages. The schema is expressed using cuelang, and I have generated an openapi schema from it because that is likely more familiar to most. Both are shared in this gist https://gist.github.com/andrewkroh/206610eecade896de9862a552a065f0b. I included NOTE: This is a secret. in the descriptions of fields should be treated as sensitive.