✨ อัปเดตล่าสุด: cossacklabs/acra/pull/

Lagovas · 2022-03-21T10:38:37Z

Added processing DataRowDescription and ParameterDescriptionData. In DataRowDescription now we replace OID values that tell drivers which data types will be returned. In ParameterDescriptionData we replace OIDs for parameters in prepared statements. To tell apps that they should pass string value instead of binary (that will be changed on Acra side)
Extend encryptor config with two new fields: default_data_value and data_type. Last values generated via protobuf in similar way as token type.
Added support of text format values in binary protocols (prepared statements). Previously was supported only binary formatted values.
Refactored QueryDataEncryptor to re-use functions related to parsing placeholders and saving encryptor's settings in context/client session values
Extended ClientSession interface to provide KV interface and store session related data for client/db packet handlers
Removed legacy acra-log-verifier config file from configs folder
Added updating context (ctx) with flag after successful decryption (base.[MarkDecryptedContext|IsDecryptedFromContext]). Needs for replacing with default values binary data types that hard to determine when they was untouched
Removed legacy interface method SubscribeOnColumnDecryption and all related methods of proxies due to not using
Refactored PgSQLDataEncoderProcessor. Now it used not only for tokenization, but for decoding all input data from database and encoding before sending to client. Also removed this logic from PostgresqlDecryptor which now responsible only for database packet parsing. All other data manipulations moved to OnQuery/OnBind/OnColumn subscribers
Additionally removed DecodedData wrapper for Column structs used by decryptor

It's ready to review. Probably last changes will be related to encryptor's config validation and updating changelogs (after that I will change it from status Draft). So welcome to review.

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

…ta-according-to-data-type

G1gg1L3s

And a few comments and wishes about git history: I think it would be better to split this PR into multiple ones. For example, each item of changes in the description could easily get its own PR. I don't mind if these small PRs would contain only change of a single line of code, because it would greatly improve the quality and speed of reviews: it's easier to separate changes and think why they are important on their own. Also, I like reading not only the final changes, but all commits one by one, to understand the logic about how and reasons why something is changing. But it was hard, because the commits often contain multiple changes at once, or are the parts of other commits. Very often they, beside some changes, they contain formatting fixes or commented code. For formatting I would recommend setup something like format on save or format on commit or something similar. Also I recommend reading about best practices about commit messages https://cbea.ms/git-commit

P.S. Sorry if some comments are outdated. They were written during the review of commits one by one, so some changes could happen in the next commits, that weren't reviewed yet.

G1gg1L3s · 2022-03-27T14:34:35Z

 				for j, value := range valTuple {
-					columnName := columnsName[j]
-					if changedValue, err := encryptor.encryptExpression(ctx, value, schema, columnName); err != nil {
+					columnName := columnsName[j%len(columnsName)]


Why is there a modulo operation?

In the vscode plugin that I use there is a support of additional diagnostics about gc-escapes and small optimizations that guarantee to apply. And when I change this to just j, this what the vscode shows:

That means the compiler was able to deduce that j is in bounds of the columnsName slice.

we use module because there should be not more than declared columns count:
insert into table1(col1, col2, col3) values (val1, val2, val3), (val4, val5, val6);
There is we have 3 columns and 3 tuples. Every tuple can have 3 values. If more than it's invalid behavior and we can skip it and pass as is to database. Because database will deny such query. But in valid case we are safe and map value to correct column.

Ok, this is reasonable. But in my opinion, it would be better to just check that if j > len(columnNames) then just skip this iteration with continue. At least we will not spend time encrypting extra values.

agree, updated

Lagovas

And a few comments and wishes about git history: I think it would be better to split this PR into multiple ones. For example, each item of changes in the description could easily get its own PR. I don't mind if these small PRs would contain only change of a single line of code, because it would greatly improve the quality and speed of reviews: it's easier to separate changes and think why they are important on their own. Also, I like reading not only the final changes, but all commits one by one, to understand the logic about how and reasons why something is changing. But it was hard, because the commits often contain multiple changes at once, or are the parts of other commits. Very often they, beside some changes, they contain formatting fixes or commented code. For formatting I would recommend setup something like format on save or format on commit or something similar. Also I recommend reading about best practices about commit messages https://cbea.ms/git-commit

P.S. Sorry if some comments are outdated. They were written during the review of commits one by one, so some changes could happen in the next commits, that weren't reviewed yet.

agree with you, will try it in next my PRs. This feature wasn't clear at the start and a lot of things were changed during the development due to existing of a lot of components linked with each other. but looking at now I see that there a lot of pieces that could be splitted into separate PRs easily *(
you prefer that I will fix current comments, commit, wait approval and after that split it into the separate PRs and review one more time or we can finish it as is and keep it in mind in the future?)
p.s. about auto-format. I already have it. But it has a small sleep before applying (to not format on every new character) and sometimes when I switch into the next tab/window after changes my IDE fails to format code) So, I will add it to pre-commit hooks to avoid such problems.

Lagovas · 2022-03-28T07:49:01Z

+	// If the configuration file contains some unknown or unsupported token type,
+	// return some safe default.


it validated here -

acra/encryptor/config/encryptionSettings.go

Line 259 in d2c125c

if err = common2.ValidateEncryptedType(dataType); err != nil {

And this function only as Getter for the field. Errors may occur in tests where such structs are initialized by value

G1gg1L3s · 2022-03-28T09:55:51Z

you prefer that I will fix current comments, commit, wait approval and after that split it into the separate PRs and review one more time or we can finish it as is and keep it in mind in the future?)

I think we can left this PR as is :)

common package decryptor/base

…ush hooks

…ta-according-to-data-type

Zhaars

Looks good for me.

G1gg1L3s

Beside one comment, everything is cool!

G1gg1L3s · 2022-03-29T15:58:41Z

+	// collect placeholder in queries to save for future ParameterDescription packet to replace according to
+	// setting's data type
+	//
+	// 32 as initial size to avoid future re-allocation. looks like enough for a lot of apps


Seems like a leftover comment

[skip ci]

G1gg1L3s · 2022-03-30T10:31:37Z

+			size = 4
+		}
+		// convert back from text to binary
+		value, err := strconv.ParseInt(string(data), 10, 64)


Small question: in all other cases we first check that value is indeed decrypted (by calling base.IsDecryptedFromContext(ctx)), and if not, we fallback to returning default values. But in this case we check decryption by parsing the number. Does it have some special meaning or reason?

the main reason is that any encrypted blob has size > len('9223372036854775807') = 19. Our acrablock has 90+ bytes of overhead. Acrastructs even more. So no one supported ciphertext cannot be valid integer string literal. So there is enough to check that it's valid valid integer string literal.

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

Lagovas added 5 commits March 23, 2022 11:35

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

d89d8a7

…ta-according-to-data-type

update changelog_dev.md

22c45eb

add tests for binary decoding

afe696a

extend config validation

90a5a5a

gofmt

d2c125c

Lagovas marked this pull request as ready for review March 24, 2022 12:06

G1gg1L3s reviewed Mar 27, 2022

View reviewed changes

Lagovas commented Mar 28, 2022

View reviewed changes

Lagovas added 11 commits March 28, 2022 15:42

encapsulate check of encrypted with type awareness columns

52296b1

correct error's message after copy-paste according to context

57dddfa

extend config validation to allow masking with type awareness

b7653df

extend comment for PostgresqlDBDataCoder

a1b7ea3

simplify encoding binary data in PostgresqlDBDataCoder

b2570a1

fix identation in tests and correct yaml configs

c9bd6c6

simplify type cast string into byte slice

baf86bb

validate default values in encryptor's config

3c89c56

encapsulate long logical check into separate function into

ef2f829

common package decryptor/base

allow to run .circleci/check_gotest.sh locally to use in pre-commit\p…

b27bb04

…ush hooks

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

a4998ff

…ta-according-to-data-type

G1gg1L3s reviewed Mar 28, 2022

View reviewed changes

Comment thread encryptor/config/encryptionSettings.go Outdated

Comment thread masking/common/patterns.go Outdated

remove commented test values

896d382

Zhaars self-requested a review March 29, 2022 12:19

Zhaars approved these changes Mar 29, 2022

View reviewed changes

avoid extra encryption in processing invalid queries by QueryEncryptor

3ca5df4

G1gg1L3s approved these changes Mar 29, 2022

View reviewed changes

remove redundant comment line

29fd34a

[skip ci]

Lagovas merged commit e522e23 into master Mar 29, 2022

G1gg1L3s reviewed Mar 30, 2022

View reviewed changes

Lagovas deleted the lagovas/encoding-data-according-to-data-type branch May 19, 2022 20:37

		// If the configuration file contains some unknown or unsupported token type,
		// return some safe default.

Conversation

Lagovas commented Mar 21, 2022

Checklist

Uh oh!

G1gg1L3s left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lagovas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

G1gg1L3s commented Mar 28, 2022

Uh oh!

Uh oh!

Uh oh!

Zhaars left a comment

Choose a reason for hiding this comment

Uh oh!

G1gg1L3s left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants