Skip to content

feat: allow prefixes at the beginning of subdomain app hostnames #10150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 10, 2023

Conversation

deansheather
Copy link
Member

@deansheather deansheather commented Oct 9, 2023

Changes hostname parsing and signed app token generation/verification to allow for (prefix---)?app--agent--workspace--user in subdomain app URLs. Signed app tokens now include the prefix in their payload and will verify it on new requests.

Old signed app tokens will become invalid (but they have an expiry of 1 minute anyways so it doesn't matter). They actually won't become invalid because prefixes weren't a thing before so the JSON parsing will default to "".

The prefix must be separated by 3 hyphens to distinguish it from the rest of the segments in the URL. This is to allow for changes to our URL format (like adding or removing segments) in the future without conflicting with prefixes.

E.g. https://frontend---8080--main--dev--deansheather.example.com is the same app as https://8080--main--dev--deansheather.example.com.

Apps can view the full Host so they can route based on this prefix if they wish.

TODO:

  • Put behind a configuration flag (disabled by default)

Closes #9903

@Emyrk
Copy link
Member

Emyrk commented Oct 9, 2023

Do we need to have it toggleable?

@deansheather
Copy link
Member Author

I guess we don't have to

"github.com/coder/coder/v2/provisioner/echo"
"github.com/coder/coder/v2/provisionersdk/proto"
"github.com/coder/coder/v2/testutil"
)

const (
proxyTestAgentName = "agent-name"
proxyTestAgentName = "agnt-name"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol did we hit a limit in the test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we hit the 62 char limit lol, Marcin reverted his PR though so it should be fine now hopefully

@MrPeacockNLB
Copy link
Contributor

Hi @deansheather,

does "we can change URL format later" mean you can fix this #9186 ? We are planing to deploy a combination of WAP + WAF in front of Coder. So this mean we have to disable this standard OWASP rule in order to get this working.

@deansheather
Copy link
Member Author

does "we can change URL format later" mean you can fix this #9186 ? We are planing to deploy a combination of WAP + WAF in front of Coder. So this mean we have to disable this standard OWASP rule in order to get this working.

@MrPeacockNLB we don't have any plans to fix that issue. We use double hyphens so we can support single hyphens in app names, agent names, workspace names and usernames.

@deansheather
Copy link
Member Author

The "we can change the URL format later" is more about adding or removing segments. The agent name segment is not necessary if you provide an app slug because apps are unique per template, not per agent. Currently the URL format requires the agent name, but in the future we could make it not required.

@deansheather deansheather requested a review from Emyrk October 10, 2023 17:23
Copy link
Member

@Emyrk Emyrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you if you want to make it a deployment toggle

@deansheather deansheather enabled auto-merge (squash) October 10, 2023 19:52
@deansheather deansheather merged commit e7d9b8d into main Oct 10, 2023
@deansheather deansheather deleted the dean/app-url-prefix branch October 10, 2023 20:02
@github-actions github-actions bot locked and limited conversation to collaborators Oct 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature request: subdomain on applications
3 participants