Conversation
Signed-off-by: Chris Thach <chris.thach@goteleport.com>
47a562e to
37902d9
Compare
Signed-off-by: Chris Thach <chris.thach@goteleport.com>
Signed-off-by: Chris Thach <chris.thach@goteleport.com>
Signed-off-by: Chris Thach <chris.thach@goteleport.com>
Signed-off-by: Chris Thach <chris.thach@goteleport.com>
…ethod. Signed-off-by: Chris Thach <chris.thach@goteleport.com>
2e2d454 to
23fcc9b
Compare
Signed-off-by: Chris Thach <chris.thach@goteleport.com>
…d add validation check in connections handler Signed-off-by: Chris Thach <chris.thach@goteleport.com>
3b08ac5 to
e91ef38
Compare
Signed-off-by: Chris Thach <chris.thach@goteleport.com>
|
Amplify deployment status
|
| "Application %q public address %q conflicts with the Teleport Proxy public address. "+ | ||
| "If both addresses are identical, requests intended for the proxy could be misrouted to the application, "+ | ||
| "compromising security. "+ | ||
| "Configure the application to use a unique public address that does not match the proxy's public addresses. "+ | ||
| "Refer to https://goteleport.com/docs/enroll-resources/application-access/guides/connecting-apps/.", |
There was a problem hiding this comment.
I would consider dropping the explanation to keep the message concise and not distract from what is wrong and how to remediate.
| "Application %q public address %q conflicts with the Teleport Proxy public address. "+ | |
| "If both addresses are identical, requests intended for the proxy could be misrouted to the application, "+ | |
| "compromising security. "+ | |
| "Configure the application to use a unique public address that does not match the proxy's public addresses. "+ | |
| "Refer to https://goteleport.com/docs/enroll-resources/application-access/guides/connecting-apps/.", | |
| "Application %q public address %q conflicts with the Teleport Proxy public address. "+ | |
| "Configure the application to use a unique public address that does not match the proxy's public addresses. "+ | |
| "Refer to https://goteleport.com/docs/enroll-resources/application-access/guides/connecting-apps/.", |
| "If both addresses are identical, requests intended for the proxy could be misrouted to the application, " + | ||
| "compromising security. " + |
There was a problem hiding this comment.
Same suggestion as above.
| "If both addresses are identical, requests intended for the proxy could be misrouted to the application, " + | |
| "compromising security. " + |
…licts with proxy Signed-off-by: Chris Thach <chris.thach@goteleport.com>
| // ValidateApp validates the Application resource. | ||
| func ValidateApp(app types.Application, proxyGetter ProxyGetter) error { | ||
| // Prevent routing conflicts and session hijacking by ensuring the application's public address | ||
| // does not match any of the proxy's public addresses. If both addresses are identical, |
There was a problem hiding this comment.
nit: grammatically I think you mean "the public address of any proxy" (i.e. each proxy only has one)
There was a problem hiding this comment.
(i.e. each proxy only has one)
When I first started this, I assumed exactly that. After looking through the code, I found that Proxy can have more than one 🤯 and it will even start just fine if multiple addresses are set in the public_addr field (e.g. [ "p1.example.com", "p2.example.com"]
Is this a bug or a valid configuration?
There was a problem hiding this comment.
Looks like we only return one with GetPublicAddr below. Maybe this could lead to a bypass if I can set multiple, but we're only validating the first, which seems to be how it works:
// GetPublicAddr returns a public address where this server can be reached.
func (s *ServerV2) GetPublicAddr() string {
addrs := s.GetPublicAddrs()
if len(addrs) != 0 {
return addrs[0]
}
return ""
}There was a problem hiding this comment.
Interesting. I'm not sure the reasoning behind that, but there is a GetPublicAddrs() that we can use. I'll update to use that instead so we're validating all public addrs.
There was a problem hiding this comment.
Looks good to me!
| h.logger.ErrorContext(r.Context(), errMsg, "launcher_params", p) | ||
|
|
||
| // Immediately return an error since this is a critical misconfiguration 🛑 | ||
| if p.publicAddr == proxyAddr.Host() { |
There was a problem hiding this comment.
Why is this if statement here, but we log the error to warn admins outside of it?
Signed-off-by: Chris Thach <chris.thach@goteleport.com>
Signed-off-by: Chris Thach <chris.thach@goteleport.com>
… a Teleport Proxy public address 👮🏾 (#58475) * fix: apps should not be able to set public_addr to the web proxy address Signed-off-by: Chris Thach <chris.thach@goteleport.com> * feat: add API validation Signed-off-by: Chris Thach <chris.thach@goteleport.com> * test: add coverage for UpsertApplicationServer Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: polish Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: make consistent Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: use ValidateApp func everywhere. Revert changes to Check* method. Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: dedupe Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: improve error messages for application address conflicts and add validation check in connections handler Signed-off-by: Chris Thach <chris.thach@goteleport.com> * ux: bubble up friendly error to UI Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: revert unnecessary change * fix: app public address in redirect Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: streamline proxy address validation in ValidateApp function Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: remove contact cluster admin in favor of self-service. Add logging. Signed-off-by: Chris Thach <chris.thach@goteleport.com> * Apply suggestions from code review Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> * fix: skip proxy servers with unset public addresses in ValidateApp function Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: simplify error messages for application public address conflicts with proxy Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: logging in the wrong spot Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: handle when a server has multiple public addrs Signed-off-by: Chris Thach <chris.thach@goteleport.com> --------- Signed-off-by: Chris Thach <chris.thach@goteleport.com> Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
… a Teleport Proxy public address 👮🏾 (#58475) * fix: apps should not be able to set public_addr to the web proxy address Signed-off-by: Chris Thach <chris.thach@goteleport.com> * feat: add API validation Signed-off-by: Chris Thach <chris.thach@goteleport.com> * test: add coverage for UpsertApplicationServer Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: polish Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: make consistent Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: use ValidateApp func everywhere. Revert changes to Check* method. Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: dedupe Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: improve error messages for application address conflicts and add validation check in connections handler Signed-off-by: Chris Thach <chris.thach@goteleport.com> * ux: bubble up friendly error to UI Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: revert unnecessary change * fix: app public address in redirect Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: streamline proxy address validation in ValidateApp function Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: remove contact cluster admin in favor of self-service. Add logging. Signed-off-by: Chris Thach <chris.thach@goteleport.com> * Apply suggestions from code review Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> * fix: skip proxy servers with unset public addresses in ValidateApp function Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: simplify error messages for application public address conflicts with proxy Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: logging in the wrong spot Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: handle when a server has multiple public addrs Signed-off-by: Chris Thach <chris.thach@goteleport.com> --------- Signed-off-by: Chris Thach <chris.thach@goteleport.com> Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> Signed-off-by: Chris Thach <chris.thach@goteleport.com>
… a Teleport Proxy public address 👮🏾 (#58475) * fix: apps should not be able to set public_addr to the web proxy address Signed-off-by: Chris Thach <chris.thach@goteleport.com> * feat: add API validation Signed-off-by: Chris Thach <chris.thach@goteleport.com> * test: add coverage for UpsertApplicationServer Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: polish Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: make consistent Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: use ValidateApp func everywhere. Revert changes to Check* method. Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: dedupe Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: improve error messages for application address conflicts and add validation check in connections handler Signed-off-by: Chris Thach <chris.thach@goteleport.com> * ux: bubble up friendly error to UI Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: revert unnecessary change * fix: app public address in redirect Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: streamline proxy address validation in ValidateApp function Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: remove contact cluster admin in favor of self-service. Add logging. Signed-off-by: Chris Thach <chris.thach@goteleport.com> * Apply suggestions from code review Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> * fix: skip proxy servers with unset public addresses in ValidateApp function Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: simplify error messages for application public address conflicts with proxy Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: logging in the wrong spot Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: handle when a server has multiple public addrs Signed-off-by: Chris Thach <chris.thach@goteleport.com> --------- Signed-off-by: Chris Thach <chris.thach@goteleport.com> Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> Signed-off-by: Chris Thach <chris.thach@goteleport.com>
… a Teleport Proxy public address 👮🏾 (#58475) * fix: apps should not be able to set public_addr to the web proxy address Signed-off-by: Chris Thach <chris.thach@goteleport.com> * feat: add API validation Signed-off-by: Chris Thach <chris.thach@goteleport.com> * test: add coverage for UpsertApplicationServer Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: polish Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: make consistent Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: use ValidateApp func everywhere. Revert changes to Check* method. Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: dedupe Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: improve error messages for application address conflicts and add validation check in connections handler Signed-off-by: Chris Thach <chris.thach@goteleport.com> * ux: bubble up friendly error to UI Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: revert unnecessary change * fix: app public address in redirect Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: streamline proxy address validation in ValidateApp function Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: remove contact cluster admin in favor of self-service. Add logging. Signed-off-by: Chris Thach <chris.thach@goteleport.com> * Apply suggestions from code review Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> * fix: skip proxy servers with unset public addresses in ValidateApp function Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: simplify error messages for application public address conflicts with proxy Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: logging in the wrong spot Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: handle when a server has multiple public addrs Signed-off-by: Chris Thach <chris.thach@goteleport.com> --------- Signed-off-by: Chris Thach <chris.thach@goteleport.com> Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> Signed-off-by: Chris Thach <chris.thach@goteleport.com>
… a Teleport Proxy public address 👮🏾 (#58475) (#58768) * fix: apps should not be able to set public_addr to the web proxy address * feat: add API validation * test: add coverage for UpsertApplicationServer * refactor: polish * refactor: make consistent * refactor: use ValidateApp func everywhere. Revert changes to Check* method. * refactor: dedupe * refactor: improve error messages for application address conflicts and add validation check in connections handler * ux: bubble up friendly error to UI * refactor: revert unnecessary change * fix: app public address in redirect * fix: streamline proxy address validation in ValidateApp function * refactor: remove contact cluster admin in favor of self-service. Add logging. * Apply suggestions from code review * fix: skip proxy servers with unset public addresses in ValidateApp function * refactor: simplify error messages for application public address conflicts with proxy * fix: logging in the wrong spot * fix: handle when a server has multiple public addrs --------- Signed-off-by: Chris Thach <chris.thach@goteleport.com> Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
… a Teleport Proxy public address 👮🏾 (#58475) (#58767) * fix: apps should not be able to set public_addr to the web proxy address * feat: add API validation * test: add coverage for UpsertApplicationServer * refactor: polish * refactor: make consistent * refactor: use ValidateApp func everywhere. Revert changes to Check* method. * refactor: dedupe * refactor: improve error messages for application address conflicts and add validation check in connections handler * ux: bubble up friendly error to UI * refactor: revert unnecessary change * fix: app public address in redirect * fix: streamline proxy address validation in ValidateApp function * refactor: remove contact cluster admin in favor of self-service. Add logging. * Apply suggestions from code review * fix: skip proxy servers with unset public addresses in ValidateApp function * refactor: simplify error messages for application public address conflicts with proxy * fix: logging in the wrong spot * fix: handle when a server has multiple public addrs --------- Signed-off-by: Chris Thach <chris.thach@goteleport.com> Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
… a Teleport Proxy public address 👮🏾 (#58475) (#58766) * fix: apps should not be able to set public_addr to the web proxy address * feat: add API validation * test: add coverage for UpsertApplicationServer * refactor: polish * refactor: make consistent * refactor: use ValidateApp func everywhere. Revert changes to Check* method. * refactor: dedupe * refactor: improve error messages for application address conflicts and add validation check in connections handler * ux: bubble up friendly error to UI * refactor: revert unnecessary change * fix: app public address in redirect * fix: streamline proxy address validation in ValidateApp function * refactor: remove contact cluster admin in favor of self-service. Add logging. * Apply suggestions from code review * fix: skip proxy servers with unset public addresses in ValidateApp function * refactor: simplify error messages for application public address conflicts with proxy * fix: logging in the wrong spot * fix: handle when a server has multiple public addrs --------- Signed-off-by: Chris Thach <chris.thach@goteleport.com> Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
… a Teleport Proxy public address 👮🏾 (#58475) * fix: apps should not be able to set public_addr to the web proxy address Signed-off-by: Chris Thach <chris.thach@goteleport.com> * feat: add API validation Signed-off-by: Chris Thach <chris.thach@goteleport.com> * test: add coverage for UpsertApplicationServer Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: polish Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: make consistent Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: use ValidateApp func everywhere. Revert changes to Check* method. Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: dedupe Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: improve error messages for application address conflicts and add validation check in connections handler Signed-off-by: Chris Thach <chris.thach@goteleport.com> * ux: bubble up friendly error to UI Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: revert unnecessary change * fix: app public address in redirect Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: streamline proxy address validation in ValidateApp function Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: remove contact cluster admin in favor of self-service. Add logging. Signed-off-by: Chris Thach <chris.thach@goteleport.com> * Apply suggestions from code review Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> * fix: skip proxy servers with unset public addresses in ValidateApp function Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: simplify error messages for application public address conflicts with proxy Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: logging in the wrong spot Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: handle when a server has multiple public addrs Signed-off-by: Chris Thach <chris.thach@goteleport.com> --------- Signed-off-by: Chris Thach <chris.thach@goteleport.com> Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
What?
Fixes https://github.com/gravitational/teleport-private/issues/2104.
Changelog: Prevents an application from being registered if its public address matches a Teleport cluster address.
Why?
If an application's public address conflicts with any public addresses of the Teleport cluster, it potentially enables a malicious attacker to perform session hijacking through the misconfigured application.
How?
Adds application public address validation for applications added through Teleport's config and through dynamic means.
It also updates the routing logic to prevent users of existing Teleport instances from routing to any misconfigured applications that existed before this change.
Demo
CLI
When attempting to set an application's public_addr to one that conflicts with the Teleport cluster's proxy address, an error is returned when starting Teleport.
Web UI
If a misconfigured application existed prior to these changes, the Web UI will prevent routing to that misconfigured app and will display an error message with next steps.