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: implement Hash.String #33430

Closed
Ma124 opened this issue Aug 2, 2019 · 13 comments
Closed

crypto: implement Hash.String #33430

Ma124 opened this issue Aug 2, 2019 · 13 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Proposals (old)
Milestone
Go1.15

Comments

@Ma124
Copy link

Ma124 commented Aug 2, 2019 •
edited

What version of Go are you using (go version)?

$ go version
go version go1.12.1 linux/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ma_124/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ma_124/.go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build821474858=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I wanted to iterate all builtin hash functions by doing something like the following:

var algs map[string]crypto.Hash

var h crypto.Hash
for ; h < crypto.maxHash; h++ {
    if h.Available() {
        algs[h.String()] = h
    }
}

What did you expect to see?

An exported crypto.maxHash and crypto.Hash implementing the fmt.Stringer interface.
@odeke-em odeke-em changed the title crypto: crypto.Hash implement fmt.Stringer and export crypto.maxHash proposal: crypto: crypto.Hash implement fmt.Stringer and export crypto.maxHash Aug 3, 2019
@gopherbot gopherbot added this to the Proposal milestone Aug 3, 2019
@andybons andybons added the Proposal-Crypto Proposal related to crypto packages or other security issues label Aug 6, 2019
@andybons
Copy link
Member

andybons commented Aug 6, 2019

@FiloSottile

@andybons
Copy link
Member

andybons commented Aug 6, 2019 •
edited

@Ma124 you mention what you want to do, but don’t explain why you or others would need this functionality. Could you elaborate a bit more on that in your proposal? What problem does this solve?

@rsc rsc mentioned this issue Aug 6, 2019
Open
@Ma124
Copy link
Author

Ma124 commented Aug 9, 2019

The Stringer would be useful for all kinds of logging purposes and I personally see no drawback.

The maxHash would be useful every time you need to show something non-go the options there are for hashing. So for example every time a user can choose a hashing algorithm in a CLI or GUI or maybe you list available hashing options to a program written in another language. And your duplicating code every time when you use the alternative: importing the package and putting it into a map. So if for example one of the choices becomes insecure you need to remove the import and the map entry and if you forget one of them you've got either an unused import which cannot be detected by the compiler or you've got a panic.

So I think there are no reasons against Stringer and very few drawbacks of a renamed maxHash.

@FiloSottile FiloSottile changed the title proposal: crypto: crypto.Hash implement fmt.Stringer and export crypto.maxHash proposal: crypto: implement Hash.String Dec 2, 2019
@FiloSottile
Copy link
Contributor

I'm not against implementing crypto.Hash.String for logging and the like.

Since hash functions need to be imported anyway to be available, I don't see the advantage of maxHash, and it's leaking an internal detail. Also, I don't want to encourage "pick your own hash" designs. The set of available hashes in a design should be ideally of size 1, and anyway clearly specified, not "whatever Go supports".

@rsc rsc added this to Active in Proposals (old) Feb 12, 2020
@rsc
Copy link
Contributor

rsc commented Feb 12, 2020

Added to proposal minutes, seems headed for likely accept.

@rsc
Copy link
Contributor

rsc commented Feb 26, 2020

Based on the discussion above, this seems like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Feb 26, 2020
@rsc
Copy link
Contributor

rsc commented Mar 4, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Mar 4, 2020
@rsc rsc modified the milestones: Proposal, Backlog Mar 4, 2020
@rsc rsc changed the title proposal: crypto: implement Hash.String crypto: implement Hash.String Mar 4, 2020
@FiloSottile FiloSottile added NeedsFix The path to resolution is known, but the work has not been done. and removed Proposal labels Mar 4, 2020
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.15 Mar 4, 2020
@katiehockman katiehockman self-assigned this Mar 20, 2020
@gopherbot
Copy link

Change https://golang.org/cl/224937 mentions this issue: crypto: implement Hash.String

@toothrot
Copy link
Contributor

toothrot commented Jun 9, 2020

@katiehockman Hello! It looks like the documentation is missing for this new API: https://tip.golang.org/pkg/crypto/#Hash.String

@FiloSottile
Copy link
Contributor

@toothrot Is it common to document trivial String() string methods?

@katiehockman
Copy link
Contributor

I just did a quick look through several crypto and x/crypto packages, and it differs (sometimes even within the same package).

From what I'm seeing, String() string methods are documented if it does something special or if more explanation is needed, e.g. https://pkg.go.dev/crypto/x509/pkix?tab=doc#Name.String and https://pkg.go.dev/golang.org/x/crypto/ssh?tab=doc#RejectionReason.String.

It isn't documented if its self explanatory, e.g. https://pkg.go.dev/crypto/x509?tab=doc#PublicKeyAlgorithm.String and https://pkg.go.dev/golang.org/x/crypto/ssh?tab=doc#RejectionReason.String.

In this case, I think String() is self-explantory. The Hash type is already described: "Hash identifies a cryptographic hash function that is implemented in another package" and any additional documentation for String() would be redundant.

@toothrot
Copy link
Contributor

Right, sorry for the false alarm. I was merely auditing all new APIs for #39489, and following up on ones that did not have documentation.

@katiehockman
Copy link
Contributor

No problem! Thanks for your diligence.

@ianlancetaylor ianlancetaylor mentioned this issue Jun 12, 2020
Closed
@golang golang locked and limited conversation to collaborators Jun 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
No open projects
Proposals (old)
Accepted
Milestone
Go1.15
Development

No branches or pull requests
7 participants
@toothrot @rsc @andybons @FiloSottile @gopherbot @Ma124 @katiehockman