Merge pull request #3449 from alphagov/ris-s3-broker-get-object-taggi…

…ng

paas-s3-broker: bump to ..., provide with common user policy, expand custom-broker-acceptance-tests

manifests/cf-manifest/operations.d/750-s3-broker.yml

@@ -4,9 +4,9 @@
   path: /releases/-
   value:
     name: s3-broker
-    version: 0.1.21 
-    url: https://s3-eu-west-1.amazonaws.com/gds-paas-build-releases/s3-broker-0.1.21.tgz
-    sha1: 31818e98cd5b420a64c9c695f866a97d7d8497f7
+    version: 0.1.25
+    url: https://s3-eu-west-1.amazonaws.com/gds-paas-build-releases/s3-broker-0.1.25.tgz
+    sha1: 3a9f71131b7f37c8b0c74d795e652823c8de13df
 
 - type: replace
   path: /addons/name=loggregator_agent/exclude/jobs/-
@@ -39,6 +39,8 @@
             resource_prefix: "paas-s3-broker-((environment))-"
             iam_user_path: "/paas-s3-broker/"
             iam_ip_restriction_policy_arn: "((terraform_outputs_s3_broker_ip_restriction_policy_arn))"
+            iam_common_user_policy_arn: "((terraform_outputs_s3_broker_common_user_policy_arn))"
+            iam_user_permissions_boundary_arn: "((terraform_outputs_s3_broker_user_permissions_boundary_arn))"
             deploy_environment: "((environment))"
             tls: ((secrets_s3_broker_tls_cert))
             locket:

platform-tests/broker-acceptance/s3_broker_test.go

@@ -1,9 +1,19 @@
 package broker_acceptance_test
 
 import (
+	"bytes"
+	"encoding/json"
+	"fmt"
 	"io/ioutil"
+	"strings"
 	"time"
 
+	"github.com/aws/aws-sdk-go/aws/credentials"
+
+	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/session"
+	"github.com/aws/aws-sdk-go/service/s3"
+
 	"github.com/cloudfoundry/cf-test-helpers/cf"
 	"github.com/cloudfoundry/cf-test-helpers/generator"
 	"github.com/cloudfoundry/cf-test-helpers/helpers"
@@ -19,6 +29,16 @@ var _ = Describe("S3 broker", func() {
 		testPlanName = "default"
 	)
 
+	type ServiceKeyCredentials struct {
+		BucketName         string `json:"bucket_name"`
+		AWSAccessKeyID     string `json:"aws_access_key_id"`
+		AWSSecretAccessKey string `json:"aws_secret_access_key"`
+		AWSRegion          string `json:"aws_region"`
+	}
+	type ServiceKeyData struct {
+		Credentials ServiceKeyCredentials `json:"credentials"`
+	}
+
 	It("is registered in the marketplace", func() {
 		plans := cf.Cf("marketplace").Wait(testConfig.DefaultTimeoutDuration())
 		Expect(plans).To(Exit(0))
@@ -33,24 +53,31 @@ var _ = Describe("S3 broker", func() {
 
 	Context("creating an S3 bucket", func() {
 		var (
-			appName             string
 			serviceInstanceName string
 		)
 
-		It("is accessible from the healthcheck app", func() {
+		createBucketWithCleanup := func(siName string) {
+			By("creating the service: "+siName, func() {
+				Expect(cf.Cf("create-service", serviceName, testPlanName, siName).Wait(testConfig.DefaultTimeoutDuration())).To(Exit(0))
+				pollForServiceCreationCompletion(siName)
+			})
 
-			appName = generator.PrefixedRandomName(testConfig.GetNamePrefix(), "APP")
+			DeferCleanup(func() {
+				By("deleting the service", func() {
+					Expect(cf.Cf("delete-service", siName, "-f").Wait(testConfig.DefaultTimeoutDuration())).To(Exit(0))
+					pollForServiceDeletionCompletion(siName)
+				})
+			})
+		}
+
+		BeforeEach(func () {
 			serviceInstanceName = generator.PrefixedRandomName(testConfig.GetNamePrefix(), "test-s3-bucket")
 
-			By("creating the service: "+serviceInstanceName, func() {
-				Expect(cf.Cf("create-service", serviceName, testPlanName, serviceInstanceName).Wait(testConfig.DefaultTimeoutDuration())).To(Exit(0))
-				pollForServiceCreationCompletion(serviceInstanceName)
-			})
+			createBucketWithCleanup(serviceInstanceName)
+		})
 
-			defer By("deleting the service", func() {
-				Expect(cf.Cf("delete-service", serviceInstanceName, "-f").Wait(testConfig.DefaultTimeoutDuration())).To(Exit(0))
-				pollForServiceDeletionCompletion(serviceInstanceName)
-			})
+		It("is accessible from the healthcheck app", func() {
+			appName := generator.PrefixedRandomName(testConfig.GetNamePrefix(), "APP")
 
 			By("pushing the healthcheck app", func() {
 				Expect(cf.Cf(
@@ -82,7 +109,213 @@ var _ = Describe("S3 broker", func() {
 				resp.Body.Close()
 				Expect(resp.StatusCode).To(Equal(200), "Got %d response from healthcheck app. Response body:\n%s\n", resp.StatusCode, string(body))
 			})
+		})
+
+		Context("external access", func() {
+			var s3Client *s3.S3
+			var bucketName string
+			var serviceKeyName string
+
+			createServiceKeyWithCleanup := func(
+				siName string,
+				skName string,
+				allowExternalAccessJSON string,
+			) {
+				By("creating the service key: "+skName, func() {
+					Expect(cf.Cf(
+						"create-service-key",
+						siName,
+						skName,
+						"-c",
+						fmt.Sprintf(
+							`{"allow_external_access": %s, "permissions": "read-write"}`,
+							allowExternalAccessJSON,
+						),
+					).Wait(testConfig.DefaultTimeoutDuration())).To(Exit(0))
+				})
+
+				DeferCleanup(func() {
+					By("deleting the service key", func() {
+						cf.Cf("delete-service-key", siName, skName, "-f").Wait(testConfig.DefaultTimeoutDuration())
+					})
+				})
+			}
+
+			s3ClientFromServiceKey := func(siName string, skName string) (*s3.S3, string) {
+				var serviceKeyData ServiceKeyData
+				cfSess := cf.Cf(
+					"service-key",
+					siName,
+					skName,
+				)
+				Expect(cfSess.Wait(testConfig.DefaultTimeoutDuration())).To(Exit(0))
+
+				outContents := cfSess.Out.Contents()
+				err := json.Unmarshal(
+					outContents[bytes.IndexByte(outContents, byte('{')):],
+					&serviceKeyData,
+				)
+				Expect(err).ToNot(HaveOccurred())
+
+				Expect(serviceKeyData.Credentials.BucketName).ToNot(Equal(""))
+				Expect(serviceKeyData.Credentials.AWSAccessKeyID).ToNot(Equal(""))
+				Expect(serviceKeyData.Credentials.AWSSecretAccessKey).ToNot(Equal(""))
+				Expect(serviceKeyData.Credentials.AWSRegion).ToNot(Equal(""))
+
+				sess := session.Must(session.NewSession(&aws.Config{
+					Region:      aws.String(serviceKeyData.Credentials.AWSRegion),
+					Credentials: credentials.NewStaticCredentials(
+						serviceKeyData.Credentials.AWSAccessKeyID,
+						serviceKeyData.Credentials.AWSSecretAccessKey,
+						"",
+					),
+				}))
+				return s3.New(sess), serviceKeyData.Credentials.BucketName
+			}
+
+			assertNoBucketAccess := func(bName string) {
+				_, err := s3Client.PutObject(&s3.PutObjectInput{
+					Bucket: aws.String(bName),
+					Key:    aws.String("test-key"),
+					Body:   strings.NewReader("test-content"),
+				})
+				Expect(err).To(MatchError(ContainSubstring("AccessDenied")))
+
+				_, err = s3Client.ListObjects(&s3.ListObjectsInput{
+					Bucket: aws.String(bName),
+				})
+				Expect(err).To(MatchError(ContainSubstring("AccessDenied")))
+
+				_, err = s3Client.GetObject(&s3.GetObjectInput{
+					Bucket: aws.String(bName),
+					Key:    aws.String("test-key"),
+				})
+				Expect(err).To(MatchError(ContainSubstring("AccessDenied")))
+
+				_, err = s3Client.GetObjectTagging(&s3.GetObjectTaggingInput{
+					Bucket: aws.String(bName),
+					Key:    aws.String("test-key"),
+				})
+				Expect(err).To(MatchError(ContainSubstring("AccessDenied")))
+
+				_, err = s3Client.DeleteObject(&s3.DeleteObjectInput{
+					Bucket: aws.String(bName),
+					Key:    aws.String("test-key"),
+				})
+				Expect(err).To(MatchError(ContainSubstring("AccessDenied")))
+			}
+
+			BeforeEach(func() {
+				serviceKeyName = generator.PrefixedRandomName(testConfig.GetNamePrefix(), "test-service-key")
+			})
 
+			Context("with a non-allow_external_access service key", func() {
+				BeforeEach(func() {
+					createServiceKeyWithCleanup(
+						serviceInstanceName,
+						serviceKeyName,
+						`false`,
+					)
+				})
+
+				It("is not externally accessible", func() {
+					By("retrieving the service key credentials", func() {
+						s3Client, bucketName = s3ClientFromServiceKey(
+							serviceInstanceName,
+							serviceKeyName,
+						)
+					})
+
+					By("failing to access the bucket from the test host", func() {
+						assertNoBucketAccess(bucketName)
+					})
+				})
+			})
+
+			Context("with an allow_external_access service key", func() {
+				BeforeEach(func() {
+					createServiceKeyWithCleanup(
+						serviceInstanceName,
+						serviceKeyName,
+						`true`,
+					)
+					s3Client, bucketName = s3ClientFromServiceKey(
+						serviceInstanceName,
+						serviceKeyName,
+					)
+				})
+
+				It("is externally accessible", func() {
+					By("accessing the bucket from the test host", func() {
+						_, err := s3Client.PutObject(&s3.PutObjectInput{
+							Bucket: aws.String(bucketName),
+							Key:    aws.String("test-key"),
+							Body:   strings.NewReader("test-content"),
+						})
+						Expect(err).ToNot(HaveOccurred())
+
+						defer func() {
+							_, err := s3Client.DeleteObject(&s3.DeleteObjectInput{
+								Bucket: aws.String(bucketName),
+								Key:    aws.String("test-key"),
+							})
+							Expect(err).ToNot(HaveOccurred())
+						}()
+
+						listObjectsResult, err := s3Client.ListObjects(&s3.ListObjectsInput{
+							Bucket: aws.String(bucketName),
+						})
+						Expect(err).ToNot(HaveOccurred())
+						Expect(len(listObjectsResult.Contents)).To(Equal(1))
+						Expect(listObjectsResult.Contents[0].Key).To(Equal(aws.String("test-key")))
+
+						_, err = s3Client.GetObject(&s3.GetObjectInput{
+							Bucket: aws.String(bucketName),
+							Key:    aws.String("test-key"),
+						})
+						Expect(err).ToNot(HaveOccurred())
+
+						_, err = s3Client.GetObjectTagging(&s3.GetObjectTaggingInput{
+							Bucket: aws.String(bucketName),
+							Key:    aws.String("test-key"),
+						})
+						Expect(err).ToNot(HaveOccurred())
+					})
+				})
+
+				Context("and a second s3 service instance", func() {
+					var serviceInstance2Name string
+					var serviceKey2Name string
+
+					BeforeEach(func() {
+						serviceInstance2Name = generator.PrefixedRandomName(testConfig.GetNamePrefix(), "test-s3-bucket-2")
+
+						serviceKey2Name = generator.PrefixedRandomName(testConfig.GetNamePrefix(), "test-service-key-2")
+
+						createBucketWithCleanup(serviceInstance2Name)
+						createServiceKeyWithCleanup(
+							serviceInstance2Name,
+							serviceKey2Name,
+							`true`,
+						)
+					})
+
+					It("is not able to use the first key to access the second bucket", func() {
+						var bucket2Name string
+
+						By("retrieving the other bucket name", func() {
+							_, bucket2Name = s3ClientFromServiceKey(
+								serviceInstance2Name,
+								serviceKey2Name,
+							)
+						})
+
+						By("failing to access the other bucket", func() {
+							assertNoBucketAccess(bucket2Name)
+						})
+					})
+				})
+			})
 		})
 	})
 

platform-tests/example-apps/healthcheck/s3.go

@@ -52,7 +52,7 @@ func testS3BucketAccess() error {
 		Bucket: aws.String(vcapService.BucketName),
 	})
 	if err != nil {
-		return err
+		return errors.Wrap(err, "ListObjects")
 	}
 
 	_, err = s3Client.PutObject(&s3.PutObjectInput{
@@ -61,23 +61,34 @@ func testS3BucketAccess() error {
 		Body:   strings.NewReader(testS3Content),
 	})
 	if err != nil {
-		return err
+		return errors.Wrap(err, "PutObject")
 	}
 
 	getObjectOutput, err := s3Client.GetObject(&s3.GetObjectInput{
 		Bucket: aws.String(vcapService.BucketName),
 		Key:    aws.String(testS3File),
 	})
 	if err != nil {
-		return err
+		return errors.Wrap(err, "GetObject")
 	}
 
 	content, err := ioutil.ReadAll(getObjectOutput.Body)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "ioutil.ReadAll")
 	}
 	defer getObjectOutput.Body.Close()
 
+	taggingOutput, err := s3Client.GetObjectTagging(&s3.GetObjectTaggingInput{
+		Bucket: aws.String(vcapService.BucketName),
+		Key:    aws.String(testS3File),
+	})
+	if err != nil {
+		return errors.Wrap(err, "GetObjectTagging")
+	}
+	if len(taggingOutput.TagSet) != 0 {
+		return fmt.Errorf("expected empty TagSet for %q, but was %q", testS3File, taggingOutput.TagSet)
+	}
+
 	if string(content) != testS3Content {
 		return fmt.Errorf("content mismatch, was writing %q but read %q", testS3Content, string(content))
 	}
@@ -87,7 +98,7 @@ func testS3BucketAccess() error {
 		Key:    aws.String(testS3File),
 	})
 	if err != nil {
-		return err
+		return errors.Wrap(err, "DeleteObject")
 	}
 
 	return nil

terraform/cloudfoundry/outputs.tf

@@ -145,6 +145,14 @@ output "s3_broker_ip_restriction_policy_arn" {
   value = aws_iam_policy.s3_broker_user_ip_restriction.arn
 }
 
+output "s3_broker_common_user_policy_arn" {
+  value = aws_iam_policy.s3_broker_user_common.arn
+}
+
+output "s3_broker_user_permissions_boundary_arn" {
+  value = "arn:aws:iam::${data.aws_caller_identity.current.account_id}:policy/S3BrokerUserPermissionsBoundary"
+}
+
 output "restrict_to_local_ips_policy_arn" {
   value = aws_iam_policy.restrict_to_local_ips.arn
 }