-
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, | ||
wozniakjan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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. 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. this is still pending, correct? |
||
} | ||
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.
Please refer an issue here