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.

Sign up for GitHub

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
+37 −6
Open

Add TLS configuration #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion devenv/vault-unseal.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
email:
enabled: false
tls_skip_verify: true
tls:
insecure: true
unseal_tokens:
- YOUR_TOKEN_HERE
vault_nodes:
Expand Down
3 changes: 2 additions & 1 deletion example.vault-unseal.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ unseal_tokens:

# skip tls checks for the given vault instance. useful if your instance doesn't
# have a certificate which has all of the server hostnames on it.
tls_skip_verify: false
tls:
insecure: false

# email notifications. setting this to false will disable all notifications.
email:
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ require (
github.com/hashicorp/vault/api v1.10.0
github.com/jessevdk/go-flags v1.5.0
github.com/joho/godotenv v1.5.1
github.com/mitchellh/mapstructure v1.5.0
github.com/phayes/permbits v0.0.0-20190612203442-39d7c581d2ee
gopkg.in/mail.v2 v2.3.1
gopkg.in/yaml.v2 v2.4.0
Expand All @@ -33,7 +34,6 @@ require (
github.com/mattn/go-colorable v0.1.13 // indirect
github.com/mattn/go-isatty v0.0.17 // indirect
github.com/mitchellh/go-homedir v1.1.0 // indirect
github.com/mitchellh/mapstructure v1.5.0 // indirect
github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e // indirect
github.com/pkg/errors v0.9.1 // indirect
github.com/ryanuber/go-glob v1.0.0 // indirect
Expand Down
35 changes: 32 additions & 3 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
vapi "github.com/hashicorp/vault/api"
flags "github.com/jessevdk/go-flags"
_ "github.com/joho/godotenv/autoload"
"github.com/mitchellh/mapstructure"
"github.com/phayes/permbits"
yaml "gopkg.in/yaml.v2"
)
Expand Down Expand Up @@ -61,8 +62,18 @@ type Config struct {

AllowSingleNode bool `env:"ALLOW_SINGLE_NODE" long:"allow-single-node" description:"allow vault-unseal to run on a single node" yaml:"allow_single_node" hidden:"true"`
Nodes []string `env:"NODES" long:"nodes" env-delim:"," description:"nodes to connect/provide tokens to (can be provided multiple times & uses comma-separated string for environment variable)" yaml:"vault_nodes"`
TLSSkipVerify bool `env:"TLS_SKIP_VERIFY" long:"tls-skip-verify" description:"disables tls certificate validation: DO NOT DO THIS" yaml:"tls_skip_verify"`
Tokens []string `env:"TOKENS" long:"tokens" env-delim:"," description:"tokens to provide to nodes (can be provided multiple times & uses comma-separated string for environment variable)" yaml:"unseal_tokens"`

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.

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.

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.

} `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.


Tokens []string `env:"TOKENS" long:"tokens" env-delim:"," description:"tokens to provide to nodes (can be provided multiple times & uses comma-separated string for environment variable)" yaml:"unseal_tokens"`

NotifyMaxElapsed time.Duration `env:"NOTIFY_MAX_ELAPSED" long:"notify-max-elapsed" description:"max time before the notification can be queued before it is sent" yaml:"notify_max_elapsed"`
NotifyQueueDelay time.Duration `env:"NOTIFY_QUEUE_DELAY" long:"notify-queue-delay" description:"time we queue the notification to allow as many notifications to be sent in one go (e.g. if no notification within X time, send all notifications)" yaml:"notify_queue_delay"`
Expand All @@ -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.

Result: dest,
})
if err != nil {
return err
}

return decoder.Decode(&src.TLS)
}

func newVault(addr string) (vault *vapi.Client) {
var err error

Expand All @@ -96,7 +118,14 @@ func newVault(addr string) (vault *vapi.Client) {
vconfig.MaxRetries = 0
vconfig.Timeout = defaultTimeout

if err = vconfig.ConfigureTLS(&vapi.TLSConfig{Insecure: conf.TLSSkipVerify}); err != nil {
var tlsConfigDest vapi.TLSConfig

errConvertTLS := ConvertTLSConfigToTLS(*conf, &tlsConfigDest)
if errConvertTLS != nil {
logger.Fatalf("error configuring vault client TLS: %v", errConvertTLS)
}

if err = vconfig.ConfigureTLS(&tlsConfigDest); err != nil {
logger.WithError(err).Fatal("error initializing tls config")
}

Expand Down
Loading