Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Test if auth host is reachable #11409

Merged
merged 1 commit into from
Jul 18, 2022
Merged

Test if auth host is reachable #11409

merged 1 commit into from
Jul 18, 2022

Conversation

AlexTugarev
Copy link
Member

@AlexTugarev AlexTugarev commented Jul 15, 2022 •
edited
Loading

Description

This PR removes a conservative host name validation using a plain regex. Instead we test and report if the host (optionally with port) is reachable.

Screen Shot 2022-07-15 at 12 24 06

Related Issue(s)

Improves the situation around #5305

How to test

Try configuring and using a Git Provider which is listening on a port other than the default 443, thus requires to specify a port.

Release Notes

Test if host of a Git Integration is reachable.

Documentation

Werft options:

  • /werft with-preview

@AlexTugarev AlexTugarev requested a review from a team July 15, 2022 09:42
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Jul 15, 2022
@AlexTugarev AlexTugarev force-pushed the at/auth-host-reachable branch from a517436 to 872521a Compare July 15, 2022 09:56
@AlexTugarev
Copy link
Member Author

AlexTugarev commented Jul 15, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-at-auth-host-reachable.2
(with .werft/ from main)

@AlexTugarev AlexTugarev marked this pull request as draft July 15, 2022 09:56
@AlexTugarev AlexTugarev force-pushed the at/auth-host-reachable branch from 872521a to ce93a51 Compare July 15, 2022 10:12
@AlexTugarev AlexTugarev added the aspect: security Anything related to preventing vulnerabilities label Jul 15, 2022
@AlexTugarev AlexTugarev marked this pull request as ready for review July 15, 2022 10:34
@AlexTugarev AlexTugarev mentioned this pull request Jul 15, 2022
Closed
geropl
geropl reviewed Jul 15, 2022
View reviewed changes
components/server/src/auth/auth-provider-service.ts
return this.config.hostUrl.with({ pathname }).toString();
};

async isHostReachable(host: string) {
return await isReachable(host, { timeout: 2000 });
Copy link
Member

Choose a reason for hiding this comment

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

💯

geropl
geropl approved these changes Jul 15, 2022
View reviewed changes
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Approving the code to unblock. 🚀

/hold so we can test against actual self-hosted SCMs

@AlexTugarev
Copy link
Member Author

AlexTugarev commented Jul 15, 2022 •
edited by werft-gitpod-dev-com bot
Loading

BTW I tagged it security relevant, as I was wondering about any chances to override existing saas providers like gihub.com with something like github.com:433 and somehow interfere with authorizations and basically enable scamming.

I tested and verified that it's not possible to use github.com:433, gitlab.com:433 ✅

cc. @geropl

@geropl geropl self-assigned this Jul 15, 2022
@jldec
Copy link
Contributor

jldec commented Jul 15, 2022

Nice - this is a helpful improvement. Thanks @AlexTugarev.

@mrsimonemms mrsimonemms self-requested a review July 15, 2022 13:15
mrsimonemms
mrsimonemms approved these changes Jul 15, 2022
View reviewed changes
Copy link
Contributor

@mrsimonemms mrsimonemms left a comment

Choose a reason for hiding this comment

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

I've done a few tests on it and it's working as expected - good work.

I created a GitLab instance on http://20.0.96.8:81/ and https://gitlab..com and was able to verify the connection ok. NB the IP address wasn't able to login as we require HTTPS, but the domain one worked fine.

@AlexTugarev
Copy link
Member Author

/unhold

I see no risk in just trying this out.

@roboquat roboquat merged commit 60f4bad into main Jul 18, 2022
@roboquat roboquat deleted the at/auth-host-reachable branch July 18, 2022 06:49
@lucasvaltl
Copy link
Contributor

lucasvaltl commented Jul 19, 2022 •
edited by werft-gitpod-dev-com bot
Loading

👋 This is awesome and was a very quick turnaround! One question: Did we add something to the docs to indicate that this is now possible - for example in this section?

@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Jul 19, 2022
@AlexTugarev AlexTugarev mentioned this pull request Aug 2, 2022
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geropl geropl geropl approved these changes

@mrsimonemms mrsimonemms mrsimonemms approved these changes

Assignees

@geropl geropl

Labels
aspect: security Anything related to preventing vulnerabilities deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note size/M team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@AlexTugarev @jldec @lucasvaltl @mrsimonemms @geropl @roboquat