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: API #2841

Closed
rsc opened this issue Feb 2, 2012 · 7 comments
Closed

crypto: API #2841

rsc opened this issue Feb 2, 2012 · 7 comments
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Feb 2, 2012

The comments in the type crypto.Hash constants need to have import paths updated.
hash.New should probably panic where it currently returns nil.
We can add a separate Available() bool method on Hash.

The various cipher docs all say the same thing.  Let's add Reset() to the
cipher.Block interface and then these all get much shorter: aes.NewCipher
returns (cipher.Block, error) instead of its own type, just like we do for
the hash functions.  That also has the benefit of pointing people toward
the crypto/cipher package, which they should be using anyway.
The warning about not inadvertently inventing ECB mode can go in one
place, on the cipher.Block interface doc comment.

crypto/cipher: NewCBCDecrypter says 'as must', should be 'and must'.

crypto/cipher: move the OCFB modes into go.crypto/openpgp?
They seem out of place here and I assume they are not using
internals of package cipher.

crypto/des: the doc comments on Encrypt, Decrypt should begin with
those words.

crypto/dsa: ErrInvalidPublicKey can use errors.New instead of a
custom type.

crypto/rsa: DecryptionError, MessageTooLongError, VerificationError
are not interesting as types (they are intentionally vague).
Can replace with ErrDecrypt = errors.New("decryption failed") etc.

crypto/rsa: DecryptOAEP: s/random/rand/ to match doc comment
and other functions.

crypto/rsa: DecryptPKCS1v15SessionKey's doc comment ends abruptly,
after a comma.

crypto/tls: Conn should implement SetWriteDeadline.  Any write failure
can be sticky (all future writes fail), but otherwise there's no way to
time out.

crypto/tls: unclear whether *Listener type is needed.  Could rename to
listener (unexported) and make the Listen and NewListener functions
return net.Listener.

crypto/x509/pkix: missing doc for AttrTypeAndValue.
@agl
Copy link
Contributor

@agl agl commented Feb 3, 2012

Comment 1:

http://golang.org/cl/5627045
http://golang.org/cl/5629044
Reset(): Since the garbage collector can copy memory around at will, and it can be
swapped out etc, I fear that Reset() is promising something that we can't deliver and so
should be removed.
The various errors in RSA might be a little vague (and need the package name at the
beginning of the error), but I've tried to follow the idea that distinct errors are
useful when code might want to behave differently based on that error. So
VerificationError means that the signature is bust, which might be useful to distinguish
from "I/O error from the random source" and "we don't support this hash function", which
are other errors that can come from signature verification.
I don't care much either way however if you want to flatten them.
SetWriteDeadline seemed like a longer change so I left that for a different CL.
@rsc
Copy link
Contributor Author

@rsc rsc commented Feb 3, 2012

Comment 2:

Thanks.
Removing Reset sounds fine; then we can still make the cipher
implementations return cipher.Block.
Regarding the errors, my suggestion is that instead of
// A VerificationError represents a failure to verify a signature.
// It is deliberately vague to avoid adaptive attacks.
type VerificationError struct{}
func (VerificationError) Error() string { return "RSA verification error" }
which people have to check for iwth
    if _, ok := err.(rsa.VerificationError); ok {
you can now write
// ErrVerification represents a failure to verify a signature.
// It is deliberately vague to avoid adaptive attacks.
var ErrVerification = errors.New("RSA verification error")
and people can check for it with
    if err == rsa.ErrVerification {
@kortschak
Copy link
Contributor

@kortschak kortschak commented Feb 13, 2012

Comment 3:

I hope this is the appropriate place to add/report this.
The error returned after attempting to serialize an unsigned public key could be more
accurate/accurate. The error text is "Signature: need to call SignRSA or SignDSA before
Serialize", but "SignRSA" and "SignDSA" do not exist anywhere else in the source tree.
They appear to have been rolled into (*Signature).Sign. So error and godoc comment
should probably be changed to reflect this: error "Signature: need to call Sign, SignKey
or SignUserId before Serialize", comment "Sign, SignKey or SignUserId must have been
called first. "
Should openpgp/errors.unknownIssuerError return "signature made by unknown entity"
rather than "signature make by unknown entity"?
@agl
Copy link
Contributor

@agl agl commented Feb 13, 2012

Comment 4:

dan.kortschak: thanks for the report. Will fix once a pending CL for the same files is
submitted.

Owner changed to @agl.

@dgryski
Copy link
Contributor

@dgryski dgryski commented Feb 14, 2012

Comment 5:

The recent commit
http://code.google.com/p/go/source/detail?r=9d7addec2635c403f33a3344444fbaa813ed81d1
seems to have removed a number of updates to go1.tmpl and go1.html, probably
unintentionally.
@agl
Copy link
Contributor

@agl agl commented Feb 14, 2012

Comment 6:

dgryski: thank you for that. hg continually finds new ways to disappoint. Fixed in
604b19afea4b
@agl
Copy link
Contributor

@agl agl commented Feb 21, 2012

Comment 7:

This issue was closed by revision golang/crypto@63736bd.

Status changed to Fixed.

@rsc rsc added fixed labels Feb 21, 2012
cheffo pushed a commit to cheffo/crypto that referenced this issue Mar 4, 2015
@rsc rsc added this to the Go1 milestone Apr 10, 2015
@rsc rsc removed priority-go1 labels Apr 10, 2015
benburkert pushed a commit to benburkert/openpgp that referenced this issue Feb 29, 2016
@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
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
5 participants
You can’t perform that action at this time.