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

Jump to bottom

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 · May be fixed by #46309
Closed

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

skgsergio opened this issue May 21, 2021 · 18 comments · May be fixed by #46309
Assignees
@FiloSottile
Labels
FeatureRequest NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Projects
Proposals (old)
Milestone
Go1.21

Comments

@skgsergio
Copy link

skgsergio commented May 21, 2021 •
edited

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
@skgsergio
crypto/tls: add crypto/tls.VersionName function
1b9e25a 
This function returns an string representation (`TLSv1.0`, `TLSv1.1`, ...)
given a TLS version number.

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

Fixes golang#46308
@gopherbot
Copy link

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

CC @FiloSottile, @katiehockman, @rolandshoemaker via owners.

@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
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) May 24, 2021
@rsc
Copy link
Contributor

rsc commented Nov 10, 2021

ping @FiloSottile @rolandshoemaker @katiehockman

@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 rsc moved this from Incoming to Active in Proposals (old) Nov 10, 2021
@rsc
Copy link
Contributor

rsc commented Dec 1, 2021

ping @FiloSottile @rolandshoemaker @katiehockman

@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 rsc moved this from Active to Likely Accept in Proposals (old) Dec 8, 2021
@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Dec 15, 2021
@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 •
edited

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 •
edited

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.

Sign in to view
@gopherbot
Copy link

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

@mknyszek mknyszek mentioned this issue Jun 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@FiloSottile FiloSottile

Labels
FeatureRequest NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Projects
Proposals
Status: Accepted
Release Status & Release Blockers
Status: Done
Proposals (old)
Accepted
Milestone
Go1.21
Development

Successfully merging a pull request may close this issue.

crypto/tls: add crypto/tls.VersionName function skgsergio/go