Allow the controller configure tls-cipher-suites and tls-min-version via command line params #2083

klzsysy · 2023-09-14T06:10:41Z

This PR changes controller webhook server tls security config

changes:

Use secure tls-cipher-suites to solve CVE-2016-2183- Red Hat Customer Portal.
Allow configure tls-cipher-suites and tls-min-version via command line params, e.g. https://kubernetes.io/docs/reference/command-line-tools-reference/kube-apiserver/, If omitted, the default PreferredTLSCipherNames cipher suites will be used.

Before

> kubectl port-forward svc/metallb-webhook-service 1443:443 &
[1] 20151
Forwarding from 127.0.0.1:1443 -> 9443
Forwarding from [::1]:1443 -> 9443
Handling connection for 1443

> nmap --script ssl-enum-ciphers 1443 127.0.0.1
Starting Nmap 7.94 ( https://nmap.org ) at 2023-09-14 14:07 CST
Nmap scan report for localhost (127.0.0.1)
Host is up (0.00068s latency).
Not shown: 996 closed tcp ports (conn-refused)
PORT      STATE SERVICE
1080/tcp  open  socks
1443/tcp  open  ies-lm
| ssl-enum-ciphers:
|   TLSv1.0:
|     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.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
9090/tcp  open  zeus-admin
49161/tcp open  unknown

after

    spec:
      containers:
      - args:
        - --port=7472
        - --log-level=info
        - --cert-service-name=metallb-webhook-service
        - --tls-min-version=VersionTLS12
        env:
        - name: METALLB_ML_SECRET_NAME
          value: metallb-memberlist
        - name: METALLB_DEPLOYMENT
          value: metallb-controller
        - name: METALLB_BGP_TYPE
          value: frr
        image: registry.smtx.io/kubesmart-dev/metallb/controller:v0.13.11  # rebuild image

> kubectl port-forward svc/metallb-webhook-service 1443:443 &

> nmap --script ssl-enum-ciphers -p 1443 127.0.0.1
Starting Nmap 7.94 ( https://nmap.org ) at 2023-10-18 16:46 CST
Nmap scan report for localhost (127.0.0.1)
Host is up (0.0053s latency).

PORT     STATE SERVICE
1443/tcp open  ies-lm
| ssl-enum-ciphers:
|   TLSv1.2:
|     ciphers:
|       TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 (secp256r1) - A
|       TLS_RSA_WITH_AES_128_GCM_SHA256 (rsa 2048) - A
|       TLS_RSA_WITH_AES_256_GCM_SHA384 (rsa 2048) - A
|     compressors:
|       NULL
|     cipher preference: client
|   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 3.51 seconds

fedepaol · 2023-09-29T14:11:03Z

Thanks!
Mind fixing the conflicts?

fedepaol · 2023-09-29T14:13:12Z

Also, the diff in nmap you showed is with the default values right?

fedepaol · 2023-09-29T14:30:04Z

@yuvalk mind having a look here?

charts/metallb/values.yaml

klzsysy · 2023-10-05T13:27:44Z

Also, the diff in nmap you showed is with the default values right?

yes, Now updated to the results after using VersionTLS12

fedepaol · 2023-10-09T12:58:43Z

Thanks @klzsysy . Couple of other asks:

can you squash the commits so they are meaningful and we get a clean commit history?
can you apply the same change setting the defaults to the kustomize based deployment here ?

klzsysy · 2023-10-09T13:37:07Z

Thanks @klzsysy . Couple of other asks:

can you squash the commits so they are meaningful and we get a clean commit history?

can you apply the same change setting the defaults to the kustomize based deployment here ?

done

fedepaol · 2023-10-09T13:43:16Z

Thanks @klzsysy . Couple of other asks:

can you squash the commits so they are meaningful and we get a clean commit history?

can you apply the same change setting the defaults to the kustomize based deployment here ?

done

thanks!

fedepaol · 2023-10-09T13:52:31Z

need to run inv helmdocs to regenerate the docs for the helm parameters

klzsysy · 2023-10-09T13:56:28Z

need to run inv helmdocs to regenerate the docs for the helm parameters

The version of helm-docs I am using is too new and has been returned to the same version as ci.

inv generatemanifests updated

yuvalk · 2023-10-10T01:57:32Z

sorry for late comment here, but just had the time to check this.
re ciphersuites
it's better to follow NIST guidelines
https://wiki.mozilla.org/Security/Server_Side_TLS#Intermediate_compatibility_.28recommended.29
you must at least remove the cliflag.InsecureTLSCiphers() ones

klzsysy · 2023-10-10T02:13:12Z

OK, it is no problem to delete the default value of ciphersuites and let the users who need it configure it themselves.

yuvalk · 2023-10-10T02:17:33Z

OK, it is no problem to delete the default value of ciphersuites and let the users who need it configure it themselves.

question is, then what happens by default?
can you please update your after section in PR opening comment?

the idea is to have a sane default..
then of course, customer who need something else can configure it

klzsysy · 2023-10-10T02:27:06Z

OK, it is no problem to delete the default value of ciphersuites and let the users who need it configure it themselves.

question is, then what happens by default? can you please update your after section in PR opening comment?

the idea is to have a sane default.. then of course, customer who need something else can configure it

The effect of updating the default value has been updated in PR opening comment，You can see that the time of nmap output log has been updated.

It seems that leaving the default value blank has a good effect.

yuvalk · 2023-10-10T02:33:30Z

It seems that leaving the default value blank has a good effect.

then I dont think it's good enough, there are still insecure ciphers in that list

klzsysy · 2023-10-10T03:37:08Z

then I dont think it's good enough, there are still insecure ciphers in that list

so just follow the NIST guidelines ?
https://wiki.mozilla.org/Security/Server_Side_TLS#Intermediate_compatibility_.28recommended.29

# TLS Cipher suites
TLS_AES_128_GCM_SHA256
TLS_AES_256_GCM_SHA384
TLS_CHACHA20_POLY1305_SHA256
TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
TLS_RSA_WITH_AES_128_GCM_SHA256
TLS_RSA_WITH_AES_256_GCM_SHA384

# result
> nmap --script ssl-enum-ciphers -p 1443 127.0.0.1
Starting Nmap 7.94 ( https://nmap.org ) at 2023-10-18 16:46 CST
Nmap scan report for localhost (127.0.0.1)
Host is up (0.0053s latency).

PORT     STATE SERVICE
1443/tcp open  ies-lm
| ssl-enum-ciphers:
|   TLSv1.2:
|     ciphers:
|       TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (secp256r1) - A
|       TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 (secp256r1) - A
|       TLS_RSA_WITH_AES_128_GCM_SHA256 (rsa 2048) - A
|       TLS_RSA_WITH_AES_256_GCM_SHA384 (rsa 2048) - A
|     compressors:
|       NULL
|     cipher preference: client
|   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 3.51 seconds

klzsysy · 2023-10-18T08:47:13Z

@yuvalk
what do you think

klzsysy · 2023-10-24T07:26:16Z

@fedepaol
can you take a look, retested after resolving the conflict

fedepaol · 2023-10-27T12:56:24Z

looks good, waiting for one last ack from @yuvalk

yuvalk · 2023-10-27T13:07:12Z

@yuvalk what do you think

this is great,
thank you!

@fedepaol yeah, you have my ack

fedepaol · 2023-10-27T13:13:46Z

thanks, sending to merge

fedepaol · 2023-10-27T13:18:13Z

@klzsysy mind squashing before I merge?
Thanks!

klzsysy · 2023-10-30T04:15:33Z

@klzsysy mind squashing before I merge? Thanks!

done

fedepaol · 2023-11-17T12:26:49Z

Sorry I left this behind. I see now that needs rebasing? Mind doing so so I will merge?
Thanks again!

klzsysy · 2023-11-19T10:43:32Z

Sorry I left this behind. I see now that needs rebasing? Mind doing so so I will merge? Thanks again!

It should be possible to merge now

fedepaol · 2023-11-30T08:30:50Z

sent to the merge queue

…via command line params Set the default tls min version to VersionTLS12 Set the default tls cipher suites to mozilla's recommended values Signed-off-by: Siyi.Yang <siyi.yang@smartx.com> Co-authored-by: Federico Paolinelli <fpaoline@redhat.com>

klzsysy requested review from fedepaol, russellb, gclawes and oribon as code owners September 14, 2023 06:10

yuvalk reviewed Oct 2, 2023

View reviewed changes

charts/metallb/values.yaml Outdated Show resolved Hide resolved

klzsysy force-pushed the tls-security-config branch from 37f33b7 to 7edc962 Compare October 5, 2023 13:44

klzsysy force-pushed the tls-security-config branch from 39c46e2 to 06c48de Compare October 9, 2023 13:35

fedepaol enabled auto-merge October 9, 2023 13:44

auto-merge was automatically disabled October 9, 2023 13:52
Head branch was pushed to by a user without write access

klzsysy force-pushed the tls-security-config branch from 06c48de to 8b39cdf Compare October 9, 2023 13:52

fedepaol enabled auto-merge October 9, 2023 13:55

auto-merge was automatically disabled October 9, 2023 14:11
Head branch was pushed to by a user without write access

klzsysy force-pushed the tls-security-config branch from 8b39cdf to 5e81bfb Compare October 9, 2023 14:11

fedepaol enabled auto-merge October 9, 2023 15:46

auto-merge was automatically disabled October 10, 2023 02:14
Head branch was pushed to by a user without write access

fedepaol added this pull request to the merge queue Oct 27, 2023

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Oct 27, 2023

fedepaol added this pull request to the merge queue Oct 27, 2023

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Oct 27, 2023

klzsysy force-pushed the tls-security-config branch from 28a82b2 to 98bfd4e Compare October 30, 2023 04:14

fedepaol enabled auto-merge October 30, 2023 09:31

auto-merge was automatically disabled October 31, 2023 03:03
Head branch was pushed to by a user without write access

klzsysy force-pushed the tls-security-config branch from 98bfd4e to e73d08c Compare October 31, 2023 03:03

klzsysy force-pushed the tls-security-config branch from 613a6d4 to a48d171 Compare November 13, 2023 09:32

fedepaol enabled auto-merge November 17, 2023 09:49

fedepaol added this pull request to the merge queue Nov 17, 2023

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Nov 17, 2023

klzsysy force-pushed the tls-security-config branch from b2b91e9 to 74e8bd1 Compare November 19, 2023 10:42

klzsysy force-pushed the tls-security-config branch from 74e8bd1 to dc34ed1 Compare November 30, 2023 06:04

fedepaol enabled auto-merge November 30, 2023 08:30

fedepaol force-pushed the tls-security-config branch from dc34ed1 to 8ac05b1 Compare November 30, 2023 13:14

fedepaol added this pull request to the merge queue Nov 30, 2023

Merged via the queue into metallb:main with commit ebf28d0 Nov 30, 2023

Allow the controller configure tls-cipher-suites and tls-min-version via command line params #2083

Allow the controller configure tls-cipher-suites and tls-min-version via command line params #2083

Uh oh!

Conversation

klzsysy commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fedepaol commented Sep 29, 2023

Uh oh!

fedepaol commented Sep 29, 2023

Uh oh!

fedepaol commented Sep 29, 2023

Uh oh!

Uh oh!

klzsysy commented Oct 5, 2023

Uh oh!

fedepaol commented Oct 9, 2023

Uh oh!

klzsysy commented Oct 9, 2023

Uh oh!

fedepaol commented Oct 9, 2023

Uh oh!

fedepaol commented Oct 9, 2023

Uh oh!

klzsysy commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuvalk commented Oct 10, 2023

Uh oh!

klzsysy commented Oct 10, 2023

Uh oh!

yuvalk commented Oct 10, 2023

Uh oh!

klzsysy commented Oct 10, 2023

Uh oh!

yuvalk commented Oct 10, 2023

Uh oh!

klzsysy commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klzsysy commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klzsysy commented Oct 24, 2023

Uh oh!

fedepaol commented Oct 27, 2023

Uh oh!

yuvalk commented Oct 27, 2023

Uh oh!

fedepaol commented Oct 27, 2023

Uh oh!

fedepaol commented Oct 27, 2023

Uh oh!

klzsysy commented Oct 30, 2023

Uh oh!

fedepaol commented Nov 17, 2023

Uh oh!

klzsysy commented Nov 19, 2023

Uh oh!

fedepaol commented Nov 30, 2023

Uh oh!

Uh oh!

klzsysy commented Sep 14, 2023 •

edited

Loading

klzsysy commented Oct 9, 2023 •

edited

Loading

klzsysy commented Oct 10, 2023 •

edited

Loading

klzsysy commented Oct 18, 2023 •

edited

Loading