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

support tls 1.3 #7325

Merged
merged 4 commits into from
Feb 19, 2020
Merged

support tls 1.3 #7325

merged 4 commits into from
Feb 19, 2020

Conversation

hanshasselberg
Copy link
Member

@hanshasselberg hanshasselberg commented Feb 19, 2020

Fixes #6784

Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

Seems good to me. I left the one comment but consider it non-blocking.

}

// TLSVersions has all the keys from the map above, make sure to keep both in sync
var TLSVersions = "tls10, tls11, tls12, tls13"
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we instead use an init func and programmatically generate this?

Copy link
Member Author

@hanshasselberg hanshasselberg Feb 19, 2020

Choose a reason for hiding this comment

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

Good catch! I went with a normal function instead. But the idea is the same.

}

// TLSVersions has all the keys from the map above, make sure to keep both in sync
var TLSVersions = "tls10, tls11, tls12, tls13"
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to have a function iterating over the keys again and again. And then I had a var with []string, but given we only ever join, I figured it could be a string as well in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

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

And I think it is better now because both vars are colocated and it is easier to spot the dependency than before.

Copy link
Member

Choose a reason for hiding this comment

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

var TLSVersions string
func init() {
   var versions []string
   for version, _ := range TLSLookup {
      versions = append(versions, version)
   }
   TLSVersions = strings.Join(versions, ", ")
}

Copy link
Member

Choose a reason for hiding this comment

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

That should do a one time generation of the string. But like I said, it doesn't really matter. I doubt there will be a tls 1.4 any time soon. Its sort of like the copyright thing. You might change it once a year so is it worth keeping the code around rather than just updating it very rarely?

@hanshasselberg hanshasselberg merged commit e05ac57 into master Feb 19, 2020
@hanshasselberg hanshasselberg deleted the tls13 branch February 19, 2020 22:22
hanshasselberg added a commit that referenced this pull request Feb 20, 2020
alvin-huang pushed a commit to alvin-huang/consul that referenced this pull request May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support TLS 1.3 in Go 1.13.x
2 participants