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

crypto/tls: add VersionName function to return a string version of the TLS Version #46308

Closed
skgsergio opened this issue May 21, 2021 · 18 comments
Assignees
Labels
Milestone

Comments

@skgsergio
Copy link

skgsergio commented May 21, 2021

I propose to add a func VersionName(version uint16) string method that returns a string representation (TLSv1.0, TLSv1.1, ...) given a version id.

Currently crypto/tls provides similar functionality for the cipher suites but not for the version. I've seen myself and others implementing this in our codes to print it in logs and other places, but this is far from ideal as, for example, if a new protocol is added in the future, that code would be outdated.

I couldn't find any related issue or ongoing work so I'll be opening a PR.

skgsergio added a commit to skgsergio/go that referenced this issue May 21, 2021
This function returns an string representation (`TLSv1.0`, `TLSv1.1`, ...)
given a TLS version number.

Fixes golang#46308
skgsergio added a commit to skgsergio/go that referenced this issue May 21, 2021
This function returns an string representation (`TLSv1.0`, `TLSv1.1`, ...)
given a TLS version number.

Fixes golang#46308
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/321733 mentions this issue: crypto/tls: add crypto/tls.VersionName function

@dmitshur dmitshur added FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 21, 2021
@dmitshur dmitshur added this to the Backlog milestone May 21, 2021
@dmitshur
Copy link
Contributor

@networkimprov
Copy link

cc @ianlancetaylor re likely proposal

@ianlancetaylor ianlancetaylor changed the title crypto/tls: Add VersionName to return a string version of the TLS Version crypto/tls: add VersionName function to return a string version of the TLS Version May 24, 2021
@ianlancetaylor ianlancetaylor changed the title crypto/tls: add VersionName function to return a string version of the TLS Version proposal: crypto/tls: add VersionName function to return a string version of the TLS Version May 24, 2021
@ianlancetaylor ianlancetaylor modified the milestones: Backlog, Proposal May 24, 2021
@rsc
Copy link
Contributor

rsc commented Nov 10, 2021

@rsc
Copy link
Contributor

rsc commented Nov 10, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Dec 1, 2021

@rolandshoemaker
Copy link
Member

Generally seems reasonable.

@rsc
Copy link
Contributor

rsc commented Dec 8, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Dec 15, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: crypto/tls: add VersionName function to return a string version of the TLS Version crypto/tls: add VersionName function to return a string version of the TLS Version Dec 15, 2021
@rsc rsc modified the milestones: Proposal, Backlog Dec 15, 2021
@FiloSottile FiloSottile removed this from the Backlog milestone Mar 2, 2022
@FiloSottile FiloSottile added this to the Go1.19 milestone Mar 2, 2022
@FiloSottile FiloSottile self-assigned this Mar 2, 2022
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 5, 2022
@mvdan
Copy link
Member

mvdan commented Mar 14, 2022

Was a String method considered? I imagine that the problem might be that making the constants use a named type rather than be left as untyped integers might break some users, but I can't help but feel like a top-level "to string" function feels out of place :)

@FiloSottile
Copy link
Contributor

Is there a way to convert the constants to a named type within the Go 1 Compatibility Promise?

@mt-inside
Copy link

If there could be a named type someone, a String() method (and thereby satisfying fmt.Stringer) would play very nicely with go 1.18 generics...

@ianlancetaylor
Copy link
Contributor

CC @golang/security

Do we want to try to get this into 1.19? Otherwise, what should the milestone be?

@FiloSottile Converting an untyped constant to a named type technically violates the Go 1 compatibility guarantee.

@rolandshoemaker
Copy link
Member

Seems unlikely to happen for 1.19, moving to 1.20 seems fine.

@rolandshoemaker rolandshoemaker modified the milestones: Go1.19, Go1.20 Jun 24, 2022
@ehaughee
Copy link

ehaughee commented Dec 22, 2022

I am writing my own stop-gap for this functionality but would like to swap that impl for this proposal once available. Is there an inclination/decision towards the expected string value/pattern for these names? For instance, will it match the CipherSuiteName pattern of being a direct map of const name to string value (e.g. tls.VersionSSL30 becomes "VersionSSL30"?

@skgsergio
Copy link
Author

skgsergio commented Dec 22, 2022

When I did the PR I used the names that are most common everywhere for representing each version instead of using the constant name.

@gopherbot gopherbot modified the milestones: Go1.20, Go1.21 Feb 1, 2023
@s-silvius

This comment was marked as duplicate.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/497377 mentions this issue: crypto/tls: add VersionName

@golang golang locked and limited conversation to collaborators May 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.