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.

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

Add OSSH #1098

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Add OSSH #1098

wants to merge 9 commits into from

Conversation

hwh33
Copy link
Contributor

@hwh33 hwh33 commented May 19, 2021

Depends on https://github.com/getlantern/ossh/pull/3 and getlantern/http-proxy#472. I'll keep this a draft until those are merged, then pull in a tagged version of each.

go.mod Outdated
@@ -21,7 +21,7 @@ require (
github.com/getlantern/appdir v0.0.0-20200615192800-a0ef1968f4da
github.com/getlantern/auth-server v0.0.0-20210413141251-d7ab3c6fa18a
github.com/getlantern/autoupdate v0.0.0-20180719190525-a22eab7ded99
github.com/getlantern/borda v0.0.0-20200613191039-d7b1c2cc6021
github.com/getlantern/borda v0.0.0-20210122163308-eccb55d42214
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A number of these changes are because devel was in need of a go mod tidy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was true when I originally opened this PR and is again true now =)

@@ -223,15 +238,14 @@ func serveContent(resp http.ResponseWriter, req *http.Request) {
}

func (helper *Helper) startProxyServer() error {
kcpConfFile, err := ioutil.TempFile("", "")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a TestResourcesDir to the Helper type for stuff like this. I broke out this KCP configuration file stuff into a helper function and used the new resources directory.

@hwh33
Copy link
Contributor Author

hwh33 commented May 19, 2021

** IMPORTANT NOTE **

This pulls in GPL code (Psiphon's OSSH library). We have been considering doing this for the message work anyway, but still, we should probably make sure we're ready to commit before merging this PR.

@hwh33
Copy link
Contributor Author

hwh33 commented May 19, 2021

Hmm, the coverage CI test failed because coverage decreased by 0.4%. I like that we're looking after test coverage, but methinks that's a bit aggressive =)

@@ -204,7 +206,7 @@ func createImpl(configDir, name, addr, transport string, s *ChainedServerInfo, u

if s.MultiplexedAddr != "" || transport == "utphttp" ||
transport == "utphttps" || transport == "utpobfs4" ||
transport == "tlsmasq" {
transport == "tlsmasq" || transport == "ossh" {
Copy link
Contributor Author

@hwh33 hwh33 May 22, 2021

Choose a reason for hiding this comment

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

This would mean that OSSH is always multiplexed. Would we ever not want to multiplex?

@myleshorton
Copy link
Contributor

Should this still be a draft @hwh33, or is it ready to go? On the coverage, feel free to adjust the sensitivity.

@hwh33
Copy link
Contributor Author

hwh33 commented Oct 19, 2021

This pulls in GPL code, so it's really just waiting on a decision on that front. Have we made one? If we're ready to merge GPL code in, I'll just take a minute to dust these changes off.

I'll adjust the coverage sensitivity.

@hwh33
Copy link
Contributor Author

hwh33 commented Oct 19, 2021

Merging in devel was not too bad, so I just went ahead and did that. This is ready to go pending a decision on GPL code in the client.

@hwh33
Copy link
Contributor Author

hwh33 commented Oct 19, 2021

Oh I also increased the threshold for coverage-decrease-sensitivity in this repo to 1%.

@hwh33
Copy link
Contributor Author

hwh33 commented Oct 19, 2021

Oh this also depends on getlantern/http-proxy#472, which is not yet reviewed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants