<p>Hi</p>
<p>Please review my code and help me to make it better</p>
<p>Thanks</p>
<pre><code>package aesctr
import (
"crypto/aes"
"crypto/cipher"
"crypto/hmac"
"crypto/rand"
"crypto/sha256"
"crypto/subtle"
"encoding/base64"
"errors"
"golang.org/x/crypto/pbkdf2"
)
func Encrypt(plaintext, key []byte) (string, error) {
if len(key) < 20 {
return "", errors.New("key is too short, the minimum length is 20 characters")
}
encryptionKey, hmacKey, salt, err := generateKeys(key)
if err != nil {
return "", err
}
block, err := aes.NewCipher(encryptionKey)
if err != nil {
return "", err
}
ciphertext := make([]byte, aes.BlockSize+len(plaintext))
iv := ciphertext[:aes.BlockSize]
if _, err := rand.Read(iv); err != nil {
return "", err
}
stream := cipher.NewCTR(block, iv)
stream.XORKeyStream(ciphertext[aes.BlockSize:], plaintext)
hmac := generateHMAC(ciphertext, hmacKey)
ciphertext = append(salt, ciphertext...)
ciphertext = append(hmac, ciphertext...)
return base64.StdEncoding.EncodeToString(ciphertext), nil
}
func Decrypt(b64ciphertext string, key []byte) ([]byte, error) {
ciphertext, err := base64.StdEncoding.DecodeString(b64ciphertext)
var plaintext []byte
if err != nil {
return plaintext, err
}
hmac, salt, ciphertext := ciphertext[:32], ciphertext[32:96], ciphertext[96:]
iv := ciphertext[:aes.BlockSize]
encryptionKey, hmacKey := deriveKey(key, salt[:32]), deriveKey(key, salt[32:])
hmacNew := generateHMAC(ciphertext, hmacKey)
if subtle.ConstantTimeCompare(hmac, hmacNew) != 1 {
return plaintext, errors.New("invalid hmac")
}
block, err := aes.NewCipher(encryptionKey)
if err != nil {
return plaintext, err
}
stream := cipher.NewCTR(block, iv)
plaintext = make([]byte, len(ciphertext)-aes.BlockSize)
stream.XORKeyStream(plaintext, ciphertext[aes.BlockSize:])
return plaintext, nil
}
func generateHMAC(message, key []byte) []byte {
mac := hmac.New(sha256.New, key)
mac.Write(message)
return mac.Sum(nil)
}
func generateKeys(key []byte) ([]byte, []byte, []byte, error) {
salt := make([]byte, 64)
if _, err := rand.Read(salt); err != nil {
return nil, nil, nil, err
}
encryptionKey, hmacKey := deriveKey(key, salt[:32]), deriveKey(key, salt[32:])
return encryptionKey, hmacKey, salt, nil
}
func deriveKey(key, salt []byte) []byte {
return pbkdf2.Key(key, salt, 1<<16, 32, sha256.New)
}
</code></pre>
<hr/>**评论：**<br/><br/>epiris: <pre><p>What is your use case? Without it I can only speculate that this was a learning experience. So at a glance:</p>
<p>Show stoppers:</p>
<ul>
<li>PANIC: You need bounds checks, Decrypt can panic on a string < 96+blocksize.</li>
<li>Home grown authentication and salt scheme, makes this unusable by default. You should not be using a cipher in block mode for single fixed length message like this, let alone a CTR block mode. If you used any cipher prim (you shouldn't) it should be a cipher in AEAD mode so you don't implement your own authentication.</li>
<li>Is it really desirable to have a single shared key? Do multiple machines produce secrets? Do you need to validate which machine made the secret? As these questions emerge your answer will probably push you towards something public-key based like <a href="https://godoc.org/golang.org/x/crypto/nacl/box" rel="nofollow">nacl/box</a>.</li>
</ul>
<p>Things that are correct but I consider poor form in a crypto context:</p>
<ul>
<li>I do not like generateKeys returning 4 arguments as it's easy to mix up 3 very important byte slices and the abstraction doesn't provide any value. I would inline it and then you can create one entropy buffer at function entry and use it for IV and Salts saving a call to rand, allocations and readability.</li>
<li>Generate hmac returns bytes without any indication of it's hashing function. You then write at the call site a fixed 32 bytes (hash.Size of sha256), if you changed the hash function to something with more or less bytes and don't use the bounds given by hmac.New's hash.Hash Size method. You could end up truncating your hash or far worse as off-by-N's propagate in offset math to write a few plain bytes of your secret into a buffer somewhere.</li>
<li>I think you may not have realized that 1<<16 is 65k, which is way way more pbkdf2 rounds than necessary.</li>
<li>My general feedback on hash sizes apply here, there are a lot of integer literals that are making assumptions on bounds based on which is asking for later-on-you to come make a mistake. Crypto primitives always have interfaces that define their bounds, use them diligently.</li>
</ul>
<p>So the summary here is you probably don't want to use AES. Forming stronger invariants will help your bounds be more clear and future proof. Good job overall, happy coding.</p></pre>ollyy48: <pre><p>Thank you for response</p>
<p>I was trying to implement aes-ctr with authentication and key derivation in go</p>
<p>It's just for fun</p></pre>ar1819: <pre><p>Pinging <a href="/u/epiris" rel="nofollow">/u/epiris</a> - you seem to know this stuff) </p></pre>TheMerovius: <pre><p>What is the problem you are trying to solve and why isn't using TLS/GPG/nacl/jwt/… a sufficient solution? Usually, if you are using these low-level primitives, you should know if you're using them correctly and at least for the crypto-part, you should probably not ask <a href="/r/golang" rel="nofollow">/r/golang</a>, but some crypto-related subreddit.</p>
<p>From a "is this idiomatic Go" perspective:</p>
<ul>
<li>Your variable names are <em>long</em>. IMO too long.</li>
<li>Why does Decrypt take a base64-encoded string, instead of simply a <code>[]byte</code>? It seems unnecessarily restrictive. If the goal of the package is to directly work on printable strings, then you shouldn't call it <code>b64ciphertext</code>, but just <code>s</code> or something. The base64 is implied by the intention of the package and it being ciphertext is implied by the function name. Similarly for <code>plaintext</code>, btw, I would just call it <code>b</code>.</li>
</ul>
<p>Apart from that, I don't see any readability problems here. But again, to be clear: I don't understand the point of this package and I don't know whether the crypto-stuff you are doing is sound <em>at all</em>.</p></pre>TheMerovius: <pre><p>Oh, of course: Your exported functions should have godoc-comments, just as the package as a whole. You should probably run <code>golint</code> and <code>go vet</code> and address what they are complaining about.</p></pre>
