Error or warn if encrypt config missing#196
Conversation
Now Proxy warns or raises error (depending on enable_mapping_errors) if configuration is missing for an encrypted column when mapping is not disabled.
Warning for missing encrypt config test assumes mapping enabled and mapping errors disabled.
yujiyokoo
force-pushed
the
error-or-warn-if-encrypt-config-missing
branch
from
4590c39 to
5eee78f
Compare
Also update comments explaining the subtle error differences
| // returns cs_encrypted_v1 and the client cannot convert to a string. | ||
| // If mapping errors are enabled (enable_mapping_errors or CS_DEVELOPMENT__ENABLE_MAPPING_ERRORS), | ||
| // then Proxy will return an error that says "Column X in table Y has no Encrypt configuration" | ||
| assert_eq!(msg, "error serializing parameter 1: cannot convert between the Rust type `&str` and the Postgres type `cs_encrypted_v1`"); |
There was a problem hiding this comment.
Unfortunately, this is not as clear now, but with mapping errors disabled (default, and how it is here) we emit a warning, and return the value to the client in cs_encrypted_v1, and this is the client (tokio-postgres) reporting an error. Not sure what else we can do to improve
There was a problem hiding this comment.
I've run into this a couple of times with tokio-postgres and it can be a bit confusing.
Not much we can do, because is actually tokio enforcing type correctness - the statement types have not been rewritten.
| # This is EQL catching the error and returning it. Details are in docs/errors.md | ||
| # When mapping errors are enabled, (enable_mapping_errors or CS_DEVELOPMENT__ENABLE_MAPPING_ERRORS) | ||
| # Proxy will return an error that says "Column X in table Y has no Encrypt configuration" | ||
| with pytest.raises(psycopg.Error, match=r"Encrypted column missing \w+ \(\w+\) field"): |
There was a problem hiding this comment.
Similarly to the other test above, we show EQL's error here when trying to insert, as we emit the warning and let the unencrypted record pass through to be caught by Postgres.
| counter!(CLIENTS_BYTES_RECEIVED_TOTAL).increment(sent); | ||
|
|
||
| if self.encrypt.is_passthrough() { | ||
| if self.encrypt.config.mapping_disabled() { |
There was a problem hiding this comment.
is_passthrough checks if mapping is disabled or encrypt config is empty. Here it's updated to only one of them
| // returns cs_encrypted_v1 and the client cannot convert to a string. | ||
| // If mapping errors are enabled (enable_mapping_errors or CS_DEVELOPMENT__ENABLE_MAPPING_ERRORS), | ||
| // then Proxy will return an error that says "Column X in table Y has no Encrypt configuration" | ||
| assert_eq!(msg, "error serializing parameter 1: cannot convert between the Rust type `&str` and the Postgres type `cs_encrypted_v1`"); |
There was a problem hiding this comment.
I've run into this a couple of times with tokio-postgres and it can be a bit confusing.
Not much we can do, because is actually tokio enforcing type correctness - the statement types have not been rewritten.
2 participants
This changes the missing config logic to:
It also adds a single test for the warning behaviour. There is no test for the error as those integration tests increase the CI time (I have tested manually though).
Acknowledgment
By submitting this pull request, I confirm that CipherStash can use, modify, copy, and redistribute this contribution, under the terms of CipherStash's choice.