Skip to content

Block incoming connection requests which are not allowed by current access lists#882

Merged
soffokl merged 4 commits intomasterfrom
apply-acl
Apr 16, 2019
Merged

Block incoming connection requests which are not allowed by current access lists#882
soffokl merged 4 commits intomasterfrom
apply-acl

Conversation

@soffokl
Copy link
Copy Markdown
Member

@soffokl soffokl commented Apr 16, 2019

Closes #878

@soffokl soffokl requested a review from donce April 16, 2019 10:25
@soffokl soffokl self-assigned this Apr 16, 2019
Copy link
Copy Markdown
Contributor

@vkuznecovas vkuznecovas left a comment

Choose a reason for hiding this comment

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

I'm not sure. I think the implementation should account for the fact that most of the service providers will be running 24/7 for months at a time with no service restarts. Therefore I think we should periodically update the list of allowed ID's and:

a) don't let anyone who's not whitelisted access our service
b) remove active sessions from ID's that are not whitelisted(since the list might change)

registered, err := di.IdentityRegistry.IsRegistered(peerID)
if err != nil {
return err
} else if !registered {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the else is redundant

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's not.
It's else if just for a smaller number of lines instead of two if. :)

@donce
Copy link
Copy Markdown
Contributor

donce commented Apr 16, 2019

I'm not sure. I think the implementation should account for the fact that most of the service providers will be running 24/7 for months at a time with no service restarts. Therefore I think we should periodically update the list of allowed ID's and:

a) don't let anyone who's not whitelisted access our service
b) remove active sessions from ID's that are not whitelisted(since the list might change)

You're totally right - this is what we discussed in grooming meeting before starting implementing, and we agreed (together with business at TL) that we need to do this, but it's not a priority. For starters, it's enough to use the rules which are fetched once.

Added a task in https://github.com/mysteriumnetwork/mysterium-vpn/issues/456 not to forget about this thing :)

donce
donce previously approved these changes Apr 16, 2019
Copy link
Copy Markdown
Contributor

@donce donce left a comment

Choose a reason for hiding this comment

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

Let's merge this PR to start testing. We can work on periodic fetching and forbidden identities kicking in parallel.

Comment thread core/service/manager.go Outdated
Comment thread cmd/service_bootstrap_desktop.go Outdated
Copy link
Copy Markdown
Contributor

@donce donce left a comment

Choose a reason for hiding this comment

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

CI is feiling

@soffokl soffokl requested a review from vkuznecovas April 16, 2019 11:35
@soffokl soffokl merged commit 8b06f39 into master Apr 16, 2019
@soffokl soffokl deleted the apply-acl branch April 16, 2019 11:37
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.

Block incoming connection requests which are not allowed by current access lists

3 participants