MySQL encoding data types #517

Zhaars · 2022-03-29T12:09:46Z

Basic implementation and code refactoring regarding data type encoding in MySQL based on @Lagovas PR for PostgreSQL.

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)

wrap encoding functions with wrapper that do nothing with valid strings

…ossible errors

user struct instead of interface for QueryDataItem to avoid extra nil checks with reflection

weren't used by any component and just produce unused code and complicate reading

…ing/decoding operations over interesting data for decryption/detokenization received from database

extend encryptor, now find ColumnEncryptionSettings for queries from Parse Packet with placeholders and save it in ClientSession to use it in ParameterDescription packet followed by Parse packet (and also for Bind packet but there we already have query to re-parse it)

update OID values according to ColumnEncryptionSetting change value encoding flow for encrypted integers Remove encoding/decoding logic from Column struct, use only from PostgresqlEncoderDecoder and BoundValue encodings

…ta-according-to-data-type

- don't use DecodedData struct - store raw data on protocol handler level - encode/decode data in OnColumn handler on higher level

…t formatted data

…unction

…g in SQL queries to encrypt bound values

fix parameter description updates

… package

…ivers

basic implementation of encoding data types for MySQL

updated columnInfo type with origin type tracking

# Conflicts: # decryptor/postgresql/data_encoder.go # decryptor/postgresql/data_encoder_test.go # decryptor/postgresql/pg_decryptor.go # decryptor/postgresql/proxy.go # encryptor/config/common/encryptedTypes.go # encryptor/config/encryptionSettings.go # encryptor/config/schemaStore_test.go # encryptor/dbDataCoder.go # encryptor/queryDataEncryptor.go # tests/test.py # utils/escape_format.go # utils/utils.go

fixed conflicts

restore deleted test file

Lagovas · 2022-03-30T08:54:37Z

decryptor/mysql/data_encoder.go

+		intValue, err := strconv.ParseInt(string(data), 10, 32)
+		if err != nil {
+			if newVal := setting.GetDefaultDataValue(); newVal != nil {
+				intValue, err = strconv.ParseInt(*newVal, 10, 64)


Suggested change

intValue, err = strconv.ParseInt(*newVal, 10, 64)

intValue, err = strconv.ParseInt(*newVal, 10, 32)

Lagovas · 2022-03-30T08:55:01Z

decryptor/mysql/data_encoder.go

+
+	case common.EncryptedType_Int64:
+		encoded := make([]byte, 8)
+		intValue, err := strconv.ParseInt(string(data), 10, 32)


Suggested change

intValue, err := strconv.ParseInt(string(data), 10, 32)

intValue, err := strconv.ParseInt(string(data), 10, 64)

Lagovas · 2022-03-30T10:02:00Z

decryptor/mysql/data_encoder.go

+					return data, false, err
+				}
+			}
+			// TODO: ask Lagovas what to do in case of error casting to defined type.


do nothing. return value inappropriate to type as it would be with encrypted binary data in case of expected integer.
at the future we will return DB specific error using wire protocol error notifications.

Discussed to implement rollaback type logic

Lagovas · 2022-03-30T10:04:24Z

decryptor/mysql/response_proxy.go

-	var n int
-	var err error
+	//var n int
+	//var err error


dont' forget to remove

Lagovas · 2022-03-30T10:06:50Z

decryptor/mysql/prepared_statements.go

+	// means that we set encrypted data
+	if !bytes.Equal(m.textData, newData) {
+		m.paramType = TypeBlob
+		m.textData = newData


hm, so now it's not only textData but binary too. maybe we should rename textData to data?

Lagovas · 2022-03-30T10:14:07Z

decryptor/mysql/prepared_statements.go

@@ -187,7 +187,11 @@ func (m *mysqlBoundValue) Copy() base.BoundValue {

 // SetData set new value to BoundValue using ColumnEncryptionSetting if provided
 func (m *mysqlBoundValue) SetData(newData []byte, setting config.ColumnEncryptionSetting) error {
-	m.textData = newData
+	// means that we set encrypted data


hm... for now it is used by QueryEncryptor uses it after encryption/tokenization. but what about if some else component will use it and there will be not only encrypted? can we somehow check that it is encrypted and we should change type?

Lagovas · 2022-03-30T10:24:35Z

decryptor/mysql/response_proxy.go

-			WithError(err).WithField("field_index", columnIndex).
-			Errorln("Failed to process column data")
-		return nil, err
+func (handler *Handler) extractProcessData(pos int, rowData []byte, field *ColumnDescription) ([]byte, int, error) {


maybe name it just extractData? because two verbs a bit discourage

Added intergration tests

Delete redundant changes

Fixed logging

Lagovas · 2022-04-05T20:13:15Z

decryptor/mysql/data_encoder.go

+var ErrConvertToDataType = errors.New("error on converting to data type")
+
+// BaseMySQLDataEncoderProcessor implements processor and encode/decode binary intX values to text format which acceptable by Tokenizer
+type BaseMySQLDataEncoderProcessor struct{}


what about BaseMySQLDataEncoderProcessor rename to BaseMySQLDataProcessor, EncodeMySQLDataEncoderProcessor -> MySQLDataEncoderProcessor and DecodeMySQLDataEncoderProcessor -> MySQLDataDecoderProcessor?
because Encode...EncoderProcessor and Decode...EncodeProcessor looks strange)

Lagovas · 2022-04-05T20:15:50Z

decryptor/mysql/data_encoder.go

+					return data, false, err
+				}
+				return PutLengthEncodedString(binValue), true, nil
+			}


why we dont return here ErrConvertToDataType in case when it wasn't decrypted?

Good question. Currently we dont rollback fields type for text protocol. Do we have to?

as we discussed, we decided to rollback too. so we should return here same error?

Lagovas · 2022-04-05T20:18:44Z

decryptor/mysql/response_proxy.go

@@ -515,13 +502,13 @@ func (handler *Handler) processTextDataRow(ctx context.Context, rowData []byte,
 		if err != nil {
 			return nil, err
 		}
-		value, err = handler.onColumnDecryption(ctx, i, value)
+		_, value, err = handler.onColumnDecryption(ctx, i, value, false, fields[i])


why we ignore here ctx?

The same question - we are not rolling back types in case of conversion failure for text protocol

Lagovas · 2022-04-05T20:25:03Z

tests/test.py

@@ -8596,6 +8715,7 @@ def testClientIDRead(self):
                self.assertEqual(data[column], row[column])
            else:
                self.assertNotEqual(data[column], row[column])
+                self.assertEqual(row[column], default_expected_values[column])


encryptor/dbDataCoder.go

Lagovas · 2022-04-05T21:01:38Z

decryptor/mysql/response_proxy.go

 					break
 				}
 			}
 			handler.logger.WithField("column_index", i).Debugln("Parse field")
-			field, err := ParseResultField(fieldPacket.GetData())
+			field, err := ParseResultField(fieldPacket, handler.setting.TableSchemaStore())


what about to split Parsing and updating type of field? or rename to something obvious that show the fact that field's type will be changed?

Agree to rename. It looks like confusion

Fixed After review

# Conflicts: # encryptor/config/encryptionSettings.go

Merge master changes

updated CHANGELOG_DEV.md file

Added addinonal logging/comments

Lagovas · 2022-04-07T09:14:22Z

decryptor/mysql/data_encoder.go

+		return ctx, PutLengthEncodedString(data), nil
+	}
+
+	var columnType = columnInfo.DataBinaryType()


let's assign this variable a bit below close to the usage block? because read this row and tried to find where it used

Agree, makes sense.

Lagovas · 2022-04-07T09:25:34Z

decryptor/mysql/data_encoder.go

+
+	case TypeTiny:
+		encoded = make([]byte, 1)
+		intValue, err := strconv.ParseInt(string(data), 10, 8)


what about to use BytesToString?)
looks like safe place for accessing data without concurrent access during parsing
same below for other int types

Lagovas · 2022-04-07T09:27:24Z

decryptor/mysql/data_encoder.go

+					return data, false, err
+				}
+				return PutLengthEncodedString(binValue), true, nil
+			}


as we discussed, we decided to rollback too. so we should return here same error?

Lagovas · 2022-04-07T09:32:04Z

decryptor/mysql/data_encoder.go

+		if err != nil {
+			break
+		}
+		return ctx, []byte(strconv.FormatInt(int64(numericValue), 10)), nil


what about to use strconv.AppendInt? It helps avoid extra conversion from string to bytes:
strconv.FormatInt(nil, int64(numericValue), 10)
same below

Cool, didnt know about these methods before

Lagovas · 2022-04-07T09:39:16Z

encryptor/dbDataCoder.go

-			return data, nil
+		case sqlparser.IntVal:
+			// if data was just tokenized, so we return it as is because it is valid int literal
+			if _, err := strconv.Atoi(string(data)); err == nil {


looks like BytesToString

Lagovas · 2022-04-07T09:46:49Z

in general looks great except question about returning error in binary encoding. improvements related to type conversions are questionable)

Lagovas · 2022-04-07T11:30:35Z

decryptor/mysql/response_proxy.go

 			if err != nil {
 				handler.logger.WithField(logging.FieldKeyEventCode, logging.EventCodeErrorProtocolProcessing).WithError(err).Errorln("Can't parse result field")
 				return err
 			}
+			// updating filed type according to DataType provided in schemaStore


Suggested change

// updating filed type according to DataType provided in schemaStore

// updating field type according to DataType provided in schemaStore

Fixed after review

Fixed failing tests

replace string() convertion to more efficient utils.BytesToString()

Lagovas

lgtm

replace string() convertion to more efficient utils.BytesToString()

Lagovas added 30 commits January 26, 2022 20:06

extend client session with map interface to store session related data

fe213b6

save session in ctx

1a28a14

save query encryption settings to client session

0ce6808

make public function of encoding binary data to hex

2e1bd4c

wrap encoding functions with wrapper that do nothing with valid strings

extend GetData method for BoundValue with error returning to handle p…

4ac626c

…ossible errors

save client session in ctx after accepting connection

f9a3e89

user struct instead of interface for QueryDataItem to avoid extra nil checks with reflection

remove redundant interface method with per column subscribing that

23e8ede

weren't used by any component and just produce unused code and complicate reading

extend PgSQLDataEncoderProcessor, make responsible for all data encod…

93d634e

…ing/decoding operations over interesting data for decryption/detokenization received from database

parse ParameterDescription + RowDescription packets,

42f6c0a

update OID values according to ColumnEncryptionSetting change value encoding flow for encrypted integers Remove encoding/decoding logic from Column struct, use only from PostgresqlEncoderDecoder and BoundValue encodings

add missing file

be63ed4

Merge remote-tracking branch 'origin/master' into lagovas/encoding-da…

fb773ec

…ta-according-to-data-type

refactor ColumnData:

fac7758

- don't use DecodedData struct - store raw data on protocol handler level - encode/decode data in OnColumn handler on higher level

fix processing binary data, refactor encoding/decoding binary and tex…

e992328

…t formatted data

log keystore's folder used on startup

530eb3b

use once initialized ctx instead of creating two new

1b97ca1

use logger related to context instead of global in handleBindPacket f…

c0b878f

…unction

store data about used placeholders and related ColumnEncryptionSettin…

fdeaa4c

…g in SQL queries to encrypt bound values

handle text format for binds too

0e8819e

fix parameter description updates

validate token type in encryption_config

26a8b13

fix token type validation

9c13ec7

fix processing insert queries

bfcc1e3

fix saving placeholder's data and add unit tests

7937285

add missing files

79990e1

move pgsql data encoder from pseudonymization to decryptor/postgresql…

83ebae9

… package

fix processing integer types

81e3176

fix encoding strings for postgresql

ab1f79c

convert comparable data to bytes due to receiving as bytes from db dr…

edb9616

…ivers

use separate setting field for data type tampering

9b7e0ba

add integration test

c60df93

Zhaars added 4 commits March 29, 2022 13:20

zhars/mysql_type_awareness

f09fdb9

basic implementation of encoding data types for MySQL

zhars/mysql-encoding-data-types

8d494e0

updated columnInfo type with origin type tracking

zhars/mysql-encoding-data-types

e20c304

fixed conflicts

Zhaars requested a review from Lagovas March 29, 2022 17:10

zhars/mysql-encoding-data-types

bfcc1dd

restore deleted test file

Lagovas reviewed Mar 30, 2022

View reviewed changes

Zhaars added 3 commits April 5, 2022 17:18

zhars/mysql-encoding-data-types

bfb34ac

Added intergration tests

zhars/mysql-encoding-data-types

0d6eb3c

Delete redundant changes

zhars/mysql-encoding-data-types

5703ee6

Fixed logging

Lagovas reviewed Apr 5, 2022

View reviewed changes

Zhaars added 3 commits April 6, 2022 13:49

zhars/mysql-encoding-data-types

5439e39

Fixed After review

Merge branch 'master' into zhars/mysql-encoding-data-types

585235a

# Conflicts: # encryptor/config/encryptionSettings.go

zhars/mysql-encoding-data-types

e433e36

Merge master changes

Zhaars marked this pull request as ready for review April 6, 2022 17:42

zhars/mysql-encoding-data-types

0c789bc

updated CHANGELOG_DEV.md file

Zhaars requested a review from Lagovas April 7, 2022 08:27

zhars/mysql-encoding-data-types

137cf22

Added addinonal logging/comments

Lagovas reviewed Apr 7, 2022

View reviewed changes

Zhaars added 3 commits April 7, 2022 14:18

zhars/mysql-encoding-data-types

4c832bf

Fixed after review

zhars/mysql-encoding-data-types

28bf269

Fixed failing tests

zhars/mysql-encoding-data-types

73fe4d5

replace string() convertion to more efficient utils.BytesToString()

Zhaars requested a review from Lagovas April 7, 2022 14:13

Lagovas approved these changes Apr 7, 2022

View reviewed changes

zhars/mysql-encoding-data-types

d9ac430

replace string() convertion to more efficient utils.BytesToString()

Zhaars merged commit 9a71cd1 into master Apr 7, 2022

Zhaars mentioned this pull request Apr 8, 2022

Update params/columns data_types for prepare packet #523

Merged

7 tasks

Lagovas deleted the zhars/mysql-encoding-data-types branch May 19, 2022 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MySQL encoding data types #517

MySQL encoding data types #517

Zhaars commented Mar 29, 2022 •

edited

Loading

Lagovas Mar 30, 2022

Lagovas Mar 30, 2022

Lagovas Mar 30, 2022

Zhaars Apr 6, 2022

Lagovas Mar 30, 2022

Lagovas Mar 30, 2022

Lagovas Mar 30, 2022

Lagovas Mar 30, 2022

Lagovas Apr 5, 2022

Zhaars Apr 6, 2022

Lagovas Apr 5, 2022

Zhaars Apr 6, 2022

Lagovas Apr 7, 2022

Lagovas Apr 5, 2022

Zhaars Apr 6, 2022

Lagovas Apr 5, 2022

Lagovas Apr 5, 2022

Zhaars Apr 6, 2022

Lagovas Apr 7, 2022

Zhaars Apr 7, 2022

Lagovas Apr 7, 2022

Zhaars Apr 7, 2022

Lagovas Apr 7, 2022

Lagovas Apr 7, 2022

Zhaars Apr 7, 2022 •

edited

Loading

Lagovas Apr 7, 2022

Lagovas commented Apr 7, 2022

Lagovas Apr 7, 2022

Lagovas left a comment

	intValue, err = strconv.ParseInt(*newVal, 10, 64)
	intValue, err = strconv.ParseInt(*newVal, 10, 32)

	intValue, err := strconv.ParseInt(string(data), 10, 32)
	intValue, err := strconv.ParseInt(string(data), 10, 64)

	// updating filed type according to DataType provided in schemaStore
	// updating field type according to DataType provided in schemaStore

MySQL encoding data types #517

MySQL encoding data types #517

Conversation

Zhaars commented Mar 29, 2022 • edited Loading

Checklist

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

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

Zhaars Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lagovas commented Apr 7, 2022

Choose a reason for hiding this comment

Lagovas left a comment

Choose a reason for hiding this comment

Zhaars commented Mar 29, 2022 •

edited

Loading

Zhaars Apr 7, 2022 •

edited

Loading