-
Notifications
You must be signed in to change notification settings - Fork 1
Error or warn if encrypt config missing #196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 11 commits
d76c858
f61e822
858f688
2d74ceb
1b66cf8
5eee78f
620804b
f78dd27
af8c2b7
8352c71
605e81c
c454963
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,7 +86,7 @@ where | |
| let sent: u64 = bytes.len() as u64; | ||
| counter!(CLIENTS_BYTES_RECEIVED_TOTAL).increment(sent); | ||
|
|
||
| if self.encrypt.is_passthrough() { | ||
| if self.encrypt.config.mapping_disabled() { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| self.write_to_server(bytes).await?; | ||
| return Ok(()); | ||
| } | ||
|
|
@@ -785,8 +785,7 @@ where | |
| msg = "Configured column", | ||
| column = ?identifier | ||
| ); | ||
| let col = self.get_column(identifier)?; | ||
| Some(col) | ||
| self.get_column(identifier)? | ||
| } | ||
| _ => None, | ||
| }; | ||
|
|
@@ -822,8 +821,7 @@ where | |
| column = ?identifier | ||
| ); | ||
|
|
||
| let col = self.get_column(identifier)?; | ||
| Some(col) | ||
| self.get_column(identifier)? | ||
| } | ||
| _ => None, | ||
| }; | ||
|
|
@@ -850,7 +848,9 @@ where | |
| identifier = ?identifier | ||
| ); | ||
| let col = self.get_column(identifier)?; | ||
| literal_columns.push(Some(col)); | ||
| if col.is_some() { | ||
| literal_columns.push(col); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -860,9 +860,9 @@ where | |
|
|
||
| /// | ||
| /// Get the column configuration for the Identifier | ||
| /// Returns `EncryptError::UnknownColumn` if configuratiuon cannot be found for the Identified column | ||
| /// Returns `EncryptError::UnknownColumn` if configuration cannot be found for the Identified column | ||
| /// | ||
| fn get_column(&self, identifier: Identifier) -> Result<Column, Error> { | ||
| fn get_column(&self, identifier: Identifier) -> Result<Option<Column>, Error> { | ||
| match self.encrypt.get_column_config(&identifier) { | ||
| Some(config) => { | ||
| debug!( | ||
|
|
@@ -871,20 +871,24 @@ where | |
| msg = "Configured column", | ||
| column = ?identifier | ||
| ); | ||
| Ok(Column::new(identifier, config)) | ||
| Ok(Some(Column::new(identifier, config))) | ||
| } | ||
| None => { | ||
| debug!( | ||
| warn!( | ||
| target: MAPPER, | ||
| client_id = self.context.client_id, | ||
| msg = "Configured column not found ", | ||
| msg = "Configured column not found. Encryption configuration may have been deleted.", | ||
| ?identifier, | ||
| ); | ||
| Err(EncryptError::UnknownColumn { | ||
| table: identifier.table.to_owned(), | ||
| column: identifier.column.to_owned(), | ||
| if self.encrypt.config.mapping_errors_enabled() { | ||
| Err(EncryptError::UnknownColumn { | ||
| table: identifier.table.to_owned(), | ||
| column: identifier.column.to_owned(), | ||
| } | ||
| .into()) | ||
| } else { | ||
| Ok(None) | ||
| } | ||
| .into()) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,7 +54,10 @@ def test_encrypted_column_with_no_configuration(): | |
|
|
||
| sql = "INSERT INTO unconfigured (id, encrypted_unconfigured) VALUES (%s, %s)" | ||
|
|
||
| with pytest.raises(psycopg.Error, match='#encrypt-unknown-column'): | ||
| # 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"): | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| cursor.execute(sql, [id, val]) | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| #!/usr/bin/env bash | ||
| #MISE description="Test for warning about missing encrypt config. Run with mapping enabled and mapping error disabled" | ||
|
|
||
| # This test assumes that Proxy is running with mapping enabled with mapping error disabled | ||
|
|
||
| set -e | ||
|
|
||
| # Simulate delete config by renaming | ||
| docker exec -i postgres"${CONTAINER_SUFFIX}" psql 'postgresql://cipherstash:password@proxy:6432/cipherstash' --command 'ALTER TABLE encrypted RENAME COLUMN encrypted_text TO unconfigured_text;' >/dev/null 2>&1 | ||
|
|
||
| set +e | ||
| TIMESTAMP=$(date -u +%Y-%m-%dT%H:%M:%SZ) | ||
| docker exec -i postgres"${CONTAINER_SUFFIX}" psql 'postgresql://cipherstash:password@proxy:6432/cipherstash?sslmode=disable' --command 'SELECT unconfigured_text from encrypted' >/dev/null 2>&1 | ||
| LOG_CONTENT=$(docker logs --since "${TIMESTAMP}" proxy | tr "\n" " ") | ||
| EXPECTED='Encryption configuration may have been deleted' | ||
|
|
||
| # Simulate delete config by renaming | ||
| docker exec -i postgres"${CONTAINER_SUFFIX}" psql 'postgresql://cipherstash:password@proxy:6432/cipherstash' --command 'ALTER TABLE encrypted RENAME COLUMN unconfigured_text TO encrypted_text;' >/dev/null 2>&1 | ||
|
|
||
| if echo "$LOG_CONTENT" | grep -v "${EXPECTED}"; then | ||
| echo "error: did not see string in output: \"${EXPECTED}\"" | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 improveThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.