Add response on fail for mysql #533

G1gg1L3s · 2022-05-04T18:07:46Z

Mysql doesn't respond to the different options of the response_on_fail (default_value, error and ciphertext) field. This PR changes that and bring support as in #521.

Checklist

Change is covered by automated tests
The coding guidelines are followed
Public API has proper documentation in the Acra documentation site or has PR on documentation repository
with new changes
~~CHANGELOG.md is updated (in case of notable or breaking changes)~~
CHANGELOG_DEV.md is updated
~~Benchmark results are attached (if applicable)~~
~~Example projects and code samples are up-to-date (in case of API changes)~~

To reuse it later in mysql

To prevent import cycles

So it looks like in the postgres version. It also simplifies and reduces the code.

It makes sense, because: a) It is used mostly in string context. b) It is an identity operation only in postgres case. In mysql it encodes string as length encoded one.

To be more consistent.

Just copy-paste from `WithoutDefaults`

In case of EncodingError, forward the it to the client as `QueryInterruptedError` but with custom message.

Lagovas · 2022-05-04T22:42:05Z

decryptor/base/encoder.go

+// ready to be encoded
+type EncodingValue interface {
+	// AsPostgresBinary returns value encoded in postgres binary format
+	AsPostgresBinary() []byte


we use base package to store common things for all databases. what we will do after adding one more protocol? for example mysql X or mariaDB or mssql?)
okay, we want return type aware values line Int/String/Bytes which knows how to encode value according to DBs protocol. okay, there is great place for design patterns)

we can introduce fabric that can create new Int/String/Bytes values that will implement EncodingValue with methods like EncodeToBinary/EncodeToInt:

type TypeFactory interface { NewBytesValue(...) EncodingValue NewIntValue(...) EncodingValue NewStringValue(...) EncodingValue }

Then update common function EncodeDefault and accept +1 parameter func EncodeDefault(..., typeFactory TypeFactory) and use it to create new Int/String values instead of calling functions NewBytesValue that should implement methods for mysql/postgresql/etc

After that target db package should implement own fabric that will return own implementations of EncodedValue and return them.

Yeah, you are right. It was one of the possible implementations, but I thought that it has much complexity. But current implementation is hard to extend. So, good idea!

Lagovas · 2022-05-04T22:43:03Z

decryptor/base/utils.go

@@ -14,13 +14,14 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package mysql
+package base


it's mysql related functions. they should be stored in mysql package, not in base)

ValueFactory will be implemented by each backend to support their own encoding.

@Lagovas

This reverts commit 1a913c1. Because as @Lagovas mentioned: > it's mysql related functions. they should be stored in mysql package, not in base)

Zhaars

Lgtm

This is a temporary solution, before proper error handling lands on.

This is temporary, until we implement proper flushing of db packets. Right now, it data is sent after our encoding error, it would probably trigger the "unexpected sequence number" error on the client side.

Lagovas · 2022-05-06T09:51:39Z

decryptor/mysql/data_encoder_test.go

+	"github.com/sirupsen/logrus"
+)
+
+const binaryFormat = true


Lagovas · 2022-05-06T09:57:00Z

decryptor/mysql/data_encoder_test.go

+	}
+
+	testcases := []testcase{
+		{[]byte("string"), common.EncryptedType_String, []byte("\x06string")},


can you explain why we expect \x in this strings? documentation tells that binary data should be sent as hex literals like X'<hex_val>' or as 0x<hex_val>. As I remember, \x<hex_val> it is postgresql specific literals

Don't forget that this is a go string :)
The first byte(s) of encoded string is a length of that string. It out case, length is 6. So, to insert a 06 byte I used escaping.
Another way of writing that is:

[]byte{06, 's', 't', 'r', 'i', 'n', 'g'}

Which, imho, is less readable

aaaaaaaa, understood. thanks for explanation. you r right

Lagovas · 2022-05-06T09:58:53Z

decryptor/mysql/data_encoder_test.go

+	}
+
+	testcases := []testcase{
+		{[]byte("string"), common.EncryptedType_String, []byte("\x06string")},


same here, why we expect \x, not 0x<hex_val>?

Lagovas

great job. especially it's pleasure to look and read to such structured commits and their separation) great example to follow)

G1gg1L3s added 12 commits May 3, 2022 21:51

Move EncodingValue into the base module

f54a1cf

To reuse it later in mysql

Move mysql.utils into base.utils

1a913c1

To prevent import cycles

Add OnFail handler into text encoder

7bde560

Add OnFail handler for a binary encoding

c0dca3c

Merge text and binary encoding into one function

89bb2e6

So it looks like in the postgres version. It also simplifies and reduces the code.

Add tests for successful, default and error paths

0faff6b

Rename IdentityValue into StringValue

240253b

It makes sense, because: a) It is used mostly in string context. b) It is an identity operation only in postgres case. In mysql it encodes string as length encoded one.

Rename ByteSequenceValue into BytesValue

28b8b6a

To be more consistent.

Add integration tests for ciphertext option

db9583b

Just copy-paste from `WithoutDefaults`

Send interrupted error with custom msg to client

d455624

In case of EncodingError, forward the it to the client as `QueryInterruptedError` but with custom message.

Add integration tests for response_on_fail: error

96325a9

Update changelog

a4ead1d

G1gg1L3s requested review from Lagovas, iamnotacake and Zhaars May 4, 2022 18:07

Lagovas reviewed May 4, 2022

View reviewed changes

G1gg1L3s added 5 commits May 5, 2022 15:07

Add ValueFactory and a temporary plug

7daf4c8

ValueFactory will be implemented by each backend to support their own encoding.

Implement postgresValueFactory

8b447d1

Implement mysqlValueFactory

03725d3

Canonize EncodingValue and remove plugs

1e867e7

Revert "Move mysql.utils into base.utils"

a638607

This reverts commit 1a913c1. Because as @Lagovas mentioned: > it's mysql related functions. they should be stored in mysql package, not in base)

Zhaars approved these changes May 5, 2022

View reviewed changes

G1gg1L3s added 2 commits May 6, 2022 13:24

Close the connection in case of encoding error

28d9ace

This is a temporary solution, before proper error handling lands on.

Continue serving packets in case of encoding error

9dfb334

This is temporary, until we implement proper flushing of db packets. Right now, it data is sent after our encoding error, it would probably trigger the "unexpected sequence number" error on the client side.

Lagovas reviewed May 6, 2022

View reviewed changes

Lagovas approved these changes May 6, 2022

View reviewed changes

G1gg1L3s merged commit 077865c into master May 6, 2022

G1gg1L3s deleted the G1gg1L3s/T2559-response-on-fail-mysql branch May 6, 2022 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add response on fail for mysql #533

Add response on fail for mysql #533

G1gg1L3s commented May 4, 2022

Lagovas May 4, 2022

G1gg1L3s May 5, 2022

Lagovas May 4, 2022

Zhaars left a comment

Lagovas May 6, 2022

Lagovas May 6, 2022

G1gg1L3s May 6, 2022

Lagovas May 6, 2022

Lagovas May 6, 2022

Lagovas left a comment

Add response on fail for mysql #533

Add response on fail for mysql #533

Conversation

G1gg1L3s commented May 4, 2022

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhaars left a 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

Lagovas left a comment

Choose a reason for hiding this comment