[mangeshraut712]

Fixes #4824

Summary

Changes

With --connection_method PG_WIRE_IAM_PROTOCOL, the password is a generated IAM auth token, so the --secret_arn secret only needs a username. Both IAM code paths, however, read the secret through get_credentials_from_secret(), which raises when there is no password key.

• connection/psycopg_pool_connection.py: the secret parsing moves into a private _read_credentials_from_secret(..., require_password), and a new get_username_from_secret() reads only the username. get_credentials_from_secret() behaves the same as before: it returns (username, password) and still raises Secret does not contain password.
• server.py:
  • The IAM branch of internal_create_connection() now calls get_username_from_secret().
  • validate_secret_arn_at_startup() takes a require_password argument (default True). main() passes False only when --connection_method is PG_WIRE_IAM_PROTOCOL, so PG_WIRE_PROTOCOL, RDS_API, and the lazy-connect mode (no --connection_method) still require a password, as before.
• Tests:
  • New unit tests for get_username_from_secret(): a username-only secret is accepted, a password is ignored if present, a missing username still raises, and is_test works. A further test confirms get_credentials_from_secret() still rejects a username-only secret.
  • Two main() tests: IAM mode accepts a username-only secret at startup, and PG_WIRE_PROTOCOL still exits 1 on one.
  • An IAM-path test for internal_create_connection() runs a real {"username": "app_reader"} payload through the reader with only boto3 mocked.
  • The existing IAM tests now mock get_username_from_secret instead of get_credentials_from_secret.
• CHANGELOG entry under Unreleased / Fixed.
• .secrets.baseline: line numbers updated by the detect-secrets pre-commit hook, because existing test fixtures moved down 4 lines.

User experience

Before: with a secret of {"username": "app_reader"}, starting the server with --connection_method PG_WIRE_IAM_PROTOCOL --secret_arn <secret> exited with MCP server cannot start: ... ValueError: Failed to retrieve credentials from Secrets Manager: Secret does not contain password. Available keys: username.

After: the server starts and connects as app_reader using an IAM auth token, so no placeholder password has to be stored in Secrets Manager. The password-based connection methods behave as before.

Checklist

• I have reviewed the contributing guidelines
• I have performed a self-review of this change
• Changes have been tested
• Changes are documented

Tests: uv run --frozen pytest tests --ignore=tests/e2e (2507 passed), plus ruff check, ruff format --check, pyright (0 errors), and pre-commit run on the changed files, all passing. I did not run the e2e suite, which needs a live AWS cluster.

Is this a breaking change? (Y/N) N

RFC issue number: N/A

Checklist:

• Migration process documented
• Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

[mangeshraut712]

Friendly ping — still green and ready when you get to it. Lets PG_WIRE_IAM_PROTOCOL secrets be username-only, since the password is the IAM token.

[mangeshraut712]

@kennthhz @thephantomthief CI is green and the contributor statement check is passing. It lets PG_WIRE_IAM_PROTOCOL secrets be username-only, since the password is the IAM token. If it looks good, could you please review and merge when you get a chance? Thanks.

[mangeshraut712]

CI is green. Accepts username-only secret for PG_WIRE_IAM_PROTOCOL. Please review and merge when you can.

[kennthhz]

Thanks for picking this up — the diagnosis in #4824 is correct and splitting the secret reader is the right shape. _read_credentials_from_secret with a require_password flag keeps get_credentials_from_secret's contract intact, which is what I'd want.

Requesting changes on five items, all inline. The one that matters most:

This must also work in lazy-connect mode. Right now the fix only takes effect when --connection_method PG_WIRE_IAM_PROTOCOL is passed at startup. connect_to_database accepts connection_method as a runtime parameter, so IAM is fully supported without pinning it at startup — and in that mode a username-only secret still exits 1 before the user can connect at all. The PR describes this as intentional ("the lazy-connect mode ... still require[s] a password, as before"), but it leaves the reported problem unfixed in a supported configuration. Details and a suggested gate inline.

The other four are a missing argparse choices= that turns a bad --connection_method into a misleading secret error, an unenforced cast, an error message that wraps itself, and two test gaps. None are large.

Once lazy mode is covered and the rest are addressed I'm happy to re-review quickly.

[kennthhz]

Must fix — the fix doesn't apply in lazy-connect mode.

This is computed unconditionally, so when --connection_method is omitted it evaluates to True and the startup probe demands a password. But omitting it is a supported mode: connect_to_database takes connection_method as a runtime parameter, so an operator can legitimately start without pinning a method and then connect with PG_WIRE_IAM_PROTOCOL.

In that configuration a username-only secret still fails exactly as reported in #4824 — and the irony is that the IAM connect path you fixed would work fine; the user just never reaches it, because startup exits first.

The important detail that makes relaxing this safe: the password requirement isn't lost, it's deferred. PsycopgPoolConnection still calls get_credentials_from_secret() at connect time on the pgwire path, so a secret missing a password surfaces then, with the method actually known and a clear error. The startup probe is a fail-fast convenience, not the enforcement point.

So I'd invert the gate to require a password only for the methods that definitely need one:

secret_requires_password = args.connection_method in {
    ConnectionMethod.PG_WIRE_PROTOCOL.name,
    ConnectionMethod.RDS_API.name,
}

Absent (lazy mode) then means "don't require a password at startup," which is the correct answer when the method is unknown. Paired with the choices= fix in my other comment, "unknown" stops being reachable, so the only relaxed case is genuine lazy mode.

Please add a test for lazy mode plus a username-only secret — that's the case that's currently broken.

[kennthhz]

Must fix — --connection_method has no choices=, and this comparison makes a bad value fail misleadingly.

ConnectionMethod is a str Enum whose values differ from its names:

RDS_API = 'rdsapi'
PG_WIRE_PROTOCOL = 'pgwire'
PG_WIRE_IAM_PROTOCOL = 'pgwire_iam'

The argparse definition (around line 1398) accepts any free-form string, and resolution happens by name via ConnectionMethod[args.connection_method] at line 1583. The lowercase values are what appear in connect_to_database responses and in the README, so --connection_method pgwire_iam is a very natural thing to type.

Because this line runs at 1554 and the enum lookup is at 1583, that invocation with a username-only secret reports:

MCP server cannot start: ... Secret does not contain password. Available keys: username

The secret is blamed for what is actually an unrecognized connection method, and the user is sent off to edit Secrets Manager. This PR makes the coupling worse by tying the secret requirement to the raw string.

Fix at the parser so bad input dies immediately with a useful message:

parser.add_argument(
    '--connection_method',
    choices=[m.name for m in ConnectionMethod],
    help='Connection method to the database: RDS_API, PG_WIRE_PROTOCOL, or PG_WIRE_IAM_PROTOCOL',
)

That also makes the comparison on this line safe by construction.

[kennthhz]

Must fix — this invariant is asserted in a comment but not enforced.

cast() is a no-op at runtime, so the only thing guaranteeing a non-None password here is the prose above it. If _read_credentials_from_secret is ever changed — a new early return, a different falsy check — this function silently returns (str, None) while its signature promises Tuple[str, str], and the failure surfaces somewhere far away as a None password handed to libpq.

Make it hold at runtime:

username, password = _read_credentials_from_secret(secret_arn, region, require_password=True)
if password is None:  # pragma: no cover - require_password=True guarantees this
    raise ValueError('Secret does not contain password')
return username, password

That also lets the type checker narrow it without a cast, so the import can go.

[kennthhz]

Must fix — this re-wraps your own validation errors into a doubled message.

The raise ValueError('Secret does not contain username...') and '...does not contain password...' raises inside the try are caught by this same except Exception and wrapped again, producing what the issue reports:

Failed to retrieve credentials from Secrets Manager: Secret does not contain password. Available keys: username

"Failed to retrieve" is wrong — retrieval succeeded; the payload was missing a field. That misdirects anyone debugging toward IAM permissions or networking.

This is pre-existing, but the PR is already restructuring this function and it's the error text users will hit most, so I'd like it fixed here. Either move the shape validation out of the try so only the boto3 call is wrapped, or let your own errors pass through:

except ValueError:
    raise
except Exception as e:
    logger.exception(...)
    raise ValueError(f'Failed to retrieve credentials from Secrets Manager: {str(e)}')

Worth checking whether any test asserts on the doubled string — if so, those assertions should be updated rather than the behavior preserved.

[kennthhz]

Must fix — two gaps in the new main() coverage.

First, neither new test covers lazy-connect mode: both pass an explicit --connection_method, so the case that's still broken (no --connection_method plus a username-only secret) has no test. That's the scenario in my comment on server.py:1554, and it should fail today.

Second, both tests omit --db_type, so main() returns before reaching ConnectionMethod[args.connection_method] at line 1583. The tests therefore never exercise the interaction between this string gate and the enum lookup that consumes the same value — which is exactly where the name-vs-value mismatch lives.

Three cases I'd want:

1. No --connection_method + username-only secret → starts (after the gate is fixed).
2. --connection_method PG_WIRE_IAM_PROTOCOL + --db_type set → reaches internal_create_connection with the IAM method resolved, confirming gate and lookup agree.
3. An invalid --connection_method → rejected by argparse with a method error, not a secret error.

Otherwise the tests are well built — asserting call_order and that the wrong probe raises AssertionError if called are both good touches.

[kennthhz]

Non-blocking nit: this ignores a password key if the secret has one, which is correct for IAM, but silent. An operator who thinks they configured password auth gets no signal that the stored password is unused. A logger.debug noting it was present and ignored would save a confusing debugging session.