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

ufw addon #228

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

ufw addon #228

wants to merge 6 commits into from

Conversation

gadelkareem
Copy link
Contributor

Add ufw to all nodes with the following rules:

ufw enabled with the following rules:
 Status: active
Logging: on (low)
Default: deny (incoming), allow (outgoing), deny (routed)
New profiles: skip

To                         Action      From
--                         ------      ----
Anywhere                   ALLOW IN    1.2.3.4          
Anywhere                   ALLOW IN    1.2.3.5               
22                         ALLOW IN    Anywhere                  
Anywhere                   ALLOW IN    10.0.1.0/24               
Anywhere                   ALLOW IN    10.244.0.0/16             
6443                       ALLOW IN    Anywhere                  
80                         ALLOW IN    Anywhere                  
443                        ALLOW IN    Anywhere                  
22 (v6)                    ALLOW IN    Anywhere (v6)             
6443 (v6)                  ALLOW IN    Anywhere (v6)             
80 (v6)                    ALLOW IN    Anywhere (v6)             
443 (v6)                   ALLOW IN    Anywhere (v6)  

fixes #36

Signed-off-by: Waleed Gadelkareem <gadelkareem@gmail.com>
Signed-off-by: Waleed Gadelkareem <gadelkareem@gmail.com>
@gadelkareem gadelkareem mentioned this pull request Nov 19, 2018
" && ufw --force enable")
FatalOnError(err)

output, err = addon.communicator.RunCmd(node, "ufw status verbose")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Output is overridden on each iteration, maybe we should print the output (moving line 79 into foreach) or collect it here and then print?

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 am not sure if we want to print the output for each server, especially when it is fairly the same output. Let me know your thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually we are going to execute a command than report the result of the command execution to user as "changes applied to all nodes".

IMHO this is not correct because (as reported above) we can have different rules based on node type, and should be "correct" to explain the change we add to each node or just report the info on expected reported situation to the user (that do not involve UFW execution report on a single node but the rule we are going to apply).

Let's suppose the last node of the iteration already have some rule define (maybe manually?) that are not configured on all nodes, a user can suppose that the result (already existing rule + rule from addon) are configured on all nodes but this is not true.

Maybe is an edge case and we prefer to ignore this situation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ideal situation would be to allow the user to override the rules or append new rules to the basic ones. You would also have to distinguish each rule based on the instance type. Which would make this addon much more complex than it is meant to be IMHO.

nodeIpRules += " && ufw allow in from " + node.IPAddress + " to any"
}
var output string
for _, node := range addon.nodes {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We apply the same rule for each node, but I think each kind of node should have some specific rules, eg: etcd/master node don't need to expose 80/443 (that I suppose should be exposed on workers); maybe I'm missing some part of the setup can you better explain me what we like to achieve with this addon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I think the ufw rules are the trickiest part of this addon. I tried to use the most common rules any new cluster would require. But you can always change these rules via script runner from #229 for ex.

pkg/addons/addon_ufw.go Outdated Show resolved Hide resolved
pkg/addons/addon_ufw.go Outdated Show resolved Hide resolved
@mavimo mavimo added the enhancement New feature or request label Nov 20, 2018
@mavimo mavimo added this to In progress in 1.0 via automation Nov 20, 2018
Signed-off-by: Waleed Gadelkareem <gadelkareem@gmail.com>
Signed-off-by: Waleed Gadelkareem <gadelkareem@gmail.com>
@xetys
Copy link
Owner

xetys commented Mar 22, 2019

whats the state here?

@willipreuk
Copy link

Any updates?

@tckb
Copy link

tckb commented Oct 30, 2019

This looks pretty important. Whats the state of this PR? @gadelkareem

@xetys
Copy link
Owner

xetys commented Nov 12, 2019

push @mavimo @gadelkareem

@mavimo
Copy link
Collaborator

mavimo commented Nov 13, 2019

@xetys I'll take a look in the next few days..

@liebig
Copy link

liebig commented Dec 4, 2019

Any news on this?

@alexanderkjeldaas
Copy link

Any update? This was referred to by https://www.tmjohnson.co.uk/posts/k8s/

@wethinkagile
Copy link

Any update here? We need this 2 years ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
1.0
  
In progress
Development

Successfully merging this pull request may close these issues.

Firewall security
8 participants