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

MYST 355: limit openvpn client reconnects #167

Merged
merged 2 commits into from Feb 21, 2018

Conversation

Projects
None yet
4 participants
@zolia
Copy link
Member

commented Feb 20, 2018

currently, if node identity changes, client auth simply fails and client hangs in reconnecting state. this PR ensures, that if auth failed, do not attempt to reconnect. Myst client now reports "not connected".

@zolia zolia changed the title limit openvpn client reconnects MYST 355: limit openvpn client reconnects Feb 20, 2018

@@ -1,19 +0,0 @@
-----BEGIN CERTIFICATE-----

This comment has been minimized.

Copy link
@donce

donce Feb 20, 2018

Contributor

Why are these removed?

This comment has been minimized.

Copy link
@zolia

zolia Feb 20, 2018

Author Member

these are generated now, leftovers from the old auth model.

This comment has been minimized.

Copy link
@donce

donce Feb 20, 2018

Contributor

If it was in a separate commit with an explanatory message, it would cause less confusion :)

@@ -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", "", ""},

This comment has been minimized.

Copy link
@donce

donce Feb 20, 2018

Contributor

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 :)

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 20, 2018

Member

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

c.setParam("connect-retry-max", "2")
c.setParam("remap-usr1", "SIGTERM")
c.setFlag("single-session")
c.setFlag("tls-exit")

This comment has been minimized.

Copy link
@donce

donce Feb 20, 2018

Contributor

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.

This comment has been minimized.

Copy link
@zolia

zolia Feb 20, 2018

Author Member

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

This comment has been minimized.

Copy link
@zolia

zolia Feb 20, 2018

Author Member

ant these are parameters that enforces that limitation..

This comment has been minimized.

Copy link
@donce

donce Feb 20, 2018

Contributor

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?

@@ -40,6 +40,7 @@ func NewClientConfig(
config.SetClientMode(remote, 1194)
config.SetTLSCACertificate(caCertPath)
config.SetTLSCrypt(tlsCryptKeyPath)
config.SetReconnectLimits()

This comment has been minimized.

Copy link
@donce

donce Feb 20, 2018

Contributor

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.

This comment has been minimized.

Copy link
@shroomist

shroomist Feb 20, 2018

Contributor

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

This comment has been minimized.

Copy link
@zolia

zolia Feb 21, 2018

Author Member

Restrict prob better term here. Will rename.

@@ -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")

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 20, 2018

Member

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

This comment has been minimized.

Copy link
@zolia

zolia Feb 21, 2018

Author Member

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.

@Waldz

Waldz approved these changes Feb 21, 2018

@zolia zolia merged commit 80125c4 into master Feb 21, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@zolia zolia deleted the feature/MYST-355-limit-openvpn-client-reconnects branch Feb 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.