Draft
Conversation
pquadri
force-pushed
the
feat/rds-auth
branch
from
dd5fbb3 to
ef825b1
Compare
Implements RDS IAM-based authentication using the AWS CLI to generate
temporary auth tokens instead of static passwords. Tokens are cached
for 13 minutes (expiry is 15 min) to avoid spawning a new process for
every pool connection.
Usage:
CREATE SECRET rds_secret (
TYPE POSTGRES,
HOST 'my-db.xxxxxx.us-east-1.rds.amazonaws.com',
PORT '5432',
USER 'my_iam_user',
DATABASE 'mydb',
USE_RDS_IAM_AUTH true,
AWS_REGION 'us-east-1' -- optional, uses AWS CLI default if omitted
);
ATTACH '' AS rds_db (TYPE POSTGRES, SECRET rds_secret);
Implementation notes:
- Token generation via `aws rds generate-db-auth-token` (inherits env for
AWS_PROFILE, instance roles, web-identity, etc.)
- Token cache keyed on (hostname, port, username, region) with a 13-min TTL
- RDS params are extracted from the secret at ATTACH time so
CreateNewConnection() can regenerate tokens without a ClientContext
- stderr from the aws command propagates to the process stderr so users
see credential errors directly
- Debug logging (pg_debug_print_queries) prints only the token prefix+length
Switch from popen/shell-out to a direct fork+exec (POSIX) /
CreateProcess (Windows) subprocess helper so RDS IAM auth token
generation works on Windows and closes several safety issues
flagged in code review.
Changes:
- Add src/process_exec.{hpp,cpp}: RunProcess(argv) — no shell, no
escaping, separate stdout/stderr pipes, 30s timeout, clear
"aws not found on PATH" error on ENOENT/ERROR_FILE_NOT_FOUND.
POSIX uses fork/execvp/poll + close-on-exec error pipe to detect
exec failure without waitpid races. Windows uses CreateProcessA
with two std::thread drain workers to avoid two-pipe deadlock.
- src/storage/postgres_catalog.cpp: replace escape_shell_arg lambda
+ popen block with RunProcess call; stderr now surfaces in the
exception message on failure instead of potentially corrupting
the token.
- src/storage/postgres_connection_pool.cpp: fix two "str" + int
pointer-arithmetic bugs in exception messages (were compile
warnings, would produce garbage strings at runtime).
- CMakeLists.txt: add ${PostgreSQL_INCLUDE_DIRS} to include path
(was missing, broke builds without vcpkg).
- test/stub/aws + test/stub/aws.cmd: fake aws binary for manual
smoke-testing; prints a known token, logs invocation to
$AWS_STUB_LOG.
- Replace hand-rolled clock_gettime/timespec arithmetic with steady_clock (chrono.hpp was already a transitive dep) - Unify the 30s subprocess timeout into a single PROCESS_TIMEOUT_MS constant shared by both POSIX and Windows paths - Fix double-space in GetFreshConnectionString: rds_base_connection_string already ends with a space from AddConnectionOption, so the leading " password=" was producing "host='x' port='5432' password=..." - Document the intentional cache TOCTOU: two concurrent misses produce two valid tokens; second write wins, both tokens are valid for 15 min - Remove child-block comment that described what the code does
- postgres_extension.cpp: store use_rds_iam_auth Value directly instead of .ToString() to preserve BOOLEAN type; BooleanValue::Get() asserts type==BOOL and would crash on a VARCHAR "true" - process_exec.cpp (Windows): split CreatePipe calls so stdout handles are closed if the stderr pipe creation fails - process_exec.cpp (POSIX): split pipe() calls with proper fd cleanup on partial failure to prevent fd leaks - postgres_catalog.cpp: strip \\r before \\n from aws CLI token output to handle Windows CRLF line endings - postgres_catalog.cpp: store rds_base_connection_string without attach_path; append attach_path after the password in GetFreshConnectionString() so the connection string is well-formed when attach_path is non-empty
Collaborator
|
Hi, thanks for the PR!
M, I cannot answer this off the top of my head, there are obviously good and bad parts of bringing in AWS SDK. Meanwhile, to pass the CI format check
Or, alternatively, just apply the diff from the CI job output. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #360.
Quite dirty, but it seemed the least intrusive solution, let me know what you think about this.
It can potentially be just an experimental addition then be changed to use the AWS SDK? (i didn't want to add that as a dependency, having a soft requirement of "you need aws-cli if you want to use this" seemed better)
I'm working on having an RDS with IAM to test this properly.