Add tls-min-version parameter #1103

mrodm · 2023-10-25T14:21:31Z

Fixes #965

This PR adds a new parameter (and a new environment variable) to define the minimum TLS version supported.

If -tls-min-version is set, but -tls-cert and -tls-key are not specified , requests fail to package-registry service like this:

 $ curl -k -v -I --tls-max 1.3 https://localhost:8080
*   Trying 127.0.0.1:8080...
* Connected to localhost (127.0.0.1) port 8080 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* TLSv1.0 (OUT), TLS header, Certificate Status (22):
* TLSv1.3 (OUT), TLS handshake, Client hello (1):
* (5454) (IN), , Unknown (72):
* error:0A00010B:SSL routines::wrong version number
* Closing connection 0
curl: (35) error:0A00010B:SSL routines::wrong version number

How to test this

create server certificate and key (accept all the default values)

mkdir -p certs
cd certs

# create CA
openssl genrsa -out servercakey.pem
openssl req -new -x509 -key servercakey.pem -out serverca.crt

# create Server certificates
openssl genrsa -out server.key
openssl req -new -key server.key -out server_reqout.txt
openssl x509 -req -in server_reqout.txt -days 3650 -sha256 -CAcreateserial -CA serverca.crt -CAkey servercakey.pem -out server.crt

Create docker image

docker rmi docker.elastic.co/package-registr/package-registry:tls-test || true
docker build --no-cache -t docker.elastic.co/package-registry/package-registry:tls-test -f Dockerfile .

Run docker image with the required parameters

# All parameters
docker run --rm -p 8080:8080 -v $(pwd)/certs:/root/certs/ -e EPR_LOG_LEVEL=debug -e EPR_FEATURE_PROXY_MODE=true -e EPR_PROXY_TO=https://epr.elastic.co -e EPR_TLS_MIN_VERSION=1.2 -v /home/mariorodriguez/Coding/work/integrations/build/packages/:/packages/package-registry -it docker.elastic.co/package-registry/package-registry:tls-test

# Just TLS_MIN_VERSION (it must fail)
docker run --rm -p 8080:8080 -v $(pwd)/certs:/root/certs/ -e EPR_LOG_LEVEL=debug -e EPR_FEATURE_PROXY_MODE=true -e EPR_PROXY_TO=https://epr.elastic.co -e EPR_TLS_MIN_VERSION=1.2 -v /home/mariorodriguez/Coding/work/integrations/build/packages/:/packages/package-registry -it docker.elastic.co/package-registry/package-registry:tls-test

Test with curl

curl -k -v -I --tls-max 1.3 https://localhost:8080
curl -k -v -I --tls-max 1.2 https://localhost:8080
curl -k -v -I --tls-max 1.1 https://localhost:8080
curl -k -v -I --tls-max 1.0 https://localhost:8080

For instance, if package-registry is configured with EPR_TLS_MIN_VERSION=1.2 (or -tls-min-version 1.2):

 $ curl -k -I --tls-max 1.2 https://localhost:8080
HTTP/2 200 
cache-control: max-age=10
cache-control: public
content-type: application/json
content-length: 72
date: Wed, 25 Oct 2023 14:27:08 GMT

 $ curl -k -I --tls-max 1.3 https://localhost:8080
HTTP/2 200 
cache-control: max-age=10
cache-control: public
content-type: application/json
content-length: 72
date: Wed, 25 Oct 2023 14:27:22 GMT

 $ curl -k -I --tls-max 1.1 https://localhost:8080
curl: (35) error:0A0000BF:SSL routines::no protocols available

mrodm · 2023-10-25T16:32:58Z

flags.go

+			if err := f.Value.Set(value); err != nil {
+				flagErrors = errors.Join(flagErrors, fmt.Errorf("failed to set -%s: %v", f.Name, err))
+			}


Previously, a wrong value set in an Environment variable was not raising an error
For instance, this was not failing:

$ EPR_FEATURE_STORAGE_INDEXER=aaddb ./package-registry`

And now, that same command would raise an error:

$ EPR_FEATURE_STORAGE_INDEXER=aaddb ./package-registry 2023/10/25 18:29:20 failed to set -feature-storage-indexer: parse error

CHANGELOG.md

mrodm · 2023-10-25T16:35:09Z

main.go

+	tlsMinVersion     string
+	tlsMinVersionCode uint16


Keep two values to be easier to compare if it is set or not this parameter.

Umm, why not using a tls value directly here? We would also have the members for further uses.

Suggested change

tlsMinVersion string

tlsMinVersionCode uint16

tlsMinVersion *tlsMinVersionValue

It could implement an IsZero() bool method to make it easy to check if it is set or not 🙂

jsoriano

Looks good, added only some nitpicking.

Btw, not related to this PR, but I have seen in the logs that the server errors are logged using a different format, I wonder if we could use the same logger for everything:

{"log.level":"info","@timestamp":"2023-10-25T17:43:33.554Z","log.logger":"http","message":"HEAD / HTTP/2.0","source.address":"172.17.0.1","http.request.method":"HEAD","url.path":"/","url.domain":"localhost","http.response.code":200,"http.response.body.bytes":72,"event.duration":29288,"source.ip":"172.17.0.1","user_agent.original":"curl/7.81.0","url.port":8080,"ecs.version":"1.6.0"}
2023/10/25 17:43:39 http: TLS handshake error from 172.17.0.1:53032: remote error: tls: protocol version not supported
2023/10/25 17:43:43 http: TLS handshake error from 172.17.0.1:37008: remote error: tls: protocol version not supported

flags.go

jsoriano · 2023-10-25T17:19:30Z

main.go

+	tlsMinVersion     string
+	tlsMinVersionCode uint16


Umm, why not using a tls value directly here? We would also have the members for further uses.

Suggested change

tlsMinVersion string

tlsMinVersionCode uint16

tlsMinVersion *tlsMinVersionValue

It could implement an IsZero() bool method to make it easy to check if it is set or not 🙂

main.go

CHANGELOG.md

jsoriano · 2023-10-25T17:36:06Z

flags.go

+func (t tlsMinVersionValue) String() string {
+	if t.version != nil {
+		return *t.version
+	}
+	return ""
+}


If we store only the code in the value, we could have also a map here to convert it back to string.

Suggested change

func (t tlsMinVersionValue) String() string {

if t.version != nil {

return *t.version

}

return ""

}

func (t tlsMinVersionValue) String() string {

switch tlsMinVersionValue {

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 ""

}

}

mrodm · 2023-10-26T10:28:24Z

Tested with the nmap command:

For instance, using -tls-min-version 1.3

PORT     STATE SERVICE
8080/tcp open  http-proxy
| ssl-enum-ciphers: 
|   TLSv1.3: 
|     ciphers: 
|       TLS_AKE_WITH_AES_128_GCM_SHA256 (ecdh_x25519) - A
|       TLS_AKE_WITH_AES_256_GCM_SHA384 (ecdh_x25519) - A
|       TLS_AKE_WITH_CHACHA20_POLY1305_SHA256 (ecdh_x25519) - A
|     cipher preference: server
|_  least strength: A

Nmap done: 1 IP address (1 host up) scanned in 0.18 seconds

Other example with -tls-min-version 1.1:

PORT     STATE SERVICE
8080/tcp open  http-proxy
| ssl-enum-ciphers: 
|   TLSv1.1: 
|     ciphers: 
|       TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (secp256r1) - A
|       TLS_RSA_WITH_AES_128_CBC_SHA (rsa 2048) - A
|       TLS_RSA_WITH_AES_256_CBC_SHA (rsa 2048) - A
|       TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA (secp256r1) - C
|       TLS_RSA_WITH_3DES_EDE_CBC_SHA (rsa 2048) - C
|     compressors: 
|       NULL
|     cipher preference: server
|     warnings: 
|       64-bit block cipher 3DES vulnerable to SWEET32 attack
|   TLSv1.2: 
|     ciphers: 
|       TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (secp256r1) - A
|       TLS_RSA_WITH_AES_128_GCM_SHA256 (rsa 2048) - A
|       TLS_RSA_WITH_AES_256_GCM_SHA384 (rsa 2048) - A
|       TLS_RSA_WITH_AES_128_CBC_SHA (rsa 2048) - A
|       TLS_RSA_WITH_AES_256_CBC_SHA (rsa 2048) - A
|       TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA (secp256r1) - C
|       TLS_RSA_WITH_3DES_EDE_CBC_SHA (rsa 2048) - C
|     compressors: 
|       NULL
|     cipher preference: server
|     warnings: 
|       64-bit block cipher 3DES vulnerable to SWEET32 attack
|   TLSv1.3: 
|     ciphers: 
|       TLS_AKE_WITH_AES_128_GCM_SHA256 (ecdh_x25519) - A
|       TLS_AKE_WITH_AES_256_GCM_SHA384 (ecdh_x25519) - A
|       TLS_AKE_WITH_CHACHA20_POLY1305_SHA256 (ecdh_x25519) - A
|     cipher preference: server
|_  least strength: C

Nmap done: 1 IP address (1 host up) scanned in 0.24 seconds

jsoriano

👍

jsoriano · 2023-10-26T11:58:39Z

flags.go

+type tlsVersionValue struct {
+	version uint16
 }


Could it be directly an int?

Suggested change

type tlsVersionValue struct {

version uint16

}

type tlsVersionValue uint16

jsoriano · 2023-10-26T11:59:27Z

main.go

@@ -118,7 +117,7 @@ func main() {
 		log.Fatal(err)
 	}

-	if tlsMinVersion != "" {
+	if tlsMinVersionValue.String() != "" {


Or

Suggested change

if tlsMinVersionValue.String() != "" {

if tlsMinVersionValue > 0 {

Could this one be changed as well as the other one below?

I missed this, sure

jsoriano · 2023-10-26T13:53:18Z

main.go

@@ -118,7 +117,7 @@ func main() {
 		log.Fatal(err)
 	}

-	if tlsMinVersion != "" {
+	if tlsMinVersionValue.String() != "" {


Could this one be changed as well as the other one below?

mrodm · 2023-10-26T14:06:21Z

Btw, not related to this PR, but I have seen in the logs that the server errors are logged using a different format, I wonder if we could use the same logger for everything:

{"log.level":"info","@timestamp":"2023-10-25T17:43:33.554Z","log.logger":"http","message":"HEAD / HTTP/2.0","source.address":"172.17.0.1","http.request.method":"HEAD","url.path":"/","url.domain":"localhost","http.response.code":200,"http.response.body.bytes":72,"event.duration":29288,"source.ip":"172.17.0.1","user_agent.original":"curl/7.81.0","url.port":8080,"ecs.version":"1.6.0"}
2023/10/25 17:43:39 http: TLS handshake error from 172.17.0.1:53032: remote error: tls: protocol version not supported
2023/10/25 17:43:43 http: TLS handshake error from 172.17.0.1:37008: remote error: tls: protocol version not supported

Created issue for this
#1104

elasticmachine · 2023-10-26T14:08:56Z

💚 Build Succeeded

Buildkite Build
Commit: 0f68c67

History

💚 Build #407 succeeded 0cc2d18
💚 Build #406 succeeded 832beec
💚 Build #405 succeeded 1f107e5
💚 Build #404 succeeded 52b742f
💚 Build #403 succeeded 22a02e3
💚 Build #402 succeeded 13ad34d

cc @mrodm

Add tls-min-version parameter

13ad34d

mrodm self-assigned this Oct 25, 2023

Move to flag.Var the tlsMinVersion parameter

22a02e3

mrodm commented Oct 25, 2023

View reviewed changes

Add changelog entry

52b742f

mrodm commented Oct 25, 2023

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

mrodm commented Oct 25, 2023

View reviewed changes

mrodm marked this pull request as ready for review October 25, 2023 16:43

mrodm requested a review from a team October 25, 2023 16:43

jsoriano approved these changes Oct 25, 2023

View reviewed changes

mrodm added 3 commits October 26, 2023 11:38

Changes from code review

5c6827e

Add bugfix changelog entry too

1f107e5

Remove unnecesary version code variable

832beec

mrodm requested a review from jsoriano October 26, 2023 10:29

jsoriano approved these changes Oct 26, 2023

View reviewed changes

Use a uint16 instead of a struct for the flag.Var

0cc2d18

jsoriano approved these changes Oct 26, 2023

View reviewed changes

Remove usage of String()

0f68c67

mrodm mentioned this pull request Oct 26, 2023

Unify log messages style for all messages #1104

Open

mrodm merged commit 1b3c3c0 into elastic:main Oct 26, 2023
1 check passed

mrodm deleted the add_min_tls_version branch October 26, 2023 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tls-min-version parameter #1103

Add tls-min-version parameter #1103

mrodm commented Oct 25, 2023 •

edited

Loading

mrodm Oct 25, 2023

mrodm Oct 25, 2023

jsoriano Oct 25, 2023

jsoriano left a comment

jsoriano Oct 25, 2023

jsoriano Oct 25, 2023

mrodm commented Oct 26, 2023

jsoriano left a comment

jsoriano Oct 26, 2023

jsoriano Oct 26, 2023

jsoriano Oct 26, 2023

mrodm Oct 26, 2023

jsoriano Oct 26, 2023

mrodm commented Oct 26, 2023

elasticmachine commented Oct 26, 2023

	tlsMinVersion string
	tlsMinVersionCode uint16
	tlsMinVersion *tlsMinVersionValue

-func (t tlsMinVersionValue) String() string {
-	if t.version != nil {
-		return *t.version
-	}
-	return ""
-}
+func (t tlsMinVersionValue) String() string {
+   switch tlsMinVersionValue {
+   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 ""
+   }
+}

	if tlsMinVersionValue.String() != "" {
	if tlsMinVersionValue > 0 {

Add tls-min-version parameter #1103

Add tls-min-version parameter #1103

Conversation

mrodm commented Oct 25, 2023 • edited Loading

How to test this

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrodm commented Oct 26, 2023

jsoriano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrodm commented Oct 26, 2023

elasticmachine commented Oct 26, 2023

💚 Build Succeeded

History

mrodm commented Oct 25, 2023 •

edited

Loading