feat: allow configuring min tls for grpc #6320
Conversation
or-shachar
force-pushed
the
ors/grpc-min-tls
branch
from
52b4a84
to
4e822dd
Compare
| @@ -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.
we should imho return err here
There was a problem hiding this comment.
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.
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.
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.
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.
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.
| string(TLS10): ctls.VersionTLS10, | ||
| string(TLS11): ctls.VersionTLS11, |
There was a problem hiding this comment.
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.
yes, probably makes more sense
There was a problem hiding this comment.
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.
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.
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))
}
There was a problem hiding this comment.
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)
or-shachar
force-pushed
the
ors/grpc-min-tls
branch
2 times, most recently
from
ae7defe
to
873f7f5
Compare
Signed-off-by: Or Shachar <[email protected]>
or-shachar
force-pushed
the
ors/grpc-min-tls
branch
from
873f7f5
to
3e2e6fa
Compare
Supporting kedacore/keda#6320 Signed-off-by: Or Shachar <[email protected]>
Supporting kedacore/keda#6320 Signed-off-by: Or Shachar <[email protected]> Signed-off-by: Or Shachar <[email protected]>
|
/run-e2e internal |
This will allow configuring grpc min grpc version very similarly to how we allow configuring the HTTP version.
Checklist
Fixes #6270