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

Jump to bottom

MYST 355: limit openvpn client reconnects #167

Merged
merged 2 commits into from Feb 21, 2018
Merged

MYST 355: limit openvpn client reconnects #167

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
686badc
limit openvpn client reconnects
zolia Feb 20, 2018
0039575
renamed SetReconnectLimits to RestrictReconnects
zolia Feb 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 0 additions & 19 deletions bin/server_package/config/ca.crt
View file
Open in desktop

This file was deleted.

10 changes: 0 additions & 10 deletions bin/server_package/config/crl.pem
View file
Open in desktop

This file was deleted.

8 changes: 0 additions & 8 deletions bin/server_package/config/dh.pem
View file
Open in desktop

This file was deleted.

83 changes: 0 additions & 83 deletions bin/server_package/config/server.crt
View file
Open in desktop

This file was deleted.

28 changes: 0 additions & 28 deletions bin/server_package/config/server.key
View file
Open in desktop

This file was deleted.

21 changes: 0 additions & 21 deletions bin/server_package/config/ta.key
View file
Open in desktop

This file was deleted.

1 change: 1 addition & 0 deletions location/detector_test.go
View file
Open in desktop
Expand Up @@ -16,6 +16,7 @@ func TestDetectorDetectCountry(t *testing.T) {
{"95.85.39.36", "NL", ""},
{"127.0.0.1", "", ""},
{"8.8.8.8.8", "", "failed to parse IP"},
{"185.243.112.225", "", ""},
Copy link
Contributor

@donce donce Feb 20, 2018 •
edited

Choose a reason for hiding this comment

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

If we return error when we are unable to found country in database, using country detector would be much easier - if error was not returned, that means country was returned :)

This doesn't have to be solved in this PR, but since you're adding such case, we can add a TODO just to track this :)

Copy link
Member

@Waldz Waldz Feb 20, 2018 •
edited

Choose a reason for hiding this comment

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

return errors.New("unkown country for IP: x.x.x")

{"asd", "", "failed to parse IP"},
}

Expand Down
7 changes: 7 additions & 0 deletions openvpn/config.go
View file
Open in desktop
Expand Up @@ -60,6 +60,13 @@ func (c *Config) SetTLSCrypt(cryptFile string) {
c.AddOptions(OptionFile("tls-crypt", cryptFile))
}

func (c *Config) SetReconnectLimits() {
c.setParam("connect-retry-max", "2")
Copy link
Member

Choose a reason for hiding this comment

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

Is it ok, that we throw such parameters server->client.
Should not client amend those under his/her choice?

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 believe no. At least not in here. This is just openvpn related. True connect / reconnect attempts should go from tequila api. Failure to connect should probably mean either real connectivity issues or failed auth due to state / outdated auth procedure in dialog stage.

c.setParam("remap-usr1", "SIGTERM")
c.setFlag("single-session")
c.setFlag("tls-exit")
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of params doesn't seem to be used for reconnection limit, which is the purpose of this method. I.e. tls-exit and remap-usr1 seems to be used for allowing to exit vpn in some graceful way.

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 use limit in broader sense, not a number, but limitation, not to reconnect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ant these are parameters that enforces that limitation..

Copy link
Contributor

@donce donce Feb 20, 2018 •
edited

Choose a reason for hiding this comment

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

i use limit in broader sense, not a number, but limitation, not to reconnect.

I don't quite understand what you meant here - anyways, documentation for this method is missing - maybe documenting this would explain what broader sense you have?

}

func (c *Config) SetKeepAlive(interval, timeout int) {
c.setParam("keepalive", strconv.Itoa(interval)+" "+strconv.Itoa(timeout))
}
Expand Down
1 change: 1 addition & 0 deletions openvpn/factory.go
View file
Open in desktop
Expand Up @@ -40,6 +40,7 @@ func NewClientConfig(
config.SetClientMode(remote, 1194)
config.SetTLSCACertificate(caCertPath)
config.SetTLSCrypt(tlsCryptKeyPath)
config.SetReconnectLimits()
Copy link
Contributor

Choose a reason for hiding this comment

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

If number 2 were a parameter to this method, we could see what that limit is in here - also, that would allow us to change it in factory without changing config class itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

intuitively I want to pass parameters to a function that has a set prefix.
perhaps RestrictConnectRetryMax(2)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restrict prob better term here. Will rename.


config.SetDevice("tun")
config.setParam("cipher", "AES-256-GCM")
Expand Down