Merge lp:~gz/juju-core/trunk-security-group-group-id-1226996 into lp:~go-bot/juju-core/trunk

[Martin Packman]

provider/openstack: bug #1226996 SecurityGroup

We intended to allow access to any port for any instance in the
default security group. However, we didn't specify a CIDR and we
didn't reference the Source Group Id. Which meant we actually were
exposing *all* ports to *all* machines.

This is a cherrypick from 1.14 rather than a merge up of all
changes, as the two branches are surprisingly diverged.

https://codereview.appspot.com/13787043/

R=rogpeppe

provider/openstack: bug #1226996 SecurityGroup

We intended to allow access to any port for any instance in the
default security group. However, we didn't specify a CIDR and we
didn't reference the Source Group Id. Which meant we actually were
exposing *all* ports to *all* machines.

This is a cherrypick from 1.14 rather than a merge up of all
changes, as the two branches are surprisingly diverged.

https://codereview.appspot.com/13787043/

[Martin Packman (gz)]

Reviewers: mp+186586_code.launchpad.net,

Message:
Please take a look.

Description:
provider/openstack: bug #1226996 SecurityGroup

We intended to allow access to any port for any instance in the
default security group. However, we didn't specify a CIDR and we
didn't reference the Source Group Id. Which meant we actually were
exposing *all* ports to *all* machines.

This is a cherrypick from 1.14 rather than a merge up of all
changes, as the two branches are surprisingly diverged.

https://code.launchpad.net/~gz/juju-core/trunk-security-group-group-id-1226996/+merge/186586

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/13787043/

Affected files (+144, -9 lines):
   A [revision details]
   M environs/jujutest/livetests.go
   M provider/openstack/export_test.go
   M provider/openstack/live_test.go
   M provider/openstack/provider.go

[Roger Peppe (rogpeppe)]

On 2013/09/19 16:59:34, gz wrote:
> Please take a look.

LGTM assuming you've tested it live and the expose logic works
as expected.

https://codereview.appspot.com/13787043/

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

>
> This is a cherrypick from 1.14 rather than a merge up of all
> changes, as the two branches are surprisingly diverged.
>

I would really like to see us get convergence here, but doing this
short term is ok. (It is only going to get harder in the future, and
make it more difficult to make bugfixes on stable branches.)

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlI8RIoACgkQJdeBCYSNAAPveQCcDTaoVOKLQywXybv+ftDKOGsW
o/UAnAzo8/ulGMxAe0oJXdHLH7oKU++n
=A+Sx
-----END PGP SIGNATURE-----

=== modified file 'environs/jujutest/livetests.go'
--- environs/jujutest/livetests.go	2013-09-18 22:54:32 +0000
+++ environs/jujutest/livetests.go	2013-09-19 17:18:25 +0000
@@ -514,6 +514,10 @@
 	conn, err := juju.NewConn(t.Env)
 	c.Assert(err, gc.IsNil)
 	conn.Close()
+
+	apiConn, err := juju.NewAPIConn(t.Env, api.DefaultDialOpts())
+	c.Assert(err, gc.IsNil)
+	apiConn.Close()
 }
 
 func (t *LiveTests) TestCheckEnvironmentOnConnectNoVerificationFile(c *gc.C) {

=== modified file 'provider/openstack/export_test.go'
--- provider/openstack/export_test.go	2013-09-18 07:13:09 +0000
+++ provider/openstack/export_test.go	2013-09-19 17:18:25 +0000
@@ -9,6 +9,7 @@
 	"strings"
 	"text/template"
 
+	"launchpad.net/goose/errors"
 	"launchpad.net/goose/identity"
 	"launchpad.net/goose/nova"
 	"launchpad.net/goose/swift"
@@ -236,6 +237,25 @@
 	WritablePublicStorage(e).Remove(productMetadatafile)
 }
 
+// DiscardSecurityGroup cleans up a security group, it is not an error to
+// delete something that doesn't exist.
+func DiscardSecurityGroup(e environs.Environ, name string) error {
+	env := e.(*environ)
+	novaClient := env.nova()
+	group, err := novaClient.SecurityGroupByName(name)
+	if err != nil {
+		if errors.IsNotFound(err) {
+			// Group already deleted, done
+			return nil
+		}
+	}
+	err = novaClient.DeleteSecurityGroup(group.Id)
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
 func FindInstanceSpec(e environs.Environ, series, arch, cons string) (spec *instances.InstanceSpec, err error) {
 	env := e.(*environ)
 	spec, err = findInstanceSpec(env, &instances.InstanceConstraint{
@@ -261,6 +281,10 @@
 	env.ecfg().attrs["use-floating-ip"] = val
 }
 
+func SetUpGlobalGroup(e environs.Environ, name string, statePort, apiPort int) (nova.SecurityGroup, error) {
+	return e.(*environ).setUpGlobalGroup(name, statePort, apiPort)
+}
+
 func EnsureGroup(e environs.Environ, name string, rules []nova.RuleInfo) (nova.SecurityGroup, error) {
 	return e.(*environ).ensureGroup(name, rules)
 }

=== modified file 'provider/openstack/live_test.go'
--- provider/openstack/live_test.go	2013-09-18 07:13:09 +0000
+++ provider/openstack/live_test.go	2013-09-19 17:18:25 +0000
@@ -7,11 +7,13 @@
 	"crypto/rand"
 	"fmt"
 	"io"
+	"sort"
 
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/goose/client"
 	"launchpad.net/goose/identity"
+	"launchpad.net/goose/nova"
 
 	"launchpad.net/juju-core/environs/jujutest"
 	"launchpad.net/juju-core/environs/storage"
@@ -126,3 +128,85 @@
 	t.LiveTests.TearDownTest(c)
 	t.LoggingSuite.TearDownTest(c)
 }
+
+func (t *LiveTests) TestEnsureGroupSetsGroupId(c *gc.C) {
+	rules := []nova.RuleInfo{
+		{ // First group explicitly asks for all services
+			IPProtocol: "tcp",
+			FromPort:   22,
+			ToPort:     22,
+			Cidr:       "0.0.0.0/0",
+		},
+		{ // Second group should only allow access from within the group
+			IPProtocol: "tcp",
+			FromPort:   1,
+			ToPort:     65535,
+		},
+	}
+	groupName := "juju-test-group-" + randomName()
+	// Make sure things are clean before we start, and clean when we are done
+	cleanup := func() {
+		c.Check(openstack.DiscardSecurityGroup(t.Env, groupName), gc.IsNil)
+	}
+	cleanup()
+	defer cleanup()
+	group, err := openstack.EnsureGroup(t.Env, groupName, rules)
+	c.Assert(err, gc.IsNil)
+	c.Check(group.Rules, gc.HasLen, 2)
+	c.Check(*group.Rules[0].IPProtocol, gc.Equals, "tcp")
+	c.Check(*group.Rules[0].FromPort, gc.Equals, 22)
+	c.Check(*group.Rules[0].ToPort, gc.Equals, 22)
+	c.Check(group.Rules[0].IPRange["cidr"], gc.Equals, "0.0.0.0/0")
+	c.Check(group.Rules[0].Group.Name, gc.Equals, "")
+	c.Check(group.Rules[0].Group.TenantId, gc.Equals, "")
+	c.Check(*group.Rules[1].IPProtocol, gc.Equals, "tcp")
+	c.Check(*group.Rules[1].FromPort, gc.Equals, 1)
+	c.Check(*group.Rules[1].ToPort, gc.Equals, 65535)
+	c.Check(group.Rules[1].IPRange, gc.HasLen, 0)
+	c.Check(group.Rules[1].Group.Name, gc.Equals, groupName)
+	c.Check(group.Rules[1].Group.TenantId, gc.Equals, group.TenantId)
+}
+
+func (t *LiveTests) TestSetupGlobalGroupExposesCorrectPorts(c *gc.C) {
+	groupName := "juju-test-group-" + randomName()
+	// Make sure things are clean before we start, and will be clean when we finish
+	cleanup := func() {
+		c.Check(openstack.DiscardSecurityGroup(t.Env, groupName), gc.IsNil)
+	}
+	cleanup()
+	defer cleanup()
+	statePort := 12345 // Default 37017
+	apiPort := 34567   // Default 17070
+	group, err := openstack.SetUpGlobalGroup(t.Env, groupName, statePort, apiPort)
+	c.Assert(err, gc.IsNil)
+	c.Assert(err, gc.IsNil)
+	// We default to exporting 22, statePort, apiPort, and icmp/udp/tcp on
+	// all ports to other machines inside the same group
+	// TODO(jam): 2013-09-18 http://pad.lv/1227142
+	// We shouldn't be exposing the API and State ports on all the machines
+	// that *aren't* hosting the state server. (And once we finish
+	// client-via-API we can disable the State port as well.)
+	stringRules := make([]string, 0, len(group.Rules))
+	for _, rule := range group.Rules {
+		ruleStr := fmt.Sprintf("%s %d %d %q %q",
+			*rule.IPProtocol,
+			*rule.FromPort,
+			*rule.ToPort,
+			rule.IPRange["cidr"],
+			rule.Group.Name,
+		)
+		stringRules = append(stringRules, ruleStr)
+	}
+	// We don't care about the ordering, so we sort the result, and compare it.
+	expectedRules := []string{
+		`tcp 22 22 "0.0.0.0/0" ""`,
+		fmt.Sprintf(`tcp %d %d "0.0.0.0/0" ""`, statePort, statePort),
+		fmt.Sprintf(`tcp %d %d "0.0.0.0/0" ""`, apiPort, apiPort),
+		fmt.Sprintf(`tcp 1 65535 "" "%s"`, groupName),
+		fmt.Sprintf(`udp 1 65535 "" "%s"`, groupName),
+		fmt.Sprintf(`icmp -1 -1 "" "%s"`, groupName),
+	}
+	sort.Strings(stringRules)
+	sort.Strings(expectedRules)
+	c.Check(stringRules, gc.DeepEquals, expectedRules)
+}

=== modified file 'provider/openstack/provider.go'
--- provider/openstack/provider.go	2013-09-18 07:13:09 +0000
+++ provider/openstack/provider.go	2013-09-19 17:18:25 +0000
@@ -991,15 +991,8 @@
 	return &providerInstance
 }
 
-// setUpGroups creates the security groups for the new machine, and
-// returns them.
-//
-// Instances are tagged with a group so they can be distinguished from
-// other instances that might be running on the same OpenStack account.
-// In addition, a specific machine security group is created for each
-// machine, so that its firewall rules can be configured per machine.
-func (e *environ) setUpGroups(machineId string, statePort, apiPort int) ([]nova.SecurityGroup, error) {
-	jujuGroup, err := e.ensureGroup(e.jujuGroupName(),
+func (e *environ) setUpGlobalGroup(groupName string, statePort, apiPort int) (nova.SecurityGroup, error) {
+	return e.ensureGroup(groupName,
 		[]nova.RuleInfo{
 			{
 				IPProtocol: "tcp",
@@ -1015,6 +1008,12 @@
 			},
 			{
 				IPProtocol: "tcp",
+				FromPort:   apiPort,
+				ToPort:     apiPort,
+				Cidr:       "0.0.0.0/0",
+			},
+			{
+				IPProtocol: "tcp",
 				FromPort:   1,
 				ToPort:     65535,
 			},
@@ -1029,6 +1028,21 @@
 				ToPort:     -1,
 			},
 		})
+}
+
+// setUpGroups creates the security groups for the new machine, and
+// returns them.
+//
+// Instances are tagged with a group so they can be distinguished from
+// other instances that might be running on the same OpenStack account.
+// In addition, a specific machine security group is created for each
+// machine, so that its firewall rules can be configured per machine.
+//
+// Note: ideally we'd have a better way to determine group membership so that 2
+// people that happen to share an openstack account and name their environment
+// "openstack" don't end up destroying each other's machines.
+func (e *environ) setUpGroups(machineId string, statePort, apiPort int) ([]nova.SecurityGroup, error) {
+	jujuGroup, err := e.setUpGlobalGroup(e.jujuGroupName(), statePort, apiPort)
 	if err != nil {
 		return nil, err
 	}
@@ -1077,6 +1091,13 @@
 	group.Rules = make([]nova.SecurityGroupRule, len(rules))
 	for i, rule := range rules {
 		rule.ParentGroupId = group.Id
+		if rule.Cidr == "" {
+			// http://pad.lv/1226996 Rules that don't have a CIDR
+			// are meant to apply only to this group. If you don't
+			// supply CIDR or GroupId then openstack assumes you
+			// mean CIDR=0.0.0.0/0
+			rule.GroupId = &group.Id
+		}
 		groupRule, err := novaClient.CreateSecurityGroupRule(rule)
 		if err != nil && !gooseerrors.IsDuplicateValue(err) {
 			return zeroGroup, err