expression: ECB/CBC modes with 128/192/256-bit key length for AES #7425
Conversation
expression/builtin_encryption.go
Outdated
| if isNull || err != nil { | ||
| return "", true, errors.Trace(err) | ||
| } | ||
| if len(iv) < aes.BlockSize { |
There was a problem hiding this comment.
what about give another const IVSize to help understand~? although our IVSize = BlockSize = 16
There was a problem hiding this comment.
OK. add const IVSize = aes.BlockSize.
expression/builtin_encryption.go
Outdated
| case "ecb": | ||
| plainText, err = encrypt.AESDecryptWithECB([]byte(cryptStr), key) | ||
| case "cbc": | ||
| plainText, err = encrypt.AESDecryptWithCBC([]byte(cryptStr), key, []byte(iv)) |
There was a problem hiding this comment.
It seems some duplicate logic in AESDecryptWithECB and AESDecryptWithCBC, AESEncryptWithECB and AESEncryptWithCBC~
what about use AESDecrypt, AESEncrypt, then inversion of control, pass a cipher.BlockMode argument?
There was a problem hiding this comment.
I add "type blockModeBuild func(block cipher.Block) cipher.BlockMode" to create BlockMode argument.
expression/builtin_encryption.go
Outdated
| "github.com/pingcap/tidb/types" | ||
| "github.com/pingcap/tidb/util/auth" | ||
| "github.com/pingcap/tidb/util/chunk" | ||
| "github.com/pingcap/tidb/util/encrypt" | ||
| "strings" |
There was a problem hiding this comment.
please move strings to line 28(it's TiDB rule, standard package need in first import group ^ ^)
lysu
added
contribution
|
@lysu PTAL. |
|
/run-all-tests |
expression/builtin_encryption.go
Outdated
| aes128ecbBlobkSize = 16 | ||
| ) | ||
| // IVSize indicates the initialization vector supplied to aes_decrypt | ||
| const IVSize = aes.BlockSize |
There was a problem hiding this comment.
no need to export this variable.
expression/builtin_encryption.go
Outdated
| // IVSize indicates the initialization vector supplied to aes_decrypt | ||
| const IVSize = aes.BlockSize | ||
|
|
||
| type aesModeAttr struct { |
There was a problem hiding this comment.
It's better to add some comment about this struct.
expression/builtin_encryption.go
Outdated
| } | ||
|
|
||
| var aesModes = map[string]*aesModeAttr{ | ||
| //TODO support more modes |
There was a problem hiding this comment.
can we create a github issue for this TODO and explain what modes need to be supported?
expression/builtin_encryption.go
Outdated
| } | ||
| if mode.ivRequired { | ||
| if len(b.args) != 3 { | ||
| return "", true, ErrIncorrectParameterCount.GenByArgs("aes_decrypt") |
There was a problem hiding this comment.
This check should be done in aesDecryptFunctionClass.getFunction()
expression/builtin_encryption.go
Outdated
| return "", true, errors.Trace(err) | ||
| } | ||
| if len(iv) < IVSize { | ||
| return "", true, errIncorrectArgs.Gen("The initialization vector supplied to aes_decrypt is too short. Must be at least 16 bytes long") |
There was a problem hiding this comment.
Is this error message the same with MySQL?
There was a problem hiding this comment.
Yes..I had checked, messages is same to mysql.
but in strict, errorcode returned by this PR has some different to mysql, expect this point, new code using errors.Errorf maybe should add a new ErrXXX.GenByArgs
expression/builtin_encryption.go
Outdated
| return "", true, errors.Trace(err) | ||
| } | ||
| if len(iv) < aes.BlockSize { | ||
| return "", true, errIncorrectArgs.Gen("The initialization vector supplied to aes_decrypt is too short. Must be at least 16 bytes long") |
expression/builtin_encryption.go
Outdated
| return &builtinAesDecryptIVSig{bf, mode}, nil | ||
| } else if len(args) == 3 { | ||
| // For modes that do not require init_vector, it is ignored and a warning is generated if it is specified. | ||
| ctx.GetSessionVars().StmtCtx.AppendWarning(errWarnOptionIgnored.GenByArgs("IV")) |
There was a problem hiding this comment.
It seems this warning should be appended in the function execution phase for every input row:
MySQL(localhost:3306) > select aes_encrypt(a, b, c) from t;
+----------------------+
| aes_encrypt(a, b, c) |
+----------------------+
| 0SF&Qٜ |
| 0SF&Qٜ |
| 0SF&Qٜ |
| 0SF&Qٜ |
+----------------------+
4 rows in set, 4 warnings (0.00 sec)
MySQL(localhost:3306) > show warnings;
+---------+------+---------------------+
| Level | Code | Message |
+---------+------+---------------------+
| Warning | 1618 | <IV> option ignored |
| Warning | 1618 | <IV> option ignored |
| Warning | 1618 | <IV> option ignored |
| Warning | 1618 | <IV> option ignored |
+---------+------+---------------------+
4 rows in set (0.00 sec)There was a problem hiding this comment.
moved to execution phase, and new testcase is added.
|
@zz-jason PTAL. |
expression/builtin_encryption.go
Outdated
|
|
||
| "crypto/aes" |
There was a problem hiding this comment.
please put the standard packages together and add an empty line between the third-part packages.
util/encrypt/aes.go
Outdated
| // See https://security.stackexchange.com/questions/4863/mysql-aes-encrypt-key-length. | ||
| func DeriveKeyMySQL(key []byte, blockSize int) []byte { | ||
| rKey := make([]byte, blockSize) | ||
| rIdx := 0 |
There was a problem hiding this comment.
why prefix Idx and Key with r?
There was a problem hiding this comment.
I have not changed it. It seems that code position change affects 'git diff'.see line#163.
util/encrypt/aes.go
Outdated
| } | ||
|
|
||
| // aesDecrypt decrypts data using AES. | ||
| func aesDecrypt(cryptStr, key []byte, build blockModeBuild) ([]byte, error) { |
There was a problem hiding this comment.
It's better to pass cipher.BlockMode instead of blockModeBuild
There was a problem hiding this comment.
@zz-jason AESEncryptWithCBC/AESDecryptWithCBC is similar with AESEncryptWithECB/AESDecryptWithECB with different cipher.BlockMode, CBC needs iv but ECB does not, so blockModeBuild is the hook method to build cipher.BlockMode.
There was a problem hiding this comment.
IMHO, if we can remove this hook, pass the required cipher.BlockMode directly, the code would be easier to read and maintain.
|
@XuHuaiyu PTAL |
|
@mccxj , I've added a word "Fix" before the issue link in the description of this PR, then the issue could be closed automatically when this PR is merged. |
|
@mccxj CI failed. |
|
/run-all-tests |
| aes128ecbBlobkSize = 16 | ||
| ) | ||
| // ivSize indicates the initialization vector supplied to aes_decrypt | ||
| const ivSize = aes.BlockSize |
There was a problem hiding this comment.
There is a const variable BlockSize in aes package.
zz-jason
added
status/LGT2
|
/run-all-tests |
What problem does this PR solve?
Fix #7300. Support the
init_vectorargument for builtin functionAES_ENCRYPT/AES_DECRYPTWhat is changed and how it works?
Check List
Tests
Code changes