From 13ad34d4f63a0a6cf8015a3d9917b7fe866d76ca Mon Sep 17 00:00:00 2001 From: Mario Rodriguez Molins Date: Wed, 25 Oct 2023 13:20:34 +0200 Subject: [PATCH 1/8] Add tls-min-version parameter --- main.go | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/main.go b/main.go index ad73f1d84..c56384aa2 100644 --- a/main.go +++ b/main.go @@ -6,6 +6,7 @@ package main import ( "context" + "crypto/tls" "flag" "fmt" "log" @@ -50,8 +51,9 @@ var ( logLevel *zapcore.Level logType string - tlsCertFile string - tlsKeyFile string + tlsCertFile string + tlsKeyFile string + tlsMinVersion string dryRun bool configPath string @@ -83,6 +85,7 @@ func init() { flag.StringVar(&logType, "log-type", util.DefaultLoggerType, "log type (ecs, dev)") flag.StringVar(&tlsCertFile, "tls-cert", "", "Path of the TLS certificate.") flag.StringVar(&tlsKeyFile, "tls-key", "", "Path of the TLS key.") + flag.StringVar(&tlsMinVersion, "tls-min-version", "", "Minimum version TLS supported.") flag.StringVar(&configPath, "config", "config.yml", "Path to the configuration file.") flag.StringVar(&httpProfAddress, "httpprof", "", "Enable HTTP profiler listening on the given address.") // This flag is experimental and might be removed in the future or renamed @@ -142,7 +145,10 @@ func main() { initHttpProf(logger) - server := initServer(logger, apmTracer, config) + server, err := initServer(logger, apmTracer, config) + if err != nil { + logger.Fatal("error occurred while initializing the server", zap.Error(err)) + } go func() { err := runServer(server) if err != nil && err != http.ErrServerClosed { @@ -230,7 +236,7 @@ func initIndexer(ctx context.Context, logger *zap.Logger, apmTracer *apm.Tracer, return combined } -func initServer(logger *zap.Logger, apmTracer *apm.Tracer, config *Config) *http.Server { +func initServer(logger *zap.Logger, apmTracer *apm.Tracer, config *Config) (*http.Server, error) { tx := apmTracer.StartTransaction("initServer", "backend.init") defer tx.End() @@ -241,7 +247,25 @@ func initServer(logger *zap.Logger, apmTracer *apm.Tracer, config *Config) *http router := mustLoadRouter(logger, config, indexer) apmgorilla.Instrument(router, apmgorilla.WithTracer(apmTracer)) - return &http.Server{Addr: address, Handler: router} + if tlsMinVersion == "" { + return &http.Server{Addr: address, Handler: router}, nil + } + + TLSConfig := &tls.Config{} + switch tlsMinVersion { + // SSLv3 option is considered deprecated: https://cs.opensource.google/go/go/+/refs/tags/go1.21.3:src/crypto/tls/common.go;l=34 + case "1.0": + TLSConfig.MinVersion = tls.VersionTLS10 + case "1.1": + TLSConfig.MinVersion = tls.VersionTLS11 + case "1.2": + TLSConfig.MinVersion = tls.VersionTLS12 + case "1.3": + TLSConfig.MinVersion = tls.VersionTLS13 + default: + return nil, fmt.Errorf("unsupported TLS version %s", tlsMinVersion) + } + return &http.Server{Addr: address, Handler: router, TLSConfig: TLSConfig}, nil } func runServer(server *http.Server) error { From 22a02e3fe143eeeda12537718911471849c57248 Mon Sep 17 00:00:00 2001 From: Mario Rodriguez Molins Date: Wed, 25 Oct 2023 18:30:57 +0200 Subject: [PATCH 2/8] Move to flag.Var the tlsMinVersion parameter --- flags.go | 51 +++++++++++++++++++++++++++++++++++++++++++++------ flags_test.go | 3 ++- main.go | 47 +++++++++++++++++++++-------------------------- 3 files changed, 68 insertions(+), 33 deletions(-) diff --git a/flags.go b/flags.go index 2571700eb..3c1d76012 100644 --- a/flags.go +++ b/flags.go @@ -5,29 +5,68 @@ package main import ( + "crypto/tls" + "errors" "flag" + "fmt" "os" "strings" ) -func parseFlags() { - parseFlagSetWithArgs(flag.CommandLine, os.Args) +var supportedTLSVersions map[string]uint16 = map[string]uint16{ + "1.0": tls.VersionTLS10, + "1.1": tls.VersionTLS11, + "1.2": tls.VersionTLS12, + "1.3": tls.VersionTLS13, } -func parseFlagSetWithArgs(flagSet *flag.FlagSet, args []string) { - flagsFromEnv(flagSet) +type tlsMinVersionValue struct { + version *string + versionCode *uint16 +} + +func (t tlsMinVersionValue) String() string { + if t.version != nil { + return *t.version + } + return "" +} + +func (t tlsMinVersionValue) Set(s string) error { + if _, ok := supportedTLSVersions[s]; !ok { + return fmt.Errorf("unsupported TLS version: %s", s) + } + *t.version = s + *t.versionCode = supportedTLSVersions[s] + return nil +} + +func parseFlags() error { + return parseFlagSetWithArgs(flag.CommandLine, os.Args) +} + +func parseFlagSetWithArgs(flagSet *flag.FlagSet, args []string) error { + err := flagsFromEnv(flagSet) + if err != nil { + return err + } // Skip args[0] as flag.Parse() does. flagSet.Parse(args[1:]) + return nil } -func flagsFromEnv(flagSet *flag.FlagSet) { +func flagsFromEnv(flagSet *flag.FlagSet) error { + var flagErrors error flagSet.VisitAll(func(f *flag.Flag) { envName := flagEnvName(f.Name) if value, found := os.LookupEnv(envName); found { - f.Value.Set(value) + if err := f.Value.Set(value); err != nil { + flagErrors = errors.Join(flagErrors, fmt.Errorf("failed to set -%s: %v", f.Name, err)) + } } }) + return flagErrors } const flagEnvPrefix = "EPR_" diff --git a/flags_test.go b/flags_test.go index dcb9cde6c..d91240124 100644 --- a/flags_test.go +++ b/flags_test.go @@ -36,7 +36,8 @@ func TestFlagsPrecedence(t *testing.T) { require.Equal(t, "default", dummyFlag) args := []string{"test", "-test-precedence-dummy=" + expected} - parseFlagSetWithArgs(flagSet, args) + err := parseFlagSetWithArgs(flagSet, args) + require.NoError(t, err) require.Equal(t, expected, dummyFlag) } diff --git a/main.go b/main.go index c56384aa2..ede5deea9 100644 --- a/main.go +++ b/main.go @@ -51,9 +51,11 @@ var ( logLevel *zapcore.Level logType string - tlsCertFile string - tlsKeyFile string - tlsMinVersion string + tlsCertFile string + tlsKeyFile string + + tlsMinVersion string + tlsMinVersionCode uint16 dryRun bool configPath string @@ -85,7 +87,7 @@ func init() { flag.StringVar(&logType, "log-type", util.DefaultLoggerType, "log type (ecs, dev)") flag.StringVar(&tlsCertFile, "tls-cert", "", "Path of the TLS certificate.") flag.StringVar(&tlsKeyFile, "tls-key", "", "Path of the TLS key.") - flag.StringVar(&tlsMinVersion, "tls-min-version", "", "Minimum version TLS supported.") + flag.Var(&tlsMinVersionValue{version: &tlsMinVersion, versionCode: &tlsMinVersionCode}, "tls-min-version", "Minimum version TLS supported.") flag.StringVar(&configPath, "config", "config.yml", "Path to the configuration file.") flag.StringVar(&httpProfAddress, "httpprof", "", "Enable HTTP profiler listening on the given address.") // This flag is experimental and might be removed in the future or renamed @@ -111,7 +113,16 @@ type Config struct { } func main() { - parseFlags() + err := parseFlags() + if err != nil { + log.Fatal(err) + } + + if tlsMinVersion != "" { + if tlsCertFile == "" || tlsKeyFile == "" { + log.Fatalf("-tls-min-version set but missing TLS cert and key files (-tls-cert and -tls-key)") + } + } if printVersionInfo { fmt.Printf("Elastic Package Registry version %v\n", version) @@ -145,10 +156,7 @@ func main() { initHttpProf(logger) - server, err := initServer(logger, apmTracer, config) - if err != nil { - logger.Fatal("error occurred while initializing the server", zap.Error(err)) - } + server := initServer(logger, apmTracer, config) go func() { err := runServer(server) if err != nil && err != http.ErrServerClosed { @@ -236,7 +244,7 @@ func initIndexer(ctx context.Context, logger *zap.Logger, apmTracer *apm.Tracer, return combined } -func initServer(logger *zap.Logger, apmTracer *apm.Tracer, config *Config) (*http.Server, error) { +func initServer(logger *zap.Logger, apmTracer *apm.Tracer, config *Config) *http.Server { tx := apmTracer.StartTransaction("initServer", "backend.init") defer tx.End() @@ -248,24 +256,11 @@ func initServer(logger *zap.Logger, apmTracer *apm.Tracer, config *Config) (*htt apmgorilla.Instrument(router, apmgorilla.WithTracer(apmTracer)) if tlsMinVersion == "" { - return &http.Server{Addr: address, Handler: router}, nil + return &http.Server{Addr: address, Handler: router} } - TLSConfig := &tls.Config{} - switch tlsMinVersion { - // SSLv3 option is considered deprecated: https://cs.opensource.google/go/go/+/refs/tags/go1.21.3:src/crypto/tls/common.go;l=34 - case "1.0": - TLSConfig.MinVersion = tls.VersionTLS10 - case "1.1": - TLSConfig.MinVersion = tls.VersionTLS11 - case "1.2": - TLSConfig.MinVersion = tls.VersionTLS12 - case "1.3": - TLSConfig.MinVersion = tls.VersionTLS13 - default: - return nil, fmt.Errorf("unsupported TLS version %s", tlsMinVersion) - } - return &http.Server{Addr: address, Handler: router, TLSConfig: TLSConfig}, nil + TLSConfig := &tls.Config{MinVersion: tlsMinVersionCode} + return &http.Server{Addr: address, Handler: router, TLSConfig: TLSConfig} } func runServer(server *http.Server) error { From 52b742f9b9b18afeeda95fb6147e58c3a31977ae Mon Sep 17 00:00:00 2001 From: Mario Rodriguez Molins Date: Wed, 25 Oct 2023 18:33:44 +0200 Subject: [PATCH 3/8] Add changelog entry --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b08240862..51941b836 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +* Add new parameter to specify minimum TLS version [#1103](https://github.com/elastic/package-registry/pull/1103) + ### Deprecated ### Known Issues From 5c6827e345e01055d5560aa87410d66967d842c6 Mon Sep 17 00:00:00 2001 From: Mario Rodriguez Molins Date: Thu, 26 Oct 2023 11:38:51 +0200 Subject: [PATCH 4/8] Changes from code review --- flags.go | 33 +++++++++++++++++++++++---------- main.go | 19 ++++++++++--------- 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/flags.go b/flags.go index 3c1d76012..97cfe4167 100644 --- a/flags.go +++ b/flags.go @@ -20,24 +20,37 @@ var supportedTLSVersions map[string]uint16 = map[string]uint16{ "1.3": tls.VersionTLS13, } -type tlsMinVersionValue struct { - version *string - versionCode *uint16 +type tlsVersionValue struct { + version *uint16 } -func (t tlsMinVersionValue) String() string { - if t.version != nil { - return *t.version +func (t tlsVersionValue) String() string { + if t.version == nil { + return "" } - return "" + switch *t.version { + case tls.VersionTLS10: + return "1.0" + case tls.VersionTLS11: + return "1.1" + case tls.VersionTLS12: + return "1.2" + case tls.VersionTLS13: + return "1.3" + default: + return "" + } +} + +func (t tlsVersionValue) Value() uint16 { + return *t.version } -func (t tlsMinVersionValue) Set(s string) error { +func (t tlsVersionValue) Set(s string) error { if _, ok := supportedTLSVersions[s]; !ok { return fmt.Errorf("unsupported TLS version: %s", s) } - *t.version = s - *t.versionCode = supportedTLSVersions[s] + *t.version = supportedTLSVersions[s] return nil } diff --git a/main.go b/main.go index ede5deea9..1a828aa17 100644 --- a/main.go +++ b/main.go @@ -54,8 +54,8 @@ var ( tlsCertFile string tlsKeyFile string - tlsMinVersion string - tlsMinVersionCode uint16 + tlsMinVersionCode uint16 + tlsMinVersionValue tlsVersionValue dryRun bool configPath string @@ -79,6 +79,8 @@ var ( ) func init() { + tlsMinVersionValue = tlsVersionValue{version: &tlsMinVersionCode} + flag.BoolVar(&printVersionInfo, "version", false, "Print Elastic Package Registry version") flag.StringVar(&address, "address", "localhost:8080", "Address of the package-registry service.") flag.StringVar(&metricsAddress, "metrics-address", "", "Address to expose the Prometheus metrics (experimental). ") @@ -87,7 +89,7 @@ func init() { flag.StringVar(&logType, "log-type", util.DefaultLoggerType, "log type (ecs, dev)") flag.StringVar(&tlsCertFile, "tls-cert", "", "Path of the TLS certificate.") flag.StringVar(&tlsKeyFile, "tls-key", "", "Path of the TLS key.") - flag.Var(&tlsMinVersionValue{version: &tlsMinVersion, versionCode: &tlsMinVersionCode}, "tls-min-version", "Minimum version TLS supported.") + flag.Var(&tlsMinVersionValue, "tls-min-version", "Minimum version TLS supported.") flag.StringVar(&configPath, "config", "config.yml", "Path to the configuration file.") flag.StringVar(&httpProfAddress, "httpprof", "", "Enable HTTP profiler listening on the given address.") // This flag is experimental and might be removed in the future or renamed @@ -118,7 +120,7 @@ func main() { log.Fatal(err) } - if tlsMinVersion != "" { + if tlsMinVersionValue.String() != "" { if tlsCertFile == "" || tlsKeyFile == "" { log.Fatalf("-tls-min-version set but missing TLS cert and key files (-tls-cert and -tls-key)") } @@ -255,12 +257,11 @@ func initServer(logger *zap.Logger, apmTracer *apm.Tracer, config *Config) *http router := mustLoadRouter(logger, config, indexer) apmgorilla.Instrument(router, apmgorilla.WithTracer(apmTracer)) - if tlsMinVersion == "" { - return &http.Server{Addr: address, Handler: router} + var tlsConfig tls.Config + if tlsMinVersionValue.String() != "" { + tlsConfig.MinVersion = tlsMinVersionValue.Value() } - - TLSConfig := &tls.Config{MinVersion: tlsMinVersionCode} - return &http.Server{Addr: address, Handler: router, TLSConfig: TLSConfig} + return &http.Server{Addr: address, Handler: router, TLSConfig: &tlsConfig} } func runServer(server *http.Server) error { From 1f107e5796d388673f8644902aef3390acd9100e Mon Sep 17 00:00:00 2001 From: Mario Rodriguez Molins Date: Thu, 26 Oct 2023 11:40:10 +0200 Subject: [PATCH 5/8] Add bugfix changelog entry too --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51941b836..25107fd92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Bugfixes * Update Go runtime to 1.21.3. [#1102](https://github.com/elastic/package-registry/pull/1102) +* Raise an error if the value of environment variables used to set parameters are not valid [#1103](https://github.com/elastic/package-registry/pull/1103) ### Added From 832beec4392ce6c197b8f954abe610192639be3b Mon Sep 17 00:00:00 2001 From: Mario Rodriguez Molins Date: Thu, 26 Oct 2023 12:17:23 +0200 Subject: [PATCH 6/8] Remove unnecesary version code variable --- flags.go | 13 +++++-------- main.go | 3 --- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/flags.go b/flags.go index 97cfe4167..12eb3105f 100644 --- a/flags.go +++ b/flags.go @@ -21,14 +21,11 @@ var supportedTLSVersions map[string]uint16 = map[string]uint16{ } type tlsVersionValue struct { - version *uint16 + version uint16 } func (t tlsVersionValue) String() string { - if t.version == nil { - return "" - } - switch *t.version { + switch t.version { case tls.VersionTLS10: return "1.0" case tls.VersionTLS11: @@ -43,14 +40,14 @@ func (t tlsVersionValue) String() string { } func (t tlsVersionValue) Value() uint16 { - return *t.version + return t.version } -func (t tlsVersionValue) Set(s string) error { +func (t *tlsVersionValue) Set(s string) error { if _, ok := supportedTLSVersions[s]; !ok { return fmt.Errorf("unsupported TLS version: %s", s) } - *t.version = supportedTLSVersions[s] + t.version = supportedTLSVersions[s] return nil } diff --git a/main.go b/main.go index 1a828aa17..52943b503 100644 --- a/main.go +++ b/main.go @@ -54,7 +54,6 @@ var ( tlsCertFile string tlsKeyFile string - tlsMinVersionCode uint16 tlsMinVersionValue tlsVersionValue dryRun bool @@ -79,8 +78,6 @@ var ( ) func init() { - tlsMinVersionValue = tlsVersionValue{version: &tlsMinVersionCode} - flag.BoolVar(&printVersionInfo, "version", false, "Print Elastic Package Registry version") flag.StringVar(&address, "address", "localhost:8080", "Address of the package-registry service.") flag.StringVar(&metricsAddress, "metrics-address", "", "Address to expose the Prometheus metrics (experimental). ") From 0cc2d18356e50b8eb520b097ba5109e8197ef1c8 Mon Sep 17 00:00:00 2001 From: Mario Rodriguez Molins Date: Thu, 26 Oct 2023 15:51:12 +0200 Subject: [PATCH 7/8] Use a uint16 instead of a struct for the flag.Var --- flags.go | 12 +++--------- main.go | 4 ++-- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/flags.go b/flags.go index 12eb3105f..802fe00bc 100644 --- a/flags.go +++ b/flags.go @@ -20,12 +20,10 @@ var supportedTLSVersions map[string]uint16 = map[string]uint16{ "1.3": tls.VersionTLS13, } -type tlsVersionValue struct { - version uint16 -} +type tlsVersionValue uint16 func (t tlsVersionValue) String() string { - switch t.version { + switch t { case tls.VersionTLS10: return "1.0" case tls.VersionTLS11: @@ -39,15 +37,11 @@ func (t tlsVersionValue) String() string { } } -func (t tlsVersionValue) Value() uint16 { - return t.version -} - func (t *tlsVersionValue) Set(s string) error { if _, ok := supportedTLSVersions[s]; !ok { return fmt.Errorf("unsupported TLS version: %s", s) } - t.version = supportedTLSVersions[s] + *t = tlsVersionValue(supportedTLSVersions[s]) return nil } diff --git a/main.go b/main.go index 52943b503..cc4d82a4b 100644 --- a/main.go +++ b/main.go @@ -255,8 +255,8 @@ func initServer(logger *zap.Logger, apmTracer *apm.Tracer, config *Config) *http apmgorilla.Instrument(router, apmgorilla.WithTracer(apmTracer)) var tlsConfig tls.Config - if tlsMinVersionValue.String() != "" { - tlsConfig.MinVersion = tlsMinVersionValue.Value() + if tlsMinVersionValue > 0 { + tlsConfig.MinVersion = uint16(tlsMinVersionValue) } return &http.Server{Addr: address, Handler: router, TLSConfig: &tlsConfig} } From 0f68c67550cb4a914904889fac7ee0d39ac8653a Mon Sep 17 00:00:00 2001 From: Mario Rodriguez Molins Date: Thu, 26 Oct 2023 15:59:43 +0200 Subject: [PATCH 8/8] Remove usage of String() --- main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.go b/main.go index cc4d82a4b..9569b3563 100644 --- a/main.go +++ b/main.go @@ -117,7 +117,7 @@ func main() { log.Fatal(err) } - if tlsMinVersionValue.String() != "" { + if tlsMinVersionValue > 0 { if tlsCertFile == "" || tlsKeyFile == "" { log.Fatalf("-tls-min-version set but missing TLS cert and key files (-tls-cert and -tls-key)") }