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

feat: allow configuring min tls for grpc #6320

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ Here is an overview of all new **experimental** features:

### Improvements

- **General**: Allow configuring minimum TLS version for GRPC client via `KEDA_GRPC_MIN_TLS_VERSION` ([#6320](https://github.com/kedacore/keda/pull/6320))
- TODO ([#XXX](https://github.com/kedacore/keda/issues/XXX))

### Fixes
Expand Down
54 changes: 54 additions & 0 deletions pkg/common/tls/tls.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package tls

import (
ctls "crypto/tls"
"fmt"
"os"
)

type tlsVersion string

const (
TLS10 tlsVersion = "TLS10"
TLS11 tlsVersion = "TLS11"
TLS12 tlsVersion = "TLS12"
TLS13 tlsVersion = "TLS13"
)

type tlsEnvVariableName string

const (
minHTTPTLSVersionEnv tlsEnvVariableName = "KEDA_HTTP_MIN_TLS_VERSION"
minGrpcTLSVersionEnv tlsEnvVariableName = "KEDA_GRPC_MIN_TLS_VERSION"
)

const (
defaultMinHTTPTLSVersion = TLS12
defaultMinGrpcTLSVersion = TLS13
)

func getMinTLSVersion(envKey tlsEnvVariableName, defaultVal tlsVersion) (uint16, error) {
version := string(defaultVal)
if val, ok := os.LookupEnv(string(envKey)); ok {
version = val
}
mapping := map[string]uint16{
string(TLS10): ctls.VersionTLS10,
string(TLS11): ctls.VersionTLS11,
Comment on lines +36 to +37
Copy link
Member

@wozniakjan wozniakjan Nov 7, 2024

Choose a reason for hiding this comment

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

not sure if it's a good idea to enable TLS 1.0 and 1.1, these are not considered secure anymore. If this code is getting refactored, maybe we remove these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, probably makes more sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though this will make this change a breaking change for those brave people who still wants to configure it with older TLS... 🫨
should we do it in a different PR?

Copy link
Member

Choose a reason for hiding this comment

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

I defer to @kedacore/keda-core-contributors for their opinion, maybe we announce deprecation first and then remove TLS 1.0 and 1.1 in KEDA 2.18 or 2.19?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah

Copy link
Contributor Author

@or-shachar or-shachar Nov 11, 2024

Choose a reason for hiding this comment

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

I can probably add a warning print in case TLS10 or TLS11 was requested:

if version == string(TLS11) || version == string(TLS10) {
		ctrl.Log.WithName("tls_setup").Info(fmt.Sprintf("Warning: Min TLS value %s is insecure and will be removed in KEDA 2.18. Please use TLS12 and above",  version))
	}

Copy link
Member

@JorTurFer JorTurFer Nov 23, 2024

Choose a reason for hiding this comment

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

I think that we should allow TLS10 until it's deprecated/removed, despite the security of it. We are using TLS1.2 as minimum by default, so I'd just keep them as optional for the users if they enable them (for HTTP, for gRPC is a new feature and we can use 1.2 or above only)

string(TLS12): ctls.VersionTLS12,
string(TLS13): ctls.VersionTLS13,
}
if v, ok := mapping[version]; ok {
return v, nil
}
fallback := mapping[string(defaultVal)]
return fallback, fmt.Errorf("invalid TLS version: %s, using %s", version, defaultVal)
}

func GetMinHTTPTLSVersion() (uint16, error) {
return getMinTLSVersion(minHTTPTLSVersionEnv, defaultMinHTTPTLSVersion)
}

func GetMinGrpcTLSVersion() (uint16, error) {
return getMinTLSVersion(minGrpcTLSVersionEnv, defaultMinGrpcTLSVersion)
}
81 changes: 81 additions & 0 deletions pkg/common/tls/tls_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package tls

import (
"crypto/tls"
"fmt"
"os"
"testing"
)

type minTLSVersionTestData struct {
name string
envSet bool
envValue string
expectedVersion uint16
shouldError bool
}

var minTLSVersionTestDatas = []minTLSVersionTestData{
{
name: "Set to TLS10",
envSet: true,
envValue: "TLS10",
expectedVersion: tls.VersionTLS10,
},
{
name: "Set to TLS11",
envSet: true,
envValue: "TLS11",
expectedVersion: tls.VersionTLS11,
},
{
name: "Set to TLS12",
envSet: true,
envValue: "TLS12",
expectedVersion: tls.VersionTLS12,
},
{
name: "Set to TLS13",
envSet: true,
envValue: "TLS13",
expectedVersion: tls.VersionTLS13,
},
{
name: "No setting",
envSet: false,
},
{
name: "Invalid settings",
envSet: true,
envValue: "TLS9",
shouldError: true,
},
}

func testResolveMinTLSVersion(t *testing.T, minVersionFunc func() (uint16, error), envName string, defaultVersion uint16) {
defer os.Unsetenv(envName)
for _, testData := range minTLSVersionTestDatas {
name := fmt.Sprintf("%s: %s", envName, testData.name)
t.Run(name, func(t *testing.T) {
os.Unsetenv(envName)
expectedVersion := defaultVersion
if testData.expectedVersion != 0 {
expectedVersion = testData.expectedVersion
}
if testData.envSet {
os.Setenv(envName, testData.envValue)
}
minVersion, err := minVersionFunc()
if testData.shouldError && err == nil {
t.Error("Expected error but got none")
}
if expectedVersion != minVersion {
t.Error("Failed to resolve minTLSVersion correctly", "wants", testData.expectedVersion, "got", minVersion)
}
})
}
}
func TestResolveMinTLSVersion(t *testing.T) {
testResolveMinTLSVersion(t, GetMinHTTPTLSVersion, "KEDA_HTTP_MIN_TLS_VERSION", tls.VersionTLS12)
testResolveMinTLSVersion(t, GetMinGrpcTLSVersion, "KEDA_GRPC_MIN_TLS_VERSION", tls.VersionTLS13)
}
9 changes: 8 additions & 1 deletion pkg/metricsservice/utils/tls.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ import (
"path"

"google.golang.org/grpc/credentials"
ctrl "sigs.k8s.io/controller-runtime"

kedatls "github.com/kedacore/keda/v2/pkg/common/tls"
)

// LoadGrpcTLSCredentials reads the certificate from the given path and returns TLS transport credentials
Expand All @@ -50,8 +53,12 @@ func LoadGrpcTLSCredentials(certDir string, server bool) (credentials.TransportC
}

// Create the credentials and return it
minTLSVersion, err := kedatls.GetMinGrpcTLSVersion()
if err != nil {
ctrl.Log.WithName("grpc_tls_setup").Info(err.Error())
Copy link
Member

Choose a reason for hiding this comment

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

we should imho return err here

Copy link
Contributor Author

@or-shachar or-shachar Nov 7, 2024

Choose a reason for hiding this comment

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

I followed the previous behavior that didn't fail the process for invalid value, but selected the default value + returned an error. I can change the function signature to not return an error and perform the logging in the function. wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

hm, I am not sure if we shouldn't just fail here. I mean, the grpc connection wouldn't be functional -> KEDA wouldn't be fully functional either.

WDYT @kedacore/keda-maintainers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not functional? The error only means that it fell back to default value because the configuration value was wrong.

Copy link
Member

@wozniakjan wozniakjan Nov 15, 2024

Choose a reason for hiding this comment

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

iiuc, if this returns error, KEDA will default back to TLS 1.3 as min for gRPC. So it might again cause difficulties with boring crypto?

I am also leaning towards failing here because that will tell the user their desired and configured min TLS version wasn't respected. They can then either fix the configuration or knowingly leave the default to KEDA.

Copy link
Contributor Author

@or-shachar or-shachar Nov 15, 2024

Choose a reason for hiding this comment

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

Ok. I'd make it fail here but I'll keep the same behavior for the HTTP client, because I don't want to introduce breaking change.

}
config := &tls.Config{
MinVersion: tls.VersionTLS13,
MinVersion: minTLSVersion,
Certificates: []tls.Certificate{cert},
}
if server {
Expand Down
22 changes: 3 additions & 19 deletions pkg/util/tls_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@ import (
"crypto/x509"
"encoding/pem"
"fmt"
"os"

"github.com/youmark/pkcs8"
ctrl "sigs.k8s.io/controller-runtime"

kedatls "github.com/kedacore/keda/v2/pkg/common/tls"
)

var minTLSVersion uint16
Expand Down Expand Up @@ -89,24 +90,7 @@ func GetMinTLSVersion() uint16 {
}

func initMinTLSVersion() (uint16, error) {
version, _ := os.LookupEnv("KEDA_HTTP_MIN_TLS_VERSION")

switch version {
case "":
minTLSVersion = tls.VersionTLS12
case "TLS10":
minTLSVersion = tls.VersionTLS10
case "TLS11":
minTLSVersion = tls.VersionTLS11
case "TLS12":
minTLSVersion = tls.VersionTLS12
case "TLS13":
minTLSVersion = tls.VersionTLS13
default:
return tls.VersionTLS12, fmt.Errorf("%s is not a valid value, using `TLS12`. Allowed values are: `TLS13`,`TLS12`,`TLS11`,`TLS10`", version)
}

return minTLSVersion, nil
return kedatls.GetMinHTTPTLSVersion()
}

func decryptClientKey(clientKey, clientKeyPassword string) ([]byte, error) {
Expand Down
50 changes: 0 additions & 50 deletions pkg/util/tls_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@ limitations under the License.
package util

import (
"crypto/tls"
"crypto/x509"
"os"
"strings"
"testing"
)
Expand Down Expand Up @@ -252,51 +250,3 @@ func TestNewTLSConfig_WithPassword(t *testing.T) {
})
}
}

type minTLSVersionTestData struct {
envSet bool
envValue string
expectedVersion uint16
}

var minTLSVersionTestDatas = []minTLSVersionTestData{
{
envSet: true,
envValue: "TLS10",
expectedVersion: tls.VersionTLS10,
},
{
envSet: true,
envValue: "TLS11",
expectedVersion: tls.VersionTLS11,
},
{
envSet: true,
envValue: "TLS12",
expectedVersion: tls.VersionTLS12,
},
{
envSet: true,
envValue: "TLS13",
expectedVersion: tls.VersionTLS13,
},
{
envSet: false,
expectedVersion: tls.VersionTLS12,
},
}

func TestResolveMinTLSVersion(t *testing.T) {
defer os.Unsetenv("KEDA_HTTP_MIN_TLS_VERSION")
for _, testData := range minTLSVersionTestDatas {
os.Unsetenv("KEDA_HTTP_MIN_TLS_VERSION")
if testData.envSet {
os.Setenv("KEDA_HTTP_MIN_TLS_VERSION", testData.envValue)
}
minVersion, _ := initMinTLSVersion()

if testData.expectedVersion != minVersion {
t.Error("Failed to resolve minTLSVersion correctly", "wants", testData.expectedVersion, "got", minVersion)
}
}
}
Loading