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: Remove redundant internal struct x509.rsaPublicKey and replace references with rsa.PublicKey #19355

Closed
andrewmbenton opened this issue Mar 2, 2017 · 5 comments

Comments

@andrewmbenton
Copy link
Contributor

@andrewmbenton andrewmbenton commented Mar 2, 2017

Just proposing a simple change to remove the apparently redundant x509.rsaPublicKey internal struct that's used in two places, preventing future accidental use.

I have a simple patch to submit and all tests pass.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 2, 2017

I don't think it does any harm keep it as-is. In fact, it's defensive: it's documented as being the asn.1 structure, whereas the other one could have fields added or reordered safely and is the one for users.

@andrewmbenton
Copy link
Contributor Author

@andrewmbenton andrewmbenton commented Mar 2, 2017

Perhaps true. I had thought that its use was somewhat idiosyncratic, but it appears to be used only twice, in calls to asn1.Marshal and asn1.Unmarshal.

So perhaps moving it into the asn1 package and possibly renaming it would better indicate its intended use?

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 2, 2017

We don't put the http2 encoder and decoder in the bytes package just because it's eventually encoded as bytes. I don't see why the asn1 package should contain anything about RSA or x509 or anything else that uses asn.1.

The layering seems good as it is. Closing, but feel free to argue otherwise.

@bradfitz bradfitz closed this Mar 2, 2017
@andrewmbenton
Copy link
Contributor Author

@andrewmbenton andrewmbenton commented Mar 2, 2017

Good point RE not including higher-level structs in codec packages. Wasn't thinking clearly on that.

I'd suggest finally then that the struct in question be renamed pkcs1PublicKey for consistency with the other ASN.1-compliant named types in the package (which I had missed before) and it should be moved to the top of x509/pkcs1.go with the other type declarations.

One of the reasons this was confusing to me was probably that it was just hanging out all alone down there at the bottom of the file.

Not going to argue too hard for this though. Handle as you see fit and thanks for taking the time to discuss. Easy enough for me to submit the CL if so desired.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 7, 2017

CL https://golang.org/cl/37885 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 7, 2017
For consistency with the other named types in this package, this
change renames the unexported rsaPublicKey struct to pkcs1PublicKey
and positions the declaration up with the other similarly-named
types in pkcs1.go.

See the final comment of #19355 for discussion.

Change-Id: I1fa0366a8efa01602b81bc69287ef747abce84f5
Reviewed-on: https://go-review.googlesource.com/37885
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Mar 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.