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

Add TLS configuration #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

heurtematte
Copy link
Contributor

🚀 Changes proposed by this PR

I propose to add full TLS configuration for vault client.

TLS struct {
	CACert        string `env:"TLS_CA_CERT" long:"ca-cert" description:"the path to a PEM-encoded CA cert file to use to verify the Vault server SSL certificate. It takes precedence over CACertBytes and CAPath" yaml:"ca_cert"`
	CACertBytes   []byte `env:"TLS_CA_CERT_BYTES" long:"ca-cert-bytes" description:"PEM-encoded certificate or bundle. It takes precedence over CAPath" yaml:"ca_cert_bytes"`
	CAPath        string `env:"TLS_CA_PATH" long:"ca-path" description:"path to a directory of PEM-encoded CA cert files to verify the Vault server SSL certificate" yaml:"ca_path"`
	ClientCert    string `env:"TLS_CLIENT_CERT" long:"client-cert" description:"path to the certificate for Vault communication" yaml:"client_path"`
	ClientKey     string `env:"TLS_CLIENT_KEY" long:"client-key" description:"path to the private key for Vault communication" yaml:"client_key"`
	TLSServerName string `env:"TLS_SERVER_NAME" long:"server-name" description:"if set, is used to set the SNI host when connecting via TLS" yaml:"server_name"`
	Insecure      bool   `env:"TLS_INSECURE" long:"insecure" description:"enables or disables SSL verification: DO NOT DO THIS" yaml:"insecure"`
} `group:"TLS Options" namespace:"tls" yaml:"tls"`

tls_skip_verifyis replaced by insecure parameter in tlsstruct.

i.e:

tls:
  insecure: false

🧰 Type of change

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that causes existing functionality to not work as expected).
  • This change requires (or is) a documentation update.

Signed-off-by: sebastien.heurtematte <[email protected]>

TLS struct {
CACert string `env:"TLS_CA_CERT" long:"ca-cert" description:"the path to a PEM-encoded CA cert file to use to verify the Vault server SSL certificate. It takes precedence over CACertBytes and CAPath" yaml:"ca_cert"`
CACertBytes []byte `env:"TLS_CA_CERT_BYTES" long:"ca-cert-bytes" description:"PEM-encoded certificate or bundle. It takes precedence over CAPath" yaml:"ca_cert_bytes"`
Copy link
Owner

Choose a reason for hiding this comment

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

What is the point of ca-cert, ca-cert-bytes, and ca-path all being supported? ca-path is going to be the most secure option, so should probably be the only supported option out of the three.

ClientKey string `env:"TLS_CLIENT_KEY" long:"client-key" description:"path to the private key for Vault communication" yaml:"client_key"`
TLSServerName string `env:"TLS_SERVER_NAME" long:"server-name" description:"if set, is used to set the SNI host when connecting via TLS" yaml:"server_name"`
Insecure bool `env:"TLS_INSECURE" long:"insecure" description:"enables or disables SSL verification: DO NOT DO THIS" yaml:"insecure"`
} `group:"TLS Options" namespace:"tls" yaml:"tls"`
Copy link
Owner

Choose a reason for hiding this comment

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

env-namespace:"TLS" can be added here, and the TLS_ prefix can be removed from all other flags.

@@ -88,6 +99,17 @@ var (
logger log.Interface
)

func ConvertTLSConfigToTLS(src Config, dest *vapi.TLSConfig) error {
decoder, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{
Copy link
Owner

Choose a reason for hiding this comment

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

using mapstructure here feels fragile, as it moves most type safety to runtime, rather than compile time. Mapping from flags to vault TLSConfig{} should be done specifically for the fields we want to override via struct initialization, similar to what we had before.

ClientCert string `env:"TLS_CLIENT_CERT" long:"client-cert" description:"path to the certificate for Vault communication" yaml:"client_path"`
ClientKey string `env:"TLS_CLIENT_KEY" long:"client-key" description:"path to the private key for Vault communication" yaml:"client_key"`
TLSServerName string `env:"TLS_SERVER_NAME" long:"server-name" description:"if set, is used to set the SNI host when connecting via TLS" yaml:"server_name"`
Insecure bool `env:"TLS_INSECURE" long:"insecure" description:"enables or disables SSL verification: DO NOT DO THIS" yaml:"insecure"`
Copy link
Owner

Choose a reason for hiding this comment

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

TLS_SKIP_VERIFY is common for Go apps, so I would prefer to keep the naming standard. with the below note about env-namespace, if you keep the environment variable as SKIP_VERIFY, this will ensure the old environment variable method approach to providing this flag will still work.

I would still support --tls-skip-verify, however, remove env from the old flag. In the TLSConfig{} initialization, the new flag should take priority.

CAPath string `env:"TLS_CA_PATH" long:"ca-path" description:"path to a directory of PEM-encoded CA cert files to verify the Vault server SSL certificate" yaml:"ca_path"`
ClientCert string `env:"TLS_CLIENT_CERT" long:"client-cert" description:"path to the certificate for Vault communication" yaml:"client_path"`
ClientKey string `env:"TLS_CLIENT_KEY" long:"client-key" description:"path to the private key for Vault communication" yaml:"client_key"`
TLSServerName string `env:"TLS_SERVER_NAME" long:"server-name" description:"if set, is used to set the SNI host when connecting via TLS" yaml:"server_name"`
Copy link
Owner

Choose a reason for hiding this comment

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

Almost forgot, I don't see a scenario where --tls.server-name should be used -- if it is, sounds like there might be a misconfiguration, legacy environment, etc, which I don't think should be supported.

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