expression: ECB/CBC modes with 128/192/256-bit key length for AES #7425

mccxj · 2018-08-17T06:46:32Z

What problem does this PR solve?

Fix #7300. Support the init_vector argument for builtin function AES_ENCRYPT/AES_DECRYPT

What is changed and how it works?

add system variable 'block_encryption_mode' support

Check List

Tests

Unit test
Integration test

Code changes

Has exported function/method change

…rypt/aes_encrypt

lysu · 2018-08-17T12:31:50Z

expression/builtin_encryption.go

+		if isNull || err != nil {
+			return "", true, errors.Trace(err)
+		}
+		if len(iv) < aes.BlockSize {


what about give another const IVSize to help understand~? although our IVSize = BlockSize = 16

OK. add const IVSize = aes.BlockSize.

lysu · 2018-08-17T12:36:58Z

expression/builtin_encryption.go

+	case "ecb":
+		plainText, err = encrypt.AESDecryptWithECB([]byte(cryptStr), key)
+	case "cbc":
+		plainText, err = encrypt.AESDecryptWithCBC([]byte(cryptStr), key, []byte(iv))


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?

I add "type blockModeBuild func(block cipher.Block) cipher.BlockMode" to create BlockMode argument.

lysu · 2018-08-17T12:38:43Z

expression/builtin_encryption.go

 	"github.com/pingcap/tidb/types"
 	"github.com/pingcap/tidb/util/auth"
 	"github.com/pingcap/tidb/util/chunk"
 	"github.com/pingcap/tidb/util/encrypt"
+	"strings"


please move strings to line 28(it's TiDB rule, standard package need in first import group ^ ^)

mccxj · 2018-08-20T14:42:53Z

@lysu PTAL.

lysu

LGTM @zz-jason @coocood PTAL

lysu · 2018-08-21T12:54:19Z

/run-all-tests

zz-jason · 2018-08-22T02:52:05Z

expression/builtin_encryption.go

-	aes128ecbBlobkSize = 16
-)
+// IVSize indicates the initialization vector supplied to aes_decrypt
+const IVSize = aes.BlockSize


no need to export this variable.

change to ivSize

zz-jason · 2018-08-22T02:52:39Z

expression/builtin_encryption.go

+// IVSize indicates the initialization vector supplied to aes_decrypt
+const IVSize = aes.BlockSize
+
+type aesModeAttr struct {


It's better to add some comment about this struct.

zz-jason · 2018-08-22T02:53:13Z

expression/builtin_encryption.go

+}
+
+var aesModes = map[string]*aesModeAttr{
+	//TODO support more modes


can we create a github issue for this TODO and explain what modes need to be supported?

OK. see #7490

zz-jason · 2018-08-22T03:00:27Z

expression/builtin_encryption.go

+	}
+	if mode.ivRequired {
+		if len(b.args) != 3 {
+			return "", true, ErrIncorrectParameterCount.GenByArgs("aes_decrypt")


This check should be done in aesDecryptFunctionClass.getFunction()

zz-jason · 2018-08-22T03:01:42Z

expression/builtin_encryption.go

+			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")


Is this error message the same with MySQL?

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

zz-jason · 2018-09-07T03:45:07Z

expression/builtin_encryption.go

+		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")


s/16/aes.BlockSize/?

zz-jason · 2018-09-07T03:49:42Z

expression/builtin_encryption.go

+		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"))


moved to execution phase, and new testcase is added.

mccxj · 2018-09-09T09:53:10Z

@zz-jason PTAL.

zz-jason · 2018-09-10T09:33:36Z

expression/builtin_encryption.go


+	"crypto/aes"


please put the standard packages together and add an empty line between the third-part packages.

zz-jason · 2018-09-10T09:43:31Z

util/encrypt/aes.go

+// 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


why prefix Idx and Key with r?

I have not changed it. It seems that code position change affects 'git diff'.see line#163.

zz-jason · 2018-09-10T09:46:23Z

util/encrypt/aes.go

+}
+
+// aesDecrypt decrypts data using AES.
+func aesDecrypt(cryptStr, key []byte, build blockModeBuild) ([]byte, error) {


It's better to pass cipher.BlockMode instead of blockModeBuild

@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.

IMHO, if we can remove this hook, pass the required cipher.BlockMode directly, the code would be easier to read and maintain.

zz-jason · 2018-09-10T09:49:07Z

@XuHuaiyu PTAL

jackysp · 2018-09-12T02:44:53Z

@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.

zz-jason · 2018-09-12T12:36:25Z

@mccxj CI failed.

zz-jason · 2018-09-12T13:06:29Z

/run-all-tests

alivxxx · 2018-09-14T06:13:57Z

expression/builtin_encryption.go

-	aes128ecbBlobkSize = 16
-)
+// ivSize indicates the initialization vector supplied to aes_decrypt
+const ivSize = aes.BlockSize


There is a const variable BlockSize in aes package.

mccxj · 2018-09-23T08:27:08Z

@zz-jason @lamxTyler @jackysp PTAL

zz-jason

LGTM

zz-jason · 2018-09-23T13:30:49Z

/run-all-tests

…ngcap#7425)

) (#9344)

mccxj added 4 commits August 17, 2018 14:30

expression: ECB/CBC modes with 128/192/256-bit key length for aes_dec…

e50e207

…rypt/aes_encrypt

expression: add more tests for AES

3ce847d

Merge branch 'master' into aes

f3ba799

*: fix ci

2f0de8d

lysu reviewed Aug 17, 2018

View reviewed changes

lysu added contribution This PR is from a community contributor. component/expression status/all tests passed labels Aug 17, 2018

mccxj added 2 commits August 20, 2018 21:44

Merge branch 'master' into aes

3bf2a60

refactor AES Encrypt/Decrypt

1c10b19

Merge branch 'master' into aes

e9e5fa4

lysu reviewed Aug 21, 2018

View reviewed changes

lysu added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 21, 2018

zz-jason reviewed Aug 22, 2018

View reviewed changes

mccxj mentioned this pull request Aug 26, 2018

add more modes for AES #7490

Open

18 tasks

mccxj added 7 commits August 26, 2018 09:40

Merge branch 'master' into aes

4d5d8a9

fix review problem

817f05a

refactor sig to two parts

d206725

Merge branch 'master' into aes

58e1efd

revert merge code for mistake

4279572

fix ci

dda3ec7

Merge branch 'master' into aes

b4e0ee9

zz-jason reviewed Sep 7, 2018

View reviewed changes

mccxj added 2 commits September 9, 2018 17:16

Merge branch 'master' into aes

f1e233b

move 'iv' check to execution phase

492f94c

zz-jason reviewed Sep 10, 2018

View reviewed changes

zz-jason assigned XuHuaiyu Sep 10, 2018

mccxj added 2 commits September 12, 2018 08:51

formate import

d732b47

Merge branch 'master' into aes

6cd647f

mccxj added 2 commits September 12, 2018 20:52

fix ci

ec0e07b

Merge branch 'master' into aes

7d06fbf

alivxxx reviewed Sep 14, 2018

View reviewed changes

mccxj added 2 commits September 22, 2018 16:23

Merge branch 'master' into aes

3b8a159

remove buildBlockMode

b423fb8

zz-jason approved these changes Sep 23, 2018

View reviewed changes

zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 23, 2018

Merge branch 'master' into aes

90b51e2

zz-jason merged commit 038ac63 into pingcap:master Sep 23, 2018

mccxj deleted the aes branch September 23, 2018 14:48

crazycs520 added a commit to crazycs520/tidb that referenced this pull request Feb 18, 2019

expression: ECB/CBC modes with 128/192/256-bit key length for AES (pi…

db1fb1f

…ngcap#7425)

crazycs520 mentioned this pull request Feb 18, 2019

expression: ECB/CBC modes with 128/192/256-bit key length for AES (#7425) #9344

Merged

jackysp pushed a commit that referenced this pull request Feb 21, 2019

expression: ECB/CBC modes with 128/192/256-bit key length for AES (#7425

2de0e5f

) (#9344)

expression: ECB/CBC modes with 128/192/256-bit key length for AES #7425

expression: ECB/CBC modes with 128/192/256-bit key length for AES #7425

Conversation

mccxj commented Aug 17, 2018 • edited by jackysp Loading

What problem does this PR solve?

What is changed and how it works?

Check List

lysu Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccxj commented Aug 20, 2018

lysu left a comment

Choose a reason for hiding this comment

lysu commented Aug 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccxj commented Sep 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccxj Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason commented Sep 10, 2018

jackysp commented Sep 12, 2018

zz-jason commented Sep 12, 2018

zz-jason commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccxj commented Sep 23, 2018

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Sep 23, 2018

mccxj commented Aug 17, 2018 •

edited by jackysp

Loading

lysu Aug 17, 2018 •

edited

Loading

mccxj Sep 12, 2018 •

edited

Loading