expose MinTLSVersion config for TLS handshake #3103
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
timraymond
reviewed
Nov 4, 2024
cns/service.go
Outdated
|
|
||
| // TLSVersionNumber returns the version number for the provided TLS version name | ||
| // (e.g. 0x0301) | ||
| func TLSVersionNumber(versionName string) (uint16, error) { |
There was a problem hiding this comment.
I am surprised the standard library doesn't have a parse function for this. I would change this to parseTLSVersionNumber since that's the convention you would find there.
cns/service.go
Outdated
| case "TLS 1.3": | ||
| return tls.VersionTLS13, nil | ||
| default: | ||
| return 0, errors.Wrap(errBootConfig, "unsupported TLS version name") |
There was a problem hiding this comment.
"configuration error" isn't adding a whole lot of information here. I think you can just have unsupported TLS version name as the constantized error, then include what you tried to parse (the versionName arg) in the Wrapf.
cns/service.go
Outdated
| @@ -226,8 +232,13 @@ func getTLSConfigFromKeyVault(tlsSettings localtls.TlsSettings, errChan chan<- e | |||
| errChan <- cr.Refresh(ctx, tlsSettings.KeyVaultCertificateRefreshInterval) | |||
| }() | |||
|
|
|||
| minTLSVersionNumber, err := TLSVersionNumber(tlsSettings.MinTLSVersion) | |||
| if err != nil { | |||
| return nil, errors.Wrap(err, "MinTLSVersion is not valid") | |||
There was a problem hiding this comment.
Usually, for error wrapping, I put what I was trying to do when the error emerged. Here it would be:
Suggested change
| return nil, errors.Wrap(err, "MinTLSVersion is not valid") | |
| return nil, errors.Wrap(err, "parsing MinTLSVersion from config") |
Doing this gives you a progressive chain of explanation.
jackieluc
requested changes
Nov 5, 2024
jackieluc
approved these changes
Nov 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reason for Change:
A configuration change is required in CNS to be able to specify the optional config of minimum TLS version support desired for TLS handshake
Issue Fixed:
Requirements:
Notes: