Skip to content

improve postgres bind params audit logging#34321

Merged
GavinFrazar merged 1 commit intomasterfrom
gavinfrazar/update-postgres-bind-logging
Nov 9, 2023
Merged

improve postgres bind params audit logging#34321
GavinFrazar merged 1 commit intomasterfrom
gavinfrazar/update-postgres-bind-logging

Conversation

@GavinFrazar
Copy link
Copy Markdown
Contributor

@GavinFrazar GavinFrazar commented Nov 7, 2023

This PR affects Postgres Bind/FunctionCall event audit logging:

  • logs parameters with same format for all params when one format code is given
  • log binary format params as base64 encoded string

changelog: Added binary formatted parameters as base64 encoded strings to PostgreSQL Statement Bind audit log events

* log with same format for all params when one format code is given
* log binary format params as base64 encoded string
@GavinFrazar GavinFrazar added audit-log Issues related to Teleports Audit Log db/postgres PostgreSQL related database access issues changelog labels Nov 7, 2023
@GavinFrazar GavinFrazar requested a review from jentfoo November 7, 2023 22:59
@github-actions github-actions Bot added database-access Database access related issues and PRs size/sm labels Nov 7, 2023
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 7, 2023

The PR changelog entry failed validation: The changelog entry must start with a letter.

// one format code applies the same format code to all params.
// https://www.postgresql.org/docs/current/protocol-message-formats.html#PROTOCOL-MESSAGE-FORMATS-BIND
// https://www.postgresql.org/docs/current/protocol-message-formats.html#PROTOCOL-MESSAGE-FORMATS-FUNCTIONCALL
if len(formatCodes) > 1 && len(formatCodes) != len(parameters) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we handle the case where len(formatCodes) == 1 and len(parameters) == 0 ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if using a prepared statement with > 0 parameters, then passing 0 parameters will be a postgres error.
If the prepared statement has 0 parameters and you pass 0 parameters with 1 format code, it's not an error, but you didn't bind anything so there's no parameters to log anyway. I don't think there's any point in generating a log warning

@GavinFrazar GavinFrazar enabled auto-merge November 9, 2023 21:22
@GavinFrazar GavinFrazar added this pull request to the merge queue Nov 9, 2023
Merged via the queue into master with commit 56d3acd Nov 9, 2023
@GavinFrazar GavinFrazar deleted the gavinfrazar/update-postgres-bind-logging branch November 9, 2023 21:54
@public-teleport-github-review-bot
Copy link
Copy Markdown

@GavinFrazar See the table below for backport results.

Branch Result
branch/v12 Failed
branch/v13 Failed
branch/v14 Create PR

GavinFrazar added a commit that referenced this pull request Nov 9, 2023
Backport #34321 to branch/v13.

* log with same format for all params when one format code is given
* log binary format params as base64 encoded string
GavinFrazar added a commit that referenced this pull request Nov 9, 2023
Backport #34321 to branch/v12.

* log with same format for all params when one format code is given
* log binary format params as base64 encoded string
github-merge-queue Bot pushed a commit that referenced this pull request Nov 10, 2023
Backport #34321 to branch/v13.

* log with same format for all params when one format code is given
* log binary format params as base64 encoded string
github-merge-queue Bot pushed a commit that referenced this pull request Nov 10, 2023
Backport #34321 to branch/v12.

* log with same format for all params when one format code is given
* log binary format params as base64 encoded string
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

audit-log Issues related to Teleports Audit Log database-access Database access related issues and PRs db/postgres PostgreSQL related database access issues size/sm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants