-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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, | ||
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) | ||
} |
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) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should imho return err here There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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)