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

Send SNI information when connecting via TLS #7

Open
wants to merge 8 commits into
base: vast/0.18.x
Choose a base branch
from

Conversation

lava
Copy link
Member

@lava lava commented Feb 3, 2023

No description provided.

@lava lava force-pushed the topic/openssl-sni branch from 2b55680 to 7703f97 Compare February 3, 2023 13:44
Benno Evers added 4 commits February 15, 2023 18:06
Without this setting, OpenSSL would only validate that the certificate
has a valid signature from a trusted CA, but not that it actually matches
the host to whom we were trying to connect.
CAF currently expects to receive OpenSSL certificates and keys
as filenames. This matches the expectations of libopenssl, but
is cumbersome to use in a Cloud context. In particular, ECS
can only pass secrets as environment variables.

So we extend the command-line option to also accept a string
like `env:SOME_VARIABLE` and read the contents from the
environment variable `SOME_VARIABLE`.
@lava lava force-pushed the topic/openssl-sni branch from 0a7d5d4 to edf4fb7 Compare February 15, 2023 17:07
ofs << *contents;
ofs.close();
return filename;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I just realized that this approach will only work for docker-compose, there's no good way to inject environment variables when the user uses his own vast and a vast.yaml.

However adding the value without the indirection has the disadvantage that the secrets will appear everywhere the VAST config is printed.

Copy link
Member

Choose a reason for hiding this comment

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

Can we make it possible to pass the file path in the env variable for the second case?

Copy link
Member Author

Choose a reason for hiding this comment

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

A file path is how it already works, before our patches.

I think the cleanest solution is to not pass this via config at all but to provide a /get-client-certificates endpoint where we can download everything before starting CAF. (and to only use this mechanism for the manager nodes) But for the short term I just added support for putting the PEM directly into the option.

Benno Evers added 2 commits February 17, 2023 13:10
* Support for reading private keys and certificates directly
  as opposed to specifying the name of a separate environment
  variable, since the latter is not possible when using a
  `vast.yaml` config file.
  Note that this is just a crux, eventually we want to have
  a cloud endpoint where the node can download the required TLS
  files on the fly given some auth token.

* Support for OpenSSL 1.1.1 series. The OSSL_DECODER API is not
  available on that version, which is used on Debian Bullseye.
@lava lava force-pushed the topic/openssl-sni branch from 1cca273 to 1804776 Compare February 17, 2023 14:12
Unlike its lower-level variants, `PEM_read_bio_PrivateKey` is not
officially deprecated yet so we can avoid maintaining two versions
of the private key loading by relying on that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants