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.

Sign up for GitHub

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

Allow DNS port when performing iptables filtering on cloud provider metadata IP #12

Merged
merged 1 commit into from
Jul 25, 2019
Merged

Allow DNS port when performing iptables filtering on cloud provider metadata IP #12

merged 1 commit into from
Jul 25, 2019

Conversation

alexanderConstantinescu
Copy link
Contributor

This addresses the bug described here

We want to allow DNS packets to be routed through the metadata IP as this is sometimes used by internal mechanism to these providers when performing certain networking tasks.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 23, 2019
@alexanderConstantinescu
Copy link
Contributor Author

/assign @squeed

squeed
squeed reviewed Jul 23, 2019
View reviewed changes
cmd/sdn-cni-plugin/openshift-sdn_linux.go Outdated
{"-A", "OUTPUT", "-d", "169.254.169.254", "-j", "REJECT"},
{"-A", "FORWARD", "-d", "169.254.169.254", "-j", "REJECT"},
// Block cloud metadata IP on all ports except 53 for tcp/udp
{"-p", "tcp", "-A", "OUTPUT", "-d", "169.254.169.254", "!", "--dport", "53", "-j", "REJECT"},
Copy link
Contributor

Choose a reason for hiding this comment

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

It's convention to put the "-A OUTPUT" first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, just updated the PR

@squeed
Copy link
Contributor

squeed commented Jul 23, 2019

cc @danwinship

@danwinship
Copy link
Contributor

I think maybe it would be better to only block port 80/443 rather than to block everything except 53.

Also, we're blocking it in two places: see pkg/network/node/ovscontroller.go as well.

However, do we actually know that the metadata service on GCP contains privileged information that we need to prevent pods from accessing? Maybe the fix is "only block on AWS", not "only block http".

@squeed
Copy link
Contributor

squeed commented Jul 24, 2019 •
edited
Loading

I think maybe it would be better to only block port 80/443 rather than to block everything except 53.

I would prefer whitelisting over blacklisting.

However, do we actually know that the metadata service on GCP contains privileged information that we need to prevent pods from accessing? Maybe the fix is "only block on AWS", not "only block http".

The GKE documentation seems to imply that, yes, the cluster metadata endpoint can contain sensitive information.

@alexanderConstantinescu
Copy link
Contributor Author

I agree with @squeed concerning the IPs to block.

I will update the PR with modifications to the additional file you mentioned @danwinship

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 24, 2019
@alexanderConstantinescu
Copy link
Contributor Author

Just updated the PR to drop packets to 443 and 80 only.

Please have a look and let me know

@alexanderConstantinescu
Copy link
Contributor Author

/assign @danwinship

danwinship
danwinship suggested changes Jul 24, 2019
View reviewed changes
Copy link
Contributor

@danwinship danwinship left a comment

Choose a reason for hiding this comment

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

We should probably
/hold
this until we've confirmed the situation on Azure...

cmd/sdn-cni-plugin/openshift-sdn_linux.go Outdated
{"-A", "OUTPUT", "-p", "tcp", "-m", "tcp", "-d", "169.254.169.254", "--dport", "80", "-j", "REJECT"},
{"-A", "OUTPUT", "-p", "udp", "-m", "udp", "-d", "169.254.169.254", "--dport", "80", "-j", "REJECT"},
{"-A", "FORWARD", "-p", "tcp", "-m", "tcp", "-d", "169.254.169.254", "--dport", "443", "-j", "REJECT"},
{"-A", "FORWARD", "-p", "udp", "-m", "udp", "-d", "169.254.169.254", "--dport", "443", "-j", "REJECT"},
Copy link
Contributor

Choose a reason for hiding this comment

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

This got messed up in one of the rewrites... now it's blocking port 80 in OUTPUT and port 443 in FORWARD.

Also, we don't need to block UDP, just TCP. Also, you can simplify using multiport. So (I think, not tested):

{"-A", "OUTPUT", "-d", "169.254.169.254", "-p", "tcp", "-m", "multiport", "--destination-ports", "80,443", "-j", "REJECT"},
{"-A", "FORWARD", "-d", "169.254.169.254", "-p", "tcp", "-m", "multiport", "--destination-ports", "80,443", "-j", "REJECT"},

but also, @squeed, what's the verdict on 8080?

Copy link
Contributor

Choose a reason for hiding this comment

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

Waiting for the west coast to have coffee, then we'll see.

pkg/network/node/ovscontroller.go Outdated
otx.AddFlow("table=30, priority=75, ip, tcp, tcp_dst=80, nw_dst=169.254.0.0/16, actions=drop")
otx.AddFlow("table=30, priority=75, ip, tcp, tcp_dst=443, nw_dst=169.254.0.0/16, actions=drop")
otx.AddFlow("table=30, priority=75, ip, udp, udp_dst=80, nw_dst=169.254.0.0/16, actions=drop")
otx.AddFlow("table=30, priority=75, ip, udp, udp_dst=443, nw_dst=169.254.0.0/16, actions=drop")
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need UDP.
And also, at this point, if we're giving up on the idea of blocking all link-local traffic out of the pod network then this should match nw_dst=169.254.169.254, not nw_dst=169.254.0.0/16

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, flows are ordered, right? So can we allow packets with a port (src or dst) of 53, and drop all others?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, you can't just generically "allow"; you have to say specifically what to do with the packet next (in this case, goto_table:100). But... then you'd have two sets of goto_table:100 rules with a drop rule between them and that kind of violates the code style? Handwave handwave.

Yes, we could do it.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 24, 2019
@squeed
Copy link
Contributor

squeed commented Jul 25, 2019

To unblock GCP, I've asked @alexanderConstantinescu to add the 53 whitelist to iptables and drop the openflow rule completely for now. I'll file a blocker bug to re-add the openflow rules.

@alexanderConstantinescu alexanderConstantinescu force-pushed the bugfix/SDN-493 branch 2 times, most recently from 92d720d to e73754e Compare July 25, 2019 08:51
squeed
squeed reviewed Jul 25, 2019
View reviewed changes
cmd/sdn-cni-plugin/openshift-sdn_linux.go Outdated
// Block cloud metadata IP
{"-A", "OUTPUT", "-d", "169.254.169.254", "-j", "REJECT"},
{"-A", "FORWARD", "-d", "169.254.169.254", "-j", "REJECT"},
// Block cloud metadata IP on ports 80 & 443 for TCP & UDP
Copy link
Contributor

Choose a reason for hiding this comment

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

"Block cloud provider metadata IP except DNS"

@squeed
Copy link
Contributor

squeed commented Jul 25, 2019

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexanderConstantinescu, squeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2019
@squeed
Copy link
Contributor

squeed commented Jul 25, 2019

@danwinship, would you be OK with removing the hold for now?

@danwinship
Copy link
Contributor

/hold cancel
sure but make sure the blocker bug involves figuring out azure too

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 25, 2019
@squeed
Copy link
Contributor

squeed commented Jul 25, 2019

/retest

@openshift-merge-robot openshift-merge-robot merged commit b3baffe into openshift:master Jul 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danwinship danwinship danwinship requested changes

@squeed squeed squeed left review comments

@JacobTanenbaum JacobTanenbaum Awaiting requested review from JacobTanenbaum

@pecameron pecameron Awaiting requested review from pecameron

Assignees

@danwinship danwinship

@squeed squeed

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alexanderConstantinescu @squeed @danwinship @openshift-ci-robot @openshift-merge-robot