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

[Change Proposal] Add special tls variable types #761

Open
kpollich opened this issue Jun 11, 2024 · 5 comments
Open

[Change Proposal] Add special tls variable types #761

kpollich opened this issue Jun 11, 2024 · 5 comments
Labels
Team:Ecosystem Label for the Packages Ecosystem team

Comments

@kpollich
Copy link
Member

Ref @andrewkroh's comment here: elastic/integrations#8610 (comment)

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:

  • TLS server variables
  • TLS client variables

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 kpollich added the Team:Ecosystem Label for the Packages Ecosystem team label Jun 11, 2024
@kpollich
Copy link
Member Author

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
Copy link
Member

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 use the settings for deployment modes, allowing to hide TLS options in deployment modes where they are not needed.
  • TLS variables could be part of "conditional groups of variables" in integrations for services that have multiple options for authentication.
  • Title, description, whether it is required and so on can be configured.

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
Copy link
Member Author

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
Copy link
Member

jsoriano commented Jun 12, 2024

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
Copy link
Member

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 code1 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.

Footnotes

  1. https://github.com/elastic/elastic-agent-libs/blob/76a4612dc357494f1ff268001f377c14c200f3b9/transport/tlscommon/config.go#L25-L37

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Ecosystem Label for the Packages Ecosystem team
Projects
None yet
Development

No branches or pull requests

3 participants