Fix App public_addr allows setting a conflicting hostname with the proxy when using capital letters by cthach · Pull Request #61123 · gravitational/teleport

cthach · 2025-11-06T21:56:53Z

What

Adds extra validation on top of #58475 to ensure that if a user attempts to configure an App with a public_addr that conflicts with one of Proxy's public addresses, Teleport will fatally error. Includes extra unit test cases and fuzz tests.

Changelog: Improve conflict detection of application public address and Teleport cluster addresses.

Required Approvers 👀

🔴 Do not merge unless all of these approvers have approved 🔴

Why

The key difference from PR #58475 is that we'll now trim any trailing dots of the hostnames, then normalize by converting to ASCII and finally doing a case insensitive comparison to detect conflicts. This would catch conflicting Internationalized Domain Names (IDN), mixed casing, and a trailing dot in the name that we previously did not catch.

Scope

This PR focuses only on preventing applications from using a public_addr value that matches the public address of any proxy.

It does not aim to improve field validation for the public_addr field beyond preventing proxy address conflicts e.g., ensuring only a hostname is set, ensuring a scheme or port is not provided, etc.

Those will be handled in issues #58751 and #4838 in future PRs.

Manual Tests

My Teleport's public cluster address is teleport.dev:443.

The following tests were performed:

Configuring an app with public_addr app.teleport.dev should succeed
Configuring an app with public_addr teleport.dev should error
Configuring an app with public_addr TELEPORT.dev should error

Change the public cluster address tomünchen.dev:443 to test IDNA/unicode domains:

Configuring an app with public_addr app.münchen.dev should succeed
Configuring an app with public_addr MüncheN.dev should error
Configuring an app with public_addr MüncheN.dev. (trailing dot) should error

…roxy when using capital letters Signed-off-by: Chris Thach <chris.thach@goteleport.com>

Signed-off-by: Chris Thach <chris.thach@goteleport.com>

…-app

Signed-off-by: Chris Thach <chris.thach@goteleport.com>

rosstimothy · 2025-11-10T19:08:09Z

+		return nil
+	}
+
+	// It is assumed that the app's public address has already been validated to be a valid address format during app


// It is assumed that the app's public address has already been validated to be a valid address format during app resource validation

Can you clarify where and when this happens? Would it hurt to be redundant and validate here again?

Can you clarify where and when this happens?

It happens in the NewAppV3 constructor. It calls the CheckAndSetDefaults.

Would it hurt to be redundant and validate here again?

Hmm yes, that is a good idea to be on the defensive side. We just have to be careful that the implementation now and going forward doesn't mutate existing struct fields that are already set.

I can also see that this is an already an established patten i.e., calling CheckAndSetDefaults outside of the method receiver's constructor. Thanks for the suggestion!

Let's not call CheckAndSetDefaults explicitly here to avoid side effects. That is the biggest pain we have today with that pattern. Well that and the fact that it's called during custom marshal/unmarshal code.

Sounds good to me.

rob-picard-teleport · 2025-11-11T00:16:02Z

+
+	// Convert the application's public address hostname to its ASCII representation for comparison. Strip any trailing
+	// dot to ensure consistent comparison.
+	asciiAppHostname, err := idna.ToASCII(strings.TrimSuffix(appAddr.Host(), "."))


Could I just set my host to teleport.example.com..? Do we need to strip all . characters at the end?

Great catch! Addressed in 49d6c55

…-app 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>

…test. Signed-off-by: Chris Thach <chris.thach@goteleport.com>

rob-picard-teleport

Looks good!

Signed-off-by: Chris Thach <chris.thach@goteleport.com>

backport-bot-workflows · 2025-11-12T13:25:19Z

@cthach See the table below for backport results.

Branch	Result
branch/v17	Failed
branch/v18	Failed

…oxy when using capital letters (#61123) * fix: App public_addr allows setting a conflicting hostname with the proxy when using capital letters Signed-off-by: Chris Thach <chris.thach@goteleport.com> * Only normalize once. Remove app public_addr validate. Signed-off-by: Chris Thach <chris.thach@goteleport.com> * Fix grpc tests Signed-off-by: Chris Thach <chris.thach@goteleport.com> * Polish and optimize ValidateApp Signed-off-by: Chris Thach <chris.thach@goteleport.com> * Apply suggestions from code review Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> * Strip all trailing dots Signed-off-by: Chris Thach <chris.thach@goteleport.com> * NewAppV3 should never panic during fuzzing. Improve comments in fuzz test. Signed-off-by: Chris Thach <chris.thach@goteleport.com> * Clarify app spec validated in CheckAndSetDefaults 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>

…oxy when using capital letters (#61123) (#61292) * fix: App public_addr allows setting a conflicting hostname with the proxy when using capital letters * Only normalize once. Remove app public_addr validate. * Fix grpc tests * Polish and optimize ValidateApp * Apply suggestions from code review * Strip all trailing dots * NewAppV3 should never panic during fuzzing. Improve comments in fuzz test. * Clarify app spec validated in CheckAndSetDefaults --------- Signed-off-by: Chris Thach <chris.thach@goteleport.com> Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>

…oxy when using capital letters (#61123) (#61290) * fix: App public_addr allows setting a conflicting hostname with the proxy when using capital letters * Only normalize once. Remove app public_addr validate. * Fix grpc tests * Polish and optimize ValidateApp * Apply suggestions from code review * Strip all trailing dots * NewAppV3 should never panic during fuzzing. Improve comments in fuzz test. * Clarify app spec validated in CheckAndSetDefaults --------- Signed-off-by: Chris Thach <chris.thach@goteleport.com> Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>

fix: App public_addr allows setting a conflicting hostname with the p…

e1eb85f

…roxy when using capital letters Signed-off-by: Chris Thach <chris.thach@goteleport.com>

cthach self-assigned this Nov 6, 2025

cthach added security Security Issues backport-required sec-internal Security Vulnerability - Reported by employees or auditors, no known exploitation backport/branch/v17 backport/branch/v18 labels Nov 6, 2025

cthach commented Nov 6, 2025

View reviewed changes

Comment thread lib/services/app.go Outdated

cthach commented Nov 6, 2025

View reviewed changes

Comment thread lib/services/app.go Outdated

cthach added 4 commits November 6, 2025 19:11

Only normalize once. Remove app public_addr validate.

84332b1

Signed-off-by: Chris Thach <chris.thach@goteleport.com>

Fix grpc tests

291929d

Signed-off-by: Chris Thach <chris.thach@goteleport.com>

Merge remote-tracking branch 'origin/master' into cthach/fix-validate…

8a2dbe8

…-app

Polish and optimize ValidateApp

eb1f8e3

Signed-off-by: Chris Thach <chris.thach@goteleport.com>

cthach force-pushed the cthach/fix-validate-app branch from 7efc819 to eb1f8e3 Compare November 7, 2025 23:29

cthach marked this pull request as ready for review November 7, 2025 23:40

Merge branch 'master' into cthach/fix-validate-app

ae002f2

cthach requested review from rob-picard-teleport, rosstimothy and zmb3 November 7, 2025 23:40

github-actions bot added the size/md label Nov 7, 2025

github-actions bot requested review from kopiczko and ravicious November 7, 2025 23:41

cthach changed the title ~~fix: App public_addr allows setting a conflicting hostname with the proxy when using capital letters~~ Fix App public_addr allows setting a conflicting hostname with the proxy when using capital letters Nov 10, 2025

rosstimothy reviewed Nov 10, 2025

View reviewed changes

rob-picard-teleport reviewed Nov 11, 2025

View reviewed changes

cthach and others added 4 commits November 11, 2025 14:24

Merge remote-tracking branch 'origin/master' into cthach/fix-validate…

af2b1a2

…-app Signed-off-by: Chris Thach <chris.thach@goteleport.com>

Apply suggestions from code review

f474783

Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>

Strip all trailing dots

49d6c55

Signed-off-by: Chris Thach <chris.thach@goteleport.com>

NewAppV3 should never panic during fuzzing. Improve comments in fuzz …

bdb86cc

…test. Signed-off-by: Chris Thach <chris.thach@goteleport.com>

rob-picard-teleport approved these changes Nov 11, 2025

View reviewed changes

Clarify app spec validated in CheckAndSetDefaults

e5f3fe0

Signed-off-by: Chris Thach <chris.thach@goteleport.com>

cthach requested a review from rosstimothy November 11, 2025 20:17

Merge branch 'master' into cthach/fix-validate-app

ce322d5

rosstimothy approved these changes Nov 11, 2025

View reviewed changes

adrian-doyensec approved these changes Nov 12, 2025

View reviewed changes

cthach added this pull request to the merge queue Nov 12, 2025

Merged via the queue into master with commit 65e73bc Nov 12, 2025
45 checks passed

cthach deleted the cthach/fix-validate-app branch November 12, 2025 13:23

cthach mentioned this pull request Nov 12, 2025

[v18] Fix App public_addr allows setting a conflicting hostname with the proxy when using capital letters #61290

Merged

cthach mentioned this pull request Nov 12, 2025

[v17] Fix App public_addr allows setting a conflicting hostname with the proxy when using capital letters #61292

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix App public_addr allows setting a conflicting hostname with the proxy when using capital letters#61123

Fix App public_addr allows setting a conflicting hostname with the proxy when using capital letters#61123
cthach merged 12 commits intomasterfrom
cthach/fix-validate-app

cthach commented Nov 6, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

rosstimothy Nov 10, 2025

Uh oh!

cthach Nov 10, 2025

Uh oh!

rosstimothy Nov 10, 2025

Uh oh!

cthach Nov 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rob-picard-teleport Nov 11, 2025

Uh oh!

cthach Nov 11, 2025

Uh oh!

rob-picard-teleport left a comment

Uh oh!

Uh oh!

backport-bot-workflows bot commented Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

cthach commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Required Approvers 👀

Why

Scope

Manual Tests

Uh oh!

Uh oh!

Uh oh!

rosstimothy Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

cthach Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

rosstimothy Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

cthach Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rob-picard-teleport Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

cthach Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

rob-picard-teleport left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

backport-bot-workflows bot commented Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cthach commented Nov 6, 2025 •

edited

Loading