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

Explicitly whitelist all traffic from concourse containers #5159

Merged
merged 1 commit into from
Feb 11, 2020
Merged

Explicitly whitelist all traffic from concourse containers #5159

merged 1 commit into from
Feb 11, 2020

Conversation

aemengo
Copy link
Contributor

@aemengo aemengo commented Feb 10, 2020

  • This declaration is necessary for platforms where this is not
    the standard networking behavior (like windows).

Existing Issue

Fixes #5028 .

Changes proposed in this pull request

  • Explicitly whitelist all traffic from concourse containers. This is already done implicitly for the linux platform, but we need it to explicit for behavior parity with the windows platform.

Contributor Checklist

  • Unit tests

Reviewer Checklist

This section is intended for the core maintainers only, to track review progress. Please do not
fill out this section.

  • Code reviewed
  • Tests reviewed
  • Documentation reviewed
  • Release notes reviewed
  • PR acceptance performed
  • New config flags added? Ensure that they are added to the BOSH
    and Helm packaging; otherwise, ignored for the integration tests (for example, if they are Garden configs that are not displayed in the --help text).

@jfeeny @genevieve

@aemengo aemengo requested a review from a team February 10, 2020 19:40
* This declaration is necessary for platforms where this is not
  the standard networking behavior (like windows).

[#171155863] (OSS Concourse w/ Garden Windows backend)

Signed-off-by: Anthony Emengo <aemengo@pivotal.io>
@cirocosta cirocosta added this to In progress in Runtime side-road via automation Feb 10, 2020
@cirocosta cirocosta moved this from In progress to Review in progress in Runtime side-road Feb 10, 2020
Runtime side-road automation moved this from Review in progress to Reviewer approved Feb 11, 2020
Copy link
Member

@cirocosta cirocosta left a comment

Choose a reason for hiding this comment

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

LGTM 😁 thanks!!


I was curious about the differences between the iptables rules that gdn would generate before and after this change (as gdn itself does not "fill" the spec with a default NetRule):

(BEFORE)

	Chain w--instance-container-chain (1 references)
	target     prot opt source               destination
	RETURN     all  --  0.0.0.0/0            0.0.0.0/0            /* container */
	ACCEPT     all  --  10.254.0.4/30        10.254.0.4/30        /* container */
	w--default  all  --  0.0.0.0/0            0.0.0.0/0           [goto]  /* container */

(AFTER)

	Chain w--instance-container-chain(1 references)
	target     prot opt source               destination
	ACCEPT     all  --  10.254.0.0/30        10.254.0.0/30        /* container */
	w--default  all  --  0.0.0.0/0            0.0.0.0/0           [goto]  /* container */

while it might seem that we're now being too open by adding that "allow any"
there, it's actually fine, as the chain that should get to this (the reference),
would never really get to it:

	Chain w--forward (1 references)
	target                          prot opt source               destination
	ACCEPT                          all  --  0.0.0.0/0            0.0.0.0/0
	w--instance-sj7jnm74l16         all  --  10.254.0.6           0.0.0.0/0           [goto]  /* container */
	DROP                            all  --  0.0.0.0/0            0.0.0.0/0

as it'd accept anything anyway.

also,

for those curious where this is handled in gdn:

  • during container creation (garden.Create()), gardener calls out to someone who implements the networker interface
// Create creates a container by combining the results of networker.Network,
// volumizer.Create and containzer.Create.
func (g *Gardener) Create(containerSpec garden.ContainerSpec) (ctr garden.Container, err error) {
         // ...
	if err = g.Networker.Network(log, containerSpec, actualSpec.Pid); err != nil {
		return nil, err
	}

https://github.com/cloudfoundry/guardian/blob/bcca70921740300398648046496790876981b158/gardener/gardener.go#L301-L303

  • kawasaki ("the great networker" 😅 ) implements that interface, and it leverages the FirewallOpener contributor, which takes care of exec'in iptables, creating the rules as desired
func (f *FirewallOpener) BulkOpen(logger lager.Logger, instance, handle string, rules []garden.NetOutRule) error {
	chain := f.iptables.InstanceChain(instance)
	logger = logger.Session("prepend-filter-rule", lager.Data{
		"rules":    rules,
		"instance": instance,
		"chain":    chain,
	})
	logger.Debug("started")
	defer logger.Debug("ending")


	collatedIPTablesRules := []Rule{}
	for _, rule := range rules {
		iptablesRules, err := f.ruleTranslator.TranslateRule(handle, rule)
		if err != nil {
			return err
		}

		collatedIPTablesRules = append(collatedIPTablesRules, iptablesRules...)
	}

	return f.iptables.BulkPrependRules(chain, collatedIPTablesRules)
}

https://github.com/cloudfoundry/guardian/blob/bcca70921740300398648046496790876981b158/kawasaki/iptables/firewall_openner.go#L49-L70

@cirocosta cirocosta merged commit a2444ed into concourse:master Feb 11, 2020
Runtime side-road automation moved this from Reviewer approved to Done Feb 11, 2020
@clarafu clarafu added the release/documented Documentation and release notes have been updated. label Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/documented Documentation and release notes have been updated.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Set NetOutRule to allow outbound connections when creating containers
4 participants