-
Notifications
You must be signed in to change notification settings - Fork 1
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
base: main
Are you sure you want to change the base?
Add OSSH #1098
Conversation
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 |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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("", "") |
There was a problem hiding this comment.
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.
** 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. |
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" { |
There was a problem hiding this comment.
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?
Should this still be a draft @hwh33, or is it ready to go? On the coverage, feel free to adjust the sensitivity. |
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. |
Merging in |
Oh I also increased the threshold for coverage-decrease-sensitivity in this repo to 1%. |
Oh this also depends on getlantern/http-proxy#472, which is not yet reviewed |
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.