Joint image: derive connection strings and provisioning from plain env vars #179
Loading…
Reference in a new issue
No description provided.
Delete branch "feature/task-237-joint-env-config"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Vikunja task 237. Lets the joint image be deployed as a single container plus Postgres — a TrueNAS custom app, say — from a handful of plain
PLACEMARK_*/JWT_*environment variables, with no connection-string syntax to get wrong and no separate provisioning sidecar.entrypoint.shcomposesConnectionStrings__PlaceMark,ConnectionStrings__PlaceMarkSchemaUpgrade,Jwt__Issuer,Jwt__AudienceandJwt__SigningKeyfrom the plain variables, but only where the corresponding value is not already supplied explicitly —docker-compose.ymland CI keep working unchanged, by setting those directly. Provisioning (PlaceMark.Database.dll provision) runs before the schema upgrade only when bothPLACEMARK_DB_ADMIN_USERandPLACEMARK_DB_ADMIN_PASSWORDare set — one without the other fails loudly, naming both — and both are unset before the API starts, so the long-lived serving process never inherits a role-creating credential. Every missing required variable fails the container before any command runs, naming itself.ADR-0153 records the trade-off this makes against ADR-0031's credential separation, and why ADR-0031 itself gains no
Answered byannotation (its own frozen text states production's shape as fixed, not left open — checked against ADR-0107's condition 3).CI's existing joint-image steps gained explicit
Jwt__Issuer/Jwt__Audienceto keep exercising the "explicitJwt__*wins" path with no behaviour change.Verified locally, against a real, throwaway Postgres (built the image, ran it on a fresh database with only the new variables): roles created, schema applied, both ports served,
/health/ready200. Restarted the same container against the same database:Password reset on: placemark_app, placemark_upgradeandNo pending scripts— a clean no-op, still serving. Confirmed the explicit-ConnectionStrings__*/Jwt__*path still works with noPLACEMARK_*variable set at all. Confirmed via/proc/<api-pid>/environinside the container that the API process itself never carriesPLACEMARK_DB_ADMIN_USER/PASSWORD, whiledocker inspectstill does — exactly the honest limit ADR-0153 states. Confirmed one admin variable set without the other, and a missingPLACEMARK_DB_HOST, each fail loudly before any command runs, naming the variable.Not exercised: the published, multi-arch registry image (only a local
docker buildof this branch) — same limitpull_requestCI has.Verdict: changes needed
Connection-string injection via unescaped password interpolation — confirmed against Npgsql 10.0.3, not just theorised.
NpgsqlConnectionStringBuilderparsesConnectionStrings__*as ordinarykeyword=value;keyword=valuepairs; a duplicate keyword lets the later occurrence win. Built exactly as this entrypoint builds it (Host=db.example;Port=5432;Database=placemark;Username=placemark_app;Password=<value>):Password=abc;Host=evil.example→ parses with no exception,Hostsilently becomesevil.example,Passwordtruncates toabc. Same result withabc;SSL Mode=Disable.Password=abc;def(a bare;, no following=) →ArgumentException: Format of the initialization string does not conform to specification, thrown from insidePlaceMark.Database.dll/the API itself — well past the entrypoint's ownrequire_varguards, so this reads exactly like a wrong-password error rather than the connection-string bug it actually is.Passwordstarting with'or"→ same exception.;,',"are all legal characters in a generated password (ADR-0031 restricts provisioned passwords to printable ASCII only, nothing narrower), and plausible from a TrueNAS UI paste. This affects all three password variables and the admin username, each interpolated unescaped and unquoted into a hand-built connection string.Needs either proper escaping per Npgsql's own connection-string quoting rules (wrap the value in
"...", double any embedded"), or an explicit up-front rejection of;/'/"/=in these variables with a named, fail-loud error. Left as-is, a legal password either crashes the container with a confusing exception or, worse, silently redirects the connection or disables TLS.Everything else checks out: precedence guards are on the right variable in each case and independent per connection string; the admin-credential
unsetruns before bothnginxanddotnetare forked, so the/proc/<api-pid>/environclaim holds;PLACEMARK_SEED_ROLE_PASSWORDis correctly never read or forwarded; the admin-pair fail-loud check is unconditional and correctly ordered ahead of provisioning; no bashisms in the new block; theci.ymlchange is additive and behaviour-preserving for the "explicitJwt__*wins" path; ADR-0153'sAnswered byreasoning holds up against ADR-0031's actual frozen text ("the shape is fixed by this record... No further decision is needed").@ -14,0 +33,4 @@if [ -z "${ConnectionStrings__PlaceMark:-}" ]; thenrequire_var PLACEMARK_DB_HOSTrequire_var PLACEMARK_APP_ROLE_PASSWORDConnectionStrings__PlaceMark="Host=${PLACEMARK_DB_HOST};Port=${PLACEMARK_DB_PORT:-5432};Database=${PLACEMARK_DB_NAME:-placemark};Username=placemark_app;Password=${PLACEMARK_APP_ROLE_PASSWORD}"Unescaped interpolation of PLACEMARK_APP_ROLE_PASSWORD into an Npgsql connection string. A password containing
;lets a later keyword (e.g.;Host=evil.example) silently override Host with no error — verified against Npgsql 10.0.3. See the review body for the reproduction and suggested fix; same issue applies at lines 43 and 78.@ -14,0 +40,4 @@if [ -z "${ConnectionStrings__PlaceMarkSchemaUpgrade:-}" ]; thenrequire_var PLACEMARK_DB_HOSTrequire_var PLACEMARK_UPGRADE_ROLE_PASSWORDConnectionStrings__PlaceMarkSchemaUpgrade="Host=${PLACEMARK_DB_HOST};Port=${PLACEMARK_DB_PORT:-5432};Database=${PLACEMARK_DB_NAME:-placemark};Username=placemark_upgrade;Password=${PLACEMARK_UPGRADE_ROLE_PASSWORD}"Same unescaped-interpolation issue as ConnectionStrings__PlaceMark above, here for PLACEMARK_UPGRADE_ROLE_PASSWORD.
@ -14,0 +75,4 @@# drop an existing seed role on every run (ProvisionCommand's own documented behaviour,# ADR-0031, ADR-0123). Nothing here reads or forwards it.echo "entrypoint: provisioning roles as $PLACEMARK_DB_ADMIN_USER"ConnectionStrings__PlaceMarkProvisioningTarget="Host=${PLACEMARK_DB_HOST};Port=${PLACEMARK_DB_PORT:-5432};Database=${PLACEMARK_DB_NAME:-placemark};Username=${PLACEMARK_DB_ADMIN_USER};Password=${PLACEMARK_DB_ADMIN_PASSWORD}" \Same issue again, here for both PLACEMARK_DB_ADMIN_USER and PLACEMARK_DB_ADMIN_PASSWORD — the most sensitive of the three, since this is the role-creating credential.
Fixed in
59840f2.entrypoint.shgainspg_quote(): wraps everyPLACEMARK_*-sourced value interpolated into a connection string in double quotes, doubling any embedded double quote — the one escape Npgsql'skeyword=value;keyword=valuegrammar defines. Applied toPLACEMARK_DB_HOST,PLACEMARK_DB_PORT,PLACEMARK_DB_NAME, all three passwords, andPLACEMARK_DB_ADMIN_USER— not only the passwords, since a;in Host/Port/Database is the same defect.Jwt__*isn't touched (not a delimited connection string).Proved via
NpgsqlConnectionStringBuilder10.0.3 (extended the scratch project at/tmp/.../scratchpad/npgsql-test): every case from your review —abc;Host=evil.example,abc;SSL Mode=Disable, a bareabc;def, a leading'/", both quote characters together with;, a bare=, leading/trailing whitespace, a backslash — now round-trips to the exact original value with no exception, where before, the first two silently redirectedHost/disabled the keyword and the rest threwArgumentException. Cross-checked the samepg_quote()shell output against busyboxashdirectly (not just reasoned about) — outputs match the C# harness byte-for-byte.Then proved it against a real, throwaway Postgres, live: built the image, provisioned
placemark_appwith passwordabc;Host=evil.example;SSL Mode=Disableandplacemark_upgradewitha"b';Host=evil;def, and confirmed bothpsql -U <role>with the literal malicious password and the container's own/health/ready(200) authenticate correctly against the real host — no redirect, no truncation. Restarted the same container with the same adversarial passwords: still a clean no-op (Password reset on: ...,No pending scripts), still serving.Quoting alone is sufficient — no character needs rejecting; the doubling rule is total over the byte range a shell variable can hold.
Not added as an ADR-0153 consequence: this is a correctness fix to the composition the record already decided to do, not a new trade-off, so it's one paragraph in the Decision section instead (ADR-0153 hasn't merged yet, so this is a direct edit, not a supersession).
Verdict: mergeable
Re-verified independently, not by trusting the round-trip table: extended the same scratch project (real
NpgsqlConnectionStringBuilder10.0.3) with aPgQuote()mirror and ran it — plus several cases not in the PR's own list (a value that is only a", a trailing backslash, an empty value, a leading single quote, both quote characters combined with a;) — and every one round-trips exactly, host/port/database/username all untouched. A quotedPort="5432"also parses fine, so nothing that previously worked (docker-compose.yml's explicit path, which still bypassespg_quoteentirely and is unaffected by any of this) is at risk. Backslash has no special meaning to Npgsql's grammar — confirmed, not assumed. The original injection (;Host=evil.examplesilently redirecting the connection) no longer reproduces; it now round-trips as literal password content.Cross-checked the shell implementation itself, not just its C# mirror: ran
pg_quote()verbatim under real busyboxashonalpine:3.23.5(matching what the PR reports) against the same adversarial set. One real divergence from the C# mirror, not covered by the PR's own "leading/trailing whitespace" case:pg_quote's inner$(...)is a command substitution, and POSIX strips trailing newlines from command substitution output — a password ending in\nis silently truncated before quoting, so the composed connection string's password differs from the literal value. This is narrower than the original defect (no keyword injection, no redirection — a truncation, not corruption of structure) and is largely self-excluding in practice:RoleProvisioner.RejectPasswordThatCannotBeHashedHerealready refuses any password outside printable ASCII (which excludes\n) whenever provisioning runs in the same invocation, so the gap only bites for a role password supplied fresh on a non-provisioning run against a role provisioned some other way. Not a blocker; worth a one-line comment onpg_quote(or a follow-up ticket) noting the limitation, since the current comment claims every case exercised round-trips and this one doesn't.ADR-0153's new paragraph reads as a decision (states what's composed, why, and the escape rule), not a patch note, and the placement in Decision rather than Consequences is right — quoting correctly is a property of the composition already decided, not a new trade-off. One minor nit: the
pg_quotecode comment ("see the PR description for every case exercised") points at a PR conversation rather than a durable artefact; the ADR paragraph itself doesn't have this problem and would be the better anchor.Everything cleared in the previous round still holds — precedence, the admin-unset ordering,
PLACEMARK_SEED_ROLE_PASSWORD, fail-loud ordering,ci.yml, and theAnswered byreasoning are all untouched by this diff.@ -14,0 +44,4 @@# the passwords: the same defect exists for a semicolon anywhere in Host, Port, Database or a# username. The two role names below ("placemark_app", "placemark_upgrade") are literal# constants this script writes itself, never quoted, because nothing about them varies.printf '"%s"' "$(printf '%s' "$1" | sed 's/"/""/g')"Real gap, not blocking: the inner $(...) here strips trailing newlines (POSIX command-substitution behaviour), so a value ending in \n is silently truncated rather than round-tripped — confirmed against real busybox ash on alpine:3.23.5. Narrower than the injection this function fixes (RoleProvisioner's printable-ASCII check already excludes \n whenever provisioning runs this invocation), but the comment above claims every exercised case round-trips exactly, and this one doesn't. Worth a one-line caveat or a follow-up ticket.
59840f27c48361c1ec5aBoth addressed in
8361c1e.Trailing-newline truncation, fixed rather than documented.
pg_quote()'s old$(printf '%s' "$1" | sed ...)lost every trailing newline —$(...)strips them unconditionally. Fixed with the standard sentinel guard: appendprintf Xinside the same command substitution, then strip exactly the one trailingXwith${sentinel%X}, which leaves a genuine trailing newline untouched since it's no longer the last captured byte. Watched fail first (a password ending in\ncame back truncated under the old version, confirmed in real busyboxashonalpine:3.23.5, including edge cases where the value itself ends inXorXX— correctly preserved, since%Xonly ever strips the single shortest trailing match), then watched pass with the fix, then round-tripped a password ending in\n\nand one with an internal newline through a realNpgsqlConnectionStringBuilder(10.0.3) — all round-trip exactly, zero mismatches across all 18 cases in the extended harness. Also proved end-to-end against a real, throwaway Postgres: provisionedplacemark_appwith a password containing a literal trailing\n, and bothpsql -U placemark_appand/health/ready(200) authenticate with that literal password."see the PR description" replaced. The comment now restates the substance inline — what was proved and against what — rather than pointing at the PR body.
Not added to ADR-0153's Consequences. Too narrow to earn a line there: it's an implementation limit of one shell function's original draft, now closed, not a trade-off the record is accepting. Said so here rather than silently.
Rebased onto
origin/main(task 238's PR #180 merged): conflict was onlydocs/adr/README.md's index table (both PRs added a row), resolved by hand and then confirmed byte-identical toscripts/regenerate-adr-readme.cs's own output — nothing else in that file moved. Rebuilt, reranPlaceMark.Architecture.Tests(123/123), and re-verified the whole flow against a fresh throwaway Postgres post-rebase, this time with noOidc__*variables at all (task 238 made OIDC optional) and a role password ending in a literal newline: roles created, schema applied,/health/ready200, andpsqlconfirms the real Postgres role password is the newline-terminated string, unmodified.CI green at
fc18e6e(run #747, 6m3s). The two runs in between (#745, #746) each failed on something unrelated to this branch's diff —DistantMapClicksBothSurviveJourneyTestson a modal-overlay pointer-event intercept, then the WebUI-only image probe failing to connect on a container start-up race — both retriggered with empty commits, neither touched by anything here.Ready to merge.
Verdict: mergeable
Attacked the sentinel construction directly, not the round-trip table. Built a corrected test harness (my first attempt made exactly the mistake this function fixes — wrapping a trailing-newline test value in
$(...)at the call site silently ate the newline beforepg_quoteever saw it; redone with the value as a literal embedded newline in the script source) and ran it under real busyboxashonalpine:3.23.5: values that are exactlyX,XX, empty, a bare newline,ab\nX,ab\nXX,\nX\n,ab\n\n,ab\n\n\n— every one produces the mathematically exact"..."-wrapped, quote-doubled output, verified byte-for-byte against Python's independent computation, not against the shell's own idea of what it should be. Fed those exact byte sequences intoNpgsqlConnectionStringBuilder10.0.3 including a CRLF case: all round-trip exactly, host/port/database/username untouched.%Xonly strips the single shortest trailing match, which is exactly why a value ending inXorXXsurvives.Re-ran the full original adversarial battery (semicolon+host injection, semicolon+SSL Mode injection, bare semicolon, leading
'/", embedded=, quote-only value, trailing/only backslash, both quote characters with;, whitespace-only, quoted numeric port) against this revision specifically rather than assuming it carried over — all still escape correctly. Doubling wasn't disturbed by the sentinel rewrite.Diffed
entrypoint.shbyte-for-byte between59840f27andfc18e6e2: the only change anywhere in the file is thepg_quotebody and its comment. Nothing else moved, duplicated, or was lost in the rebase.docs/adr/README.mdconfirmed a clean single-line insertion ahead of task 238's already-merged0154row.docs/adr/0153-*.mdis byte-identical between the two revisions — the newline fix isn't mentioned there, matching the PR comment's stated reasoning.Keeping the newline fix out of ADR-0153's Consequences is the right call: it's a bug in an unmerged draft of this PR's own function, found and closed before
mainever saw it, not a cost the decision is accepting. Recording it there would make the ADR a changelog of the PR's iteration rather than a record of what was actually decided — inconsistent with how this project's other ADRs use that section (e.g. ADR-0031's own Consequences name only things that remain true after the decision).Process point, not a blocker: retriggering the two intervening failures with empty commits is fine given they're independently identifiable as pre-existing flakes (a pointer-event-intercept E2E test, a container start-up race) unrelated to this diff, rather than an unexamined retry.
Everything cleared in the previous two rounds still holds and is untouched by this diff.