[SHOT-97] fix: target container names instead of bare service hostnames#7894
[SHOT-97] fix: target container names instead of bare service hostnames#7894mimartin12 wants to merge 4 commits into
Conversation
nginx proxy_pass and the internal identity service URI used bare Docker service names, which can resolve to the wrong container when another container sharing the network answers to that name, returning 502s and misrouting auth traffic. Point them at the unique bitwarden-* container names. - NginxConfig.hbs: all proxy_pass upstreams use bitwarden-* names - EnvironmentFileBuilder.cs: pin internalIdentity and the Key Connector identityServerUri to http://bitwarden-identity:5000
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This reviews an infrastructure fix that retargets nginx No new findings. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| { | ||
| ["globalSettings__baseServiceUri__vault"] = _context.Config.Url, | ||
| ["globalSettings__baseServiceUri__cloudRegion"] = _context.Install?.CloudRegion.ToString(), | ||
| ["globalSettings__baseServiceUri__internalIdentity"] = "http://bitwarden-identity:5000", |
There was a problem hiding this comment.
❓ QUESTION: Are the other internal service URIs intentionally left on bare service names?
Details
This pins internalIdentity to http://bitwarden-identity:5000, but the other internal URIs still fall through to BuildInternalUri's default, which produces bare service names (http://identity:5000 style). For example, InternalNotifications resolves to http://notifications:5000 and is actively consumed by NotificationsApiPushEngine (src/Core/Platform/Push/Engines/NotificationsApiPushEngine.cs:28), so it shares the exact same cross-container hostname-collision failure mode this PR is fixing for identity.
If SHOT-97 is scoped only to auth traffic that's fine — just confirming whether InternalNotifications/InternalApi/InternalVault/InternalSso (and the mssql DataSource = "tcp:mssql,1433" at line 71) were deliberately left out of this change.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7894 +/- ##
==========================================
+ Coverage 61.21% 61.23% +0.02%
==========================================
Files 2219 2220 +1
Lines 98059 98110 +51
Branches 8848 8854 +6
==========================================
+ Hits 60024 60076 +52
+ Misses 35918 35917 -1
Partials 2117 2117 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
1 participant
🎟️ Tracking
https://bitwarden.atlassian.net/browse/SHOT-97
📔 Objective
Fixes: #5692
nginx proxy_pass and the internal identity service URI used bare Docker service names, which can resolve to the wrong container when another container sharing the network answers to that name, returning 502s and misrouting auth traffic. Point them at the unique bitwarden-* container names.
📸 Screenshots