From f4984dafab1aa57f9723051564f787cb36f7a4fc Mon Sep 17 00:00:00 2001
From: justinsb <justinsb@google.com>
Date: Sun, 11 Dec 2022 21:37:15 -0500
Subject: [PATCH] Support public buckets for serviceAccountIssuers on S3

S3 is also nudging towards bucket level permissions, so don't set an ACL when bucket is public.
---
 pkg/model/issuerdiscovery.go | 41 +++++++++++++++++++++++---
 util/pkg/vfs/s3fs.go         | 56 ++++++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+), 4 deletions(-)

diff --git a/pkg/model/issuerdiscovery.go b/pkg/model/issuerdiscovery.go
index 40d63d05ff..88afcd63c2 100644
--- a/pkg/model/issuerdiscovery.go
+++ b/pkg/model/issuerdiscovery.go
@@ -18,6 +18,7 @@ package model
 
 import (
 	"bytes"
+	"context"
 	"crypto"
 	"crypto/x509"
 	"encoding/base64"
@@ -27,9 +28,11 @@ import (
 	"sort"
 
 	"gopkg.in/square/go-jose.v2"
+	"k8s.io/klog/v2"
 	"k8s.io/kops/pkg/apis/kops"
 	"k8s.io/kops/upup/pkg/fi"
 	"k8s.io/kops/upup/pkg/fi/fitasks"
+	"k8s.io/kops/util/pkg/vfs"
 )
 
 // IssuerDiscoveryModelBuilder publish OIDC issuer discovery metadata
@@ -51,6 +54,8 @@ type oidcDiscovery struct {
 }
 
 func (b *IssuerDiscoveryModelBuilder) Build(c *fi.ModelBuilderContext) error {
+	ctx := context.TODO()
+
 	said := b.Cluster.Spec.ServiceAccountIssuerDiscovery
 	if said == nil || said.DiscoveryStore == "" {
 		return nil
@@ -71,13 +76,41 @@ func (b *IssuerDiscoveryModelBuilder) Build(c *fi.ModelBuilderContext) error {
 	if err != nil {
 		return err
 	}
+
+	publicFileACL := fi.PtrTo(true)
+
+	discoveryStorePath := b.Cluster.Spec.ServiceAccountIssuerDiscovery.DiscoveryStore
+	discoveryStore, err := vfs.Context.BuildVfsPath(discoveryStorePath)
+	if err != nil {
+		return fmt.Errorf("building VFS path for %q: %w", discoveryStorePath, err)
+	}
+
+	switch discoveryStore := discoveryStore.(type) {
+	case *vfs.S3Path:
+		isPublic, err := discoveryStore.IsBucketPublic(ctx)
+		if err != nil {
+			return fmt.Errorf("checking if bucket was public: %w", err)
+		}
+		if !isPublic {
+			klog.Infof("serviceAccountIssuers bucket %q is not public, will use object ACL", discoveryStore.Bucket())
+		} else {
+			publicFileACL = nil
+		}
+
+	case *vfs.MemFSPath:
+		// ok
+
+	default:
+		return fmt.Errorf("unhandled type %T", discoveryStore)
+	}
+
 	keysFile := &fitasks.ManagedFile{
 		Contents:  keys,
 		Lifecycle: b.Lifecycle,
 		Location:  fi.PtrTo("openid/v1/jwks"),
 		Name:      fi.PtrTo("keys.json"),
-		Base:      fi.PtrTo(b.Cluster.Spec.ServiceAccountIssuerDiscovery.DiscoveryStore),
-		PublicACL: fi.PtrTo(true),
+		Base:      fi.PtrTo(discoveryStorePath),
+		PublicACL: publicFileACL,
 	}
 	c.AddTask(keysFile)
 
@@ -86,8 +119,8 @@ func (b *IssuerDiscoveryModelBuilder) Build(c *fi.ModelBuilderContext) error {
 		Lifecycle: b.Lifecycle,
 		Location:  fi.PtrTo(".well-known/openid-configuration"),
 		Name:      fi.PtrTo("discovery.json"),
-		Base:      fi.PtrTo(b.Cluster.Spec.ServiceAccountIssuerDiscovery.DiscoveryStore),
-		PublicACL: fi.PtrTo(true),
+		Base:      fi.PtrTo(discoveryStorePath),
+		PublicACL: publicFileACL,
 	}
 	c.AddTask(discoveryFile)
 
diff --git a/util/pkg/vfs/s3fs.go b/util/pkg/vfs/s3fs.go
index cc1982222c..f40b3d3c42 100644
--- a/util/pkg/vfs/s3fs.go
+++ b/util/pkg/vfs/s3fs.go
@@ -18,6 +18,7 @@ package vfs
 
 import (
 	"bytes"
+	"context"
 	"encoding/hex"
 	"fmt"
 	"io"
@@ -503,6 +504,61 @@ func (p *S3Path) GetHTTPsUrl(dualstack bool) (string, error) {
 	return strings.TrimSuffix(url, "/"), nil
 }
 
+func (p *S3Path) IsBucketPublic(ctx context.Context) (bool, error) {
+	client, err := p.client()
+	if err != nil {
+		return false, err
+	}
+
+	result, err := client.GetBucketPolicyStatusWithContext(ctx, &s3.GetBucketPolicyStatusInput{})
+	if err != nil {
+		return false, fmt.Errorf("from AWS S3 GetBucketPolicyStatusWithContext: %w", err)
+	}
+	if aws.BoolValue(result.PolicyStatus.IsPublic) {
+		return true, nil
+	}
+	return false, nil
+
+	// We could check bucket ACLs also...
+
+	// acl, err := client.GetBucketAclWithContext(ctx, &s3.GetBucketAclInput{
+	// 	Bucket: &p.bucket,
+	// })
+	// if err != nil {
+	// 	return false, fmt.Errorf("failed to get ACL for bucket %q: %w", p.bucket, err)
+	// }
+
+	// allowsAnonymousRead := false
+	// for _, grant := range acl.Grants {
+	// 	isAllUsers := false
+
+	// 	switch aws.StringValue(grant.Grantee.URI) {
+	// 	case "http://acs.amazonaws.com/groups/global/AllUsers":
+	// 		isAllUsers = true
+	// 	}
+
+	// 	if isAllUsers {
+	// 		permission := aws.StringValue(grant.Permission)
+	// 		switch permission {
+	// 		case "FULL_CONTROL":
+	// 			klog.Warningf("bucket %q allows anonymous users full access", p.bucket)
+	// 			allowsAnonymousRead = true
+	// 		case "WRITE", "WRITE_ACP":
+	// 			klog.Warningf("bucket %q allows anonymous users write access", p.bucket)
+	// 			// it's not _read_ access
+	// 		case "READ":
+	// 			allowsAnonymousRead = true
+	// 		case "READ_ACP":
+	// 			// does not grant read
+	// 		default:
+	// 			klog.Warningf("bucket %q has unknown permission %q for anonymous access", p.bucket, permission)
+	// 		}
+	// 	}
+	// }
+
+	// return allowsAnonymousRead, nil
+}
+
 func (p *S3Path) IsPublic() (bool, error) {
 	client, err := p.client()
 	if err != nil {