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

fix(core): refactor the openssl provider option so it is usable #18261

Closed

Conversation

hhromic
Copy link
Contributor

@hhromic hhromic commented Aug 15, 2023

While checking the OpenSSL legacy provider CLI option, I realized that it is actually unusable.
Therefore, this PR re-implements the --openssl-legacy-provider (bool) into --openssl-provider (enum).

Previously, the option default was set to true and there was no way to switch it to false using the CLI argument because the arg action by default is SetTrue. The associated environment variable was the only way to turn the legacy provider on or off. To avoid a potentially non-intuitive falsey --openssl-no-legacy-provider boolean option, it is now re-implemented as an enum option with all the possible built-in OpenSSL 3.1 implementation providers.

The default was kept as legacy to remain consistent with the previous behavior.
When the legacy provider is fully deprecated, we can simply switch the default for this option to default.

Note that I decided to include all of the built-in OpenSSL providers for completeness, but I'm not sure if the Vector team and users really need them all. Please let me know if you want to scope the list of available providers to a smaller list.

This is a breaking change for users using the VECTOR_OPENSSL_LEGACY_PROVIDER=false environment variable. This flag was released in Vector 0.32.0 (today) but not announced in the highlights. So I'm not sure to flag this as a breaking change or not. The migration step after this PR would be to use VECTOR_OPENSSL_PROVIDER=default.

OpenSSL 3.1 reference: https://www.openssl.org/docs/man3.1/man7/crypto.html (OPENSSL PROVIDERS)

This commit re-implements the `--openssl-legacy-provider` (bool) into `--openssl-provider` (enum).

Previously, the option default was set to `true` and there was no way to switch it to `false` using
the CLI argument because the arg action by default is `SetTrue`. The associated environment variable
was the only way to turn the legacy provider on or off. To avoid a potentially non-intuitive falsey
`--openssl-no-legacy-provider` boolean option, it is now re-implemented as an enum option with all
the possible built-in OpenSSL 3.1 implementation providers.

The default was kept as `legacy` to remain consistent with the previous behavior.

OpenSSL 3.1 reference: <https://www.openssl.org/docs/man3.1/man7/crypto.html> (OPENSSL PROVIDERS)

Signed-off-by: Hugo Hromic <hhromic@gmail.com>
@hhromic hhromic requested a review from a team as a code owner August 15, 2023 22:00
@netlify
Copy link

netlify bot commented Aug 15, 2023

Deploy Preview for vector-project ready!

Name Link
🔨 Latest commit fefea66
🔍 Latest deploy log https://app.netlify.com/sites/vector-project/deploys/64dbf7c16a7bda0008ebad40
😎 Deploy Preview https://deploy-preview-18261--vector-project.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Aug 15, 2023

Deploy Preview for vrl-playground canceled.

Name Link
🔨 Latest commit fefea66
🔍 Latest deploy log https://app.netlify.com/sites/vrl-playground/deploys/64dbf7c1d0adc200088cd2cd

@github-actions github-actions bot added the domain: external docs Anything related to Vector's external, public documentation label Aug 15, 2023
Signed-off-by: Hugo Hromic <hhromic@gmail.com>
@hhromic
Copy link
Contributor Author

hhromic commented Aug 15, 2023

A bit more context.

The original implementation was in PR #17669, where the default was originally false for the --openssl-legacy-provider option and therefore it worked correctly. Then the default was changed to true for backwards compatibility reasons and the option stopped working correctly.

@jszwedko jszwedko requested a review from dsmith3197 August 15, 2023 22:22
@jszwedko jszwedko added this to the Vector 0.32.1 milestone Aug 15, 2023
@jszwedko
Copy link
Member

Thanks for this @hhromic ! This does seem like a more flexible approach. This is another motivator to do release candidates to flag this sort of thing earlier.

It does seem like there isn't a way to disable it using the CLI flag. I was expecting --openssl-legacy-provider=false to work but it complains about excess arguments.

This is a breaking change for users using the VECTOR_OPENSSL_LEGACY_PROVIDER=false environment variable. This flag was released in Vector 0.32.0 (today) but not announced in the highlights. So I'm not sure to flag this as a breaking change or not. The migration step after this PR would be to use VECTOR_OPENSSL_PROVIDER=default.

I had actually meant to add it as a highlight, but completely forgot. I'm putting that together now.

Given it has technically been released, even if not particularly highlighted yet, I think we have to leave the flag in and remove it in 0.33.0 instead to meet user expectations for compatibility. For the purposes of your PR I think that would mean:

  1. Re-add --openssl-legacy-provider. We can open a different PR to fix 0.32.0 by making --openssl-legacy-provider able to take false as a value.
  2. Add your new CLI option and have it take precedence over --openssl-legacy-provider

In the future I think we can remove --openssl-legacy-provider still but leave in your added CLI option since it seems to be useful to switch between other providers such as the fips and null providers.

What do you think?

@hhromic
Copy link
Contributor Author

hhromic commented Aug 16, 2023

Hey @jszwedko , thanks for your comment/input !

I was expecting --openssl-legacy-provider=false to work but it complains about excess arguments.

Yep. Exactly how I realized it was not working too :).

The issue is that the attribute for this variable is boolean, so it does not take a value. The available actions for boolean arguments are SetTrue (default) or SetFalse. Given that the default for this attribute is true, for the arg flag to do something the action needs to be SetFalse. However, the name of the flag would not make sense anymore in this case. It would need to be renamed to something like --openssl-no-legacy-provider. This is what motivated the refactoring I did. If we are to rename the argument/env-variable then I took the opportunity to do it more flexible as you saw.

  • Re-add --openssl-legacy-provider. We can open a different PR to fix 0.32.0 by making --openssl-legacy-provider able to take false as a value.

The existing flag is fully broken because it doesn't do anything. I am confident nobody is using it or will not be able to use it. I honestly think that we can get rid of it without any harm. The environment variable VECTOR_OPENSSL_LEGACY_PROVIDER on the other hand does work as expected and in my opinion is the only aspect to preserve for backwards compatibility.

Modifying the attribute/flag to accept values and then parse true/false out of it I think will be too much boilerplate and generate inconsistency compared to other boolean flags. Not worth the effort imho.

In summary, I believe we can get away with renaming/changing the argument flag like I did in this PR as long as we preserve the funcionality of the VECTOR_OPENSSL_LEGACY_PROVIDER env variable. I looked at the highlight article you made for this feature and you only mentioned the env variable, not the flag. So even less confusion.

Maybe you can add a known-issue for 0.32.0 for this flag and mention that it will be refactored in the next version?

  • Add your new CLI option and have it take precedence over --openssl-legacy-provider

See above. I think the simplest approach would be for me to implement backwards compatibility for the VECTOR_OPENSSL_LEGACY_PROVIDER env variable alone and deprecate it for the next release. Then for the subsequent release we can simply remove support for the old env variable alone and the rest will be like in this PR.

What do you think?

@dsmith3197
Copy link
Contributor

Hi @hhromic,

Thank you for bringing this to our attention and for implementing a fix. For the sake of time and addressing a related issue, I made a separate PR fixing the existing cli arg (#18276) to avoid breaking backwards compatibility.

Going forward, I like your proposed changes. Given that a user can and sometimes would like to load multiple providers, I think we should change VECTOR_OPENSSL_PROVIDER to VECTOR_OPENSSL_PROVIDERS and make it a comma-separated list, similar to VECTOR_CONFIG_YAML. What do you think?

@dsmith3197 dsmith3197 removed this from the Vector 0.32.1 milestone Aug 16, 2023
@hhromic
Copy link
Contributor Author

hhromic commented Aug 16, 2023

Going forward, I like your proposed changes. Given that a user can and sometimes would like to load multiple providers, I think we should change VECTOR_OPENSSL_PROVIDER to VECTOR_OPENSSL_PROVIDERS and make it a comma-separated list, similar to VECTOR_CONFIG_YAML. What do you think?

Ah, I didn't think that you can chain providers. I thought that you are supposed to load one at a time.
These are implementation providers, not sure they really chain. Will check the OpenSSL documentation again.

@hhromic
Copy link
Contributor Author

hhromic commented Aug 16, 2023

A provider in OpenSSL is a component that collects together algorithm implementations. In order to use an algorithm you must have at least one provider loaded that contains an implementation of it. OpenSSL comes with a number of providers and they may also be obtained from third parties. If you don't load a provider explicitly (either in program code or via config) then the OpenSSL built-in "default" provider will be automatically loaded.

Ah you are right, it is possible to combine them. I'll do that then!

@dsmith3197 dsmith3197 added the meta: awaiting author Pull requests that are awaiting their author. label Aug 17, 2023
@jszwedko jszwedko added this to the Vector 0.33.0 milestone Aug 17, 2023
@hhromic
Copy link
Contributor Author

hhromic commented Aug 29, 2023

Hey @dsmith3197 @jszwedko ,

Given that --openssl-legacy-provider is now fixed and that the ability to use the legacy provider is deprecated and expected to be removed entirely, I am not so sure this PR is relevant anymore.

I gave this more thought and I am not sure having the ability to configure arbitrary SSL providers is truly useful to users after all. The only potential use case could be enabling the FIPS provider but I think it would be better to implement it using a dedicated --openssl-use-fips flag (in the future when this is properly understood/tested) instead of a generic OpenSSL providers flag.

All that being said, let me know if I should simply close this PR for now. No worries!

@dsmith3197
Copy link
Contributor

Hey @dsmith3197 @jszwedko ,

Given that --openssl-legacy-provider is now fixed and that the ability to use the legacy provider is deprecated and expected to be removed entirely, I am not so sure this PR is relevant anymore.

I gave this more thought and I am not sure having the ability to configure arbitrary SSL providers is truly useful to users after all. The only potential use case could be enabling the FIPS provider but I think it would be better to implement it using a dedicated --openssl-use-fips flag (in the future when this is properly understood/tested) instead of a generic OpenSSL providers flag.

All that being said, let me know if I should simply close this PR for now. No worries!

Hey @hhromic,

I think I agree with your assessment overall. Implementing / assessing proper FIPS support for Vector will take some effort, particularly because some of the crates we use depend upon tls crates other than openssl. I think we can close this PR for now. Thank you for all your help!

@dsmith3197 dsmith3197 closed this Aug 30, 2023
@hhromic hhromic deleted the fix-openssl-provider-flag branch August 30, 2023 14:33
@jszwedko
Copy link
Member

jszwedko commented Sep 1, 2023

I personally think this PR would still be useful to as it allow users to configure the FIPS provider. Granted I agree that there is more work to be done to make Vector totally FIPS compliant but it could be a stepping stone and sufficient for some users to satisfy their requirements. It would also future proof Vector to let users use custom providers, once those exist more in the wild. Per the OpenSSL docs:

There are five providers distributed with OpenSSL. In the future, we expect third parties to distribute their own providers which can be added to OpenSSL dynamically. Documentation about writing providers is available on the provider(7) manual page.

It will also allow users who want to use the legacy provider after it is removed from Vector's default providers, to do so.

What do you think @hhromic ? Would you be open to pushing this PR forward if you agree? Otherwise I can open an issue for us to follow up on.

@hhromic
Copy link
Contributor Author

hhromic commented Sep 1, 2023

What do you think @hhromic ? Would you be open to pushing this PR forward if you agree? Otherwise I can open an issue for us to follow up on.

After taking a closer look to the OpenSSL FIPS module documentation, it is clear that the recommended approach is to configure OpenSSL on the system (e.g. in /etc/ssl/openssl.cnf or similar) and let applications simply rely on OpenSSL as-is without requiring code modifications for different setups.

I just did a quick check and Vector's vendored OpenSSL module seems to try to read a system configuration file by default:

root@docker-desktop:/git/vectordotdev/vector# strace target/debug/vector -c test.yaml 2>&1 | grep -F openssl.cnf
openat(AT_FDCWD, "/usr/local/ssl/openssl.cnf", O_RDONLY) = -1 ENOENT (No such file or directory)

That default location is likely missing in most environments, however it can be customised and Vector does pick it up:

root@docker-desktop:/git/vectordotdev/vector# OPENSSL_CONF=/etc/ssl/openssl.cnf strace target/debug/vector -c test.yaml 2>&1 | grep -F openssl.cnf
openat(AT_FDCWD, "/etc/ssl/openssl.cnf", O_RDONLY) = 3

In other words, I think Vector actually already fully supports FIPS and any openssl providers via configuration this way :)
In pure OpenSSL spirit, "no application code changes are necessary for different providers".

This also means that users wanting to use the legacy provider, could have done so via OpenSSL configuration without even needing the temporary --openssl-legacy-provider flag.

If you agree, I can do some experiments and tests and submit a documentation PR explaining how to leverage configuring OpenSSL externally to Vector for advanced setups. This keeps Vector code less polluted too.

@hhromic
Copy link
Contributor Author

hhromic commented Sep 2, 2023

Out of curiosity/challenge I went ahead and managed to prove my own prediction:

In other words, I think Vector actually already fully supports FIPS and any openssl providers via configuration this way :)
In pure OpenSSL spirit, "no application code changes are necessary for different providers".
This also means that users wanting to use the legacy provider, could have done so via OpenSSL configuration without even needing the temporary --openssl-legacy-provider flag.

First, create a self-signed certificate/key pair for testing (make sure you are using OpenSSL 3.x tools!):

$ openssl genrsa -out private.key 2048
$ openssl req -new -key private.key -out request.csr   # does not matter what you input here
$ openssl x509 -req -in request.csr -signkey private.key -out certificate.crt

Then, create a PKCS#12 file from the above using LEGACY algorithms (notice the -legacy flag):

$ openssl pkcs12 -export -inkey private.key -in certificate.crt -name certificate -out certificate.p12 -legacy

Write down the password used to encrypt the file. For the example below, I just used 1234.

For testing, consider the following Vector pipeline that will try to use the above PKCS#12 file:

sources:
  exec:
    type: exec
    command: ["printenv"]
    mode: scheduled
sinks:
  http:
    type: http
    inputs: ["exec"]
    uri: https://localhost:1234
    tls:
      crt_file: certificate.p12
      key_pass: "1234"   # used in the "openssl pkcs12 ..." command
    encoding:
      codec: json

If you run Vector with the above pipeline and the legacy provider disabled, it will NOT work (legacy alg not found):

$ vector -c test.yaml --openssl-legacy-provider=false
2023-09-02T18:40:36.154592Z  INFO vector::app: Log level is enabled. level="vector=info,codec=info,vrl=info,file_source=info,tower_limit=info,rdkafka=info,buffers=info,lapin=info,kube=info"
2023-09-02T18:40:36.157184Z  INFO vector::app: Loading configs. paths=["test.yaml"]
2023-09-02T18:40:36.175296Z ERROR vector::topology: Configuration error. error=Sink "http": PKCS#12 parse failed: error:0308010C:digital envelope routines:inner_evp_generic_fetch:unsupported:crypto/evp/evp_fetch.c:341:Global default library context, Algorithm (RC2-40-CBC : 0), Properties ()

Now, let's create an OpenSSL configuration file that enables the legacy provider OUTSIDE of Vector:

openssl_conf = openssl_init

[openssl_init]
providers = provider_sect

[provider_sect]
default = default_sect
legacy = legacy_sect

[default_sect]
activate = 1

[legacy_sect]
activate = 1

If we run Vector again using the above configuration AND --openssl-legacy-provider=false it WILL work:

# OPENSSL_CONF=vector-openssl.cnf vector -c test.yaml --openssl-legacy-provider=false
2023-09-02T18:43:23.532275Z  INFO vector::app: Log level is enabled. level="vector=info,codec=info,vrl=info,file_source=info,tower_limit=info,rdkafka=info,buffers=info,lapin=info,kube=info"
2023-09-02T18:43:23.535673Z  INFO vector::app: Loading configs. paths=["test.yaml"]
2023-09-02T18:43:23.645134Z  INFO vector::topology::running: Running healthchecks.
2023-09-02T18:43:23.645701Z  INFO vector::topology::builder: Healthcheck passed.
2023-09-02T18:43:23.646989Z  INFO vector: Vector has started. debug="true" version="0.33.0" arch="x86_64" revision=""
2023-09-02T18:43:23.647211Z  INFO vector::app: API is disabled, enable by setting `api.enabled` to `true` and use commands like `vector top`.

It is true that an --openssl-legacy-provider flag is more convenient than the above but Vector was always capable of running in legacy mode without it. Moreover, whenever you remove this flag, users will actually be still able to use legacy mode with the above approach.

In this same manner, any OpenSSL setup (including using the FIPS module) can be used in Vector without needing any specific code in Vector itself to handle different advanced configurations. This is the way operators are supposed to configure applications with a single point of configuration when all of them use OpenSSL as the SSL/TLS library.

TLDR; if an operator/user configures FIPS in the system via /etc/ssl/openssl.cnf or similar for all of their applications to work in SSL/TLS FIPS-compliant mode, all they need to do for Vector is to start it with OPENSSL_CONF=/etc/ssl/openssl.cnf in the environment. Note that, until --openssl-legacy-provider is removed, users MUST also use --openssl-legacy-provider=false with OPENSSL_CONF=... because currently Vector by default overrides the configuration with the legacy provider.

In summary, I think Vector should get rid of --openssl-legacy-provider as soon as possible and document how to properly point Vector to standard OpenSSL configuration files. I would add the above example for using legacy algorithms and point to the official documentation for the FIPS module. Let me know if you want me to contribute such documentation 👍.

@juvenn
Copy link
Contributor

juvenn commented Sep 9, 2023

If you agree, I can do some experiments and tests and submit a documentation PR explaining how to leverage configuring OpenSSL externally to Vector for advanced setups.

Coming from #18467 , I can't agree more that leveraging and adding doc about system openssl seems better approach overall. OpenSSL has plethora of options to configure, proxy those in vector can be tiresome and confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: external docs Anything related to Vector's external, public documentation meta: awaiting author Pull requests that are awaiting their author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants