From c75e08415837adce66f1671d606de6803bf063ec Mon Sep 17 00:00:00 2001 From: Justin SB Date: Sat, 20 Mar 2021 14:31:39 -0400 Subject: [PATCH] Re-add integration tests for jwks We removed them from #10756, but they can be re-added. --- cmd/kops/integration_test.go | 24 +++++++++++++++ pkg/apis/kops/validation/BUILD.bazel | 1 + pkg/apis/kops/validation/legacy.go | 19 ++++++++++-- pkg/model/iam/subject.go | 7 +++++ ....kube-system.sa.minimal.example.com_policy | 4 +-- ..._policy_masters.minimal.example.com_policy | 29 ------------------- ...t-1a.masters.minimal.example.com_user_data | 8 +++-- .../public-jwks/in-v1alpha2.yaml | 1 + .../update_cluster/public-jwks/kubernetes.tf | 4 +-- .../cloudup/bootstrapchannelbuilder_test.go | 14 +++++++++ .../public-jwks/cluster.yaml | 1 + upup/pkg/fi/fitasks/managedfile.go | 7 ++++- 12 files changed, 80 insertions(+), 39 deletions(-) diff --git a/cmd/kops/integration_test.go b/cmd/kops/integration_test.go index 5a1c5313f0..a7c2f394ba 100644 --- a/cmd/kops/integration_test.go +++ b/cmd/kops/integration_test.go @@ -79,6 +79,12 @@ func newIntegrationTest(clusterName, srcDir string) *integrationTest { } } +// withCAKey indicates that we should use a fixed ca.crt & ca.key from the source directory as our CA. +// This is needed when the CA is exposed, for example when using AWS WebIdentity federation. +func (i *integrationTest) withCAKey() *integrationTest { + i.caKey = true + return i +} func (i *integrationTest) withVersion(version string) *integrationTest { i.version = version return i @@ -114,6 +120,12 @@ func (i *integrationTest) withPrivate() *integrationTest { return i } +// withServiceAccountRoles indicates we expect to assign per-ServiceAccount IAM roles (instead of just using the node roles) +func (i *integrationTest) withServiceAccountRoles() *integrationTest { + i.expectServiceAccountRoles = true + return i +} + func (i *integrationTest) withBastionUserData() *integrationTest { i.bastionUserData = true return i @@ -262,6 +274,18 @@ func TestPrivateDns2(t *testing.T) { newIntegrationTest("privatedns2.example.com", "privatedns2").withPrivate().runTestTerraformAWS(t) } +// TestPublicJWKS runs a simple configuration, but with UseServiceAccountIAM and PublicJWKS enabled +func TestPublicJWKS(t *testing.T) { + featureflag.ParseFlags("+UseServiceAccountIAM,+PublicJWKS") + unsetFeatureFlags := func() { + featureflag.ParseFlags("-UseServiceAccountIAM,-PublicJWKS") + } + defer unsetFeatureFlags() + + // We have to use a fixed CA because the fingerprint is inserted into the AWS WebIdentity configuration. + newIntegrationTest("minimal.example.com", "public-jwks").withCAKey().withServiceAccountRoles().runTestTerraformAWS(t) +} + // TestSharedSubnet runs the test on a configuration with a shared subnet (and VPC) func TestSharedSubnet(t *testing.T) { newIntegrationTest("sharedsubnet.example.com", "shared_subnet").runTestTerraformAWS(t) diff --git a/pkg/apis/kops/validation/BUILD.bazel b/pkg/apis/kops/validation/BUILD.bazel index 7738027ad9..10f0cb3975 100644 --- a/pkg/apis/kops/validation/BUILD.bazel +++ b/pkg/apis/kops/validation/BUILD.bazel @@ -24,6 +24,7 @@ go_library( "//pkg/util/subnet:go_default_library", "//upup/pkg/fi:go_default_library", "//upup/pkg/fi/cloudup/awsup:go_default_library", + "//util/pkg/vfs:go_default_library", "//vendor/github.com/aws/aws-sdk-go/aws/arn:go_default_library", "//vendor/github.com/aws/aws-sdk-go/service/ec2:go_default_library", "//vendor/github.com/blang/semver/v4:go_default_library", diff --git a/pkg/apis/kops/validation/legacy.go b/pkg/apis/kops/validation/legacy.go index bd569babdc..a28ada4ac7 100644 --- a/pkg/apis/kops/validation/legacy.go +++ b/pkg/apis/kops/validation/legacy.go @@ -27,6 +27,7 @@ import ( "k8s.io/kops/pkg/featureflag" "k8s.io/kops/pkg/util/subnet" "k8s.io/kops/upup/pkg/fi" + "k8s.io/kops/util/pkg/vfs" ) // legacy contains validation functions that don't match the apimachinery style @@ -374,8 +375,22 @@ func ValidateCluster(c *kops.Cluster, strict bool) field.ErrorList { publicDataStore := c.Spec.PublicDataStore if publicDataStore != "" { - if !strings.HasPrefix(publicDataStore, "s3://") { - allErrs = append(allErrs, field.Invalid(fieldSpec.Child("publicDataStore"), publicDataStore, "S3 is the only supported VFS for publicStore")) + base, err := vfs.Context.BuildVfsPath(publicDataStore) + if err != nil { + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("publicDataStore"), publicDataStore, "not a valid VFS path")) + } else { + switch base := base.(type) { + case *vfs.S3Path: + // OK + case *vfs.MemFSPath: + // memfs is ok for tests; not OK otherwise + if !base.IsClusterReadable() { + // (If this _is_ a test, we should call MarkClusterReadable) + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("publicDataStore"), publicDataStore, "S3 is the only supported VFS for publicStore")) + } + default: + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("publicDataStore"), publicDataStore, "S3 is the only supported VFS for publicStore")) + } } } diff --git a/pkg/model/iam/subject.go b/pkg/model/iam/subject.go index 90d72adfd5..acb9a955ac 100644 --- a/pkg/model/iam/subject.go +++ b/pkg/model/iam/subject.go @@ -18,6 +18,7 @@ package iam import ( "fmt" + "strings" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" @@ -108,6 +109,12 @@ func ServiceAccountIssuer(clusterSpec *kops.ClusterSpec) (string, error) { return "", err } return baseURL + "/oidc", nil + case *vfs.MemFSPath: + if !base.IsClusterReadable() { + // If this _is_ a test, we should call MarkClusterReadable + return "", fmt.Errorf("cluster.spec.publicDataStore=%q is only supported in tests", clusterSpec.PublicDataStore) + } + return strings.Replace(base.Path(), "memfs://", "https://", 1) + "/oidc", nil default: return "", fmt.Errorf("cluster.spec.publicDataStore=%q is of unexpected type %T", clusterSpec.PublicDataStore, base) } diff --git a/tests/integration/update_cluster/public-jwks/data/aws_iam_role_dns-controller.kube-system.sa.minimal.example.com_policy b/tests/integration/update_cluster/public-jwks/data/aws_iam_role_dns-controller.kube-system.sa.minimal.example.com_policy index 5d7e379e46..529264db47 100644 --- a/tests/integration/update_cluster/public-jwks/data/aws_iam_role_dns-controller.kube-system.sa.minimal.example.com_policy +++ b/tests/integration/update_cluster/public-jwks/data/aws_iam_role_dns-controller.kube-system.sa.minimal.example.com_policy @@ -4,12 +4,12 @@ "Action": "sts:AssumeRoleWithWebIdentity", "Condition": { "StringEquals": { - "api.minimal.example.com:sub": "system:serviceaccount:kube-system:dns-controller" + "discovery.example.com/minimal.example.com/oidc:sub": "system:serviceaccount:kube-system:dns-controller" } }, "Effect": "Allow", "Principal": { - "Federated": "arn:aws:iam::123456789012:oidc-provider/api.minimal.example.com" + "Federated": "arn:aws:iam::123456789012:oidc-provider/discovery.example.com/minimal.example.com/oidc" } } ], diff --git a/tests/integration/update_cluster/public-jwks/data/aws_iam_role_policy_masters.minimal.example.com_policy b/tests/integration/update_cluster/public-jwks/data/aws_iam_role_policy_masters.minimal.example.com_policy index 47e801abe7..5fabb0ca67 100644 --- a/tests/integration/update_cluster/public-jwks/data/aws_iam_role_policy_masters.minimal.example.com_policy +++ b/tests/integration/update_cluster/public-jwks/data/aws_iam_role_policy_masters.minimal.example.com_policy @@ -135,35 +135,6 @@ "Resource": [ "*" ] - }, - { - "Action": [ - "route53:ChangeResourceRecordSets", - "route53:ListResourceRecordSets", - "route53:GetHostedZone" - ], - "Effect": "Allow", - "Resource": [ - "arn:aws:route53:::hostedzone/Z1AFAKE1ZON3YO" - ] - }, - { - "Action": [ - "route53:GetChange" - ], - "Effect": "Allow", - "Resource": [ - "arn:aws:route53:::change/*" - ] - }, - { - "Action": [ - "route53:ListHostedZones" - ], - "Effect": "Allow", - "Resource": [ - "*" - ] } ], "Version": "2012-10-17" diff --git a/tests/integration/update_cluster/public-jwks/data/aws_launch_template_master-us-test-1a.masters.minimal.example.com_user_data b/tests/integration/update_cluster/public-jwks/data/aws_launch_template_master-us-test-1a.masters.minimal.example.com_user_data index 0356010d6a..6517e2bfe6 100644 --- a/tests/integration/update_cluster/public-jwks/data/aws_launch_template_master-us-test-1a.masters.minimal.example.com_user_data +++ b/tests/integration/update_cluster/public-jwks/data/aws_launch_template_master-us-test-1a.masters.minimal.example.com_user_data @@ -168,7 +168,7 @@ etcdClusters: version: 3.4.13 kubeAPIServer: allowPrivileged: true - anonymousAuth: true + anonymousAuth: false apiAudiences: - kubernetes.svc.default apiServerCount: 1 @@ -190,6 +190,8 @@ kubeAPIServer: - http://127.0.0.1:4001 etcdServersOverrides: - /events#http://127.0.0.1:4002 + featureGates: + ServiceAccountIssuerDiscovery: "true" image: k8s.gcr.io/kube-apiserver:v1.20.0 kubeletPreferredAddressTypes: - InternalIP @@ -205,8 +207,8 @@ kubeAPIServer: requestheaderUsernameHeaders: - X-Remote-User securePort: 443 - serviceAccountIssuer: https://api.minimal.example.com - serviceAccountJWKSURI: https://api.minimal.example.com/openid/v1/jwks + serviceAccountIssuer: https://discovery.example.com/minimal.example.com/oidc + serviceAccountJWKSURI: https://discovery.example.com/minimal.example.com/oidc/keys.json serviceClusterIPRange: 100.64.0.0/13 storageBackend: etcd3 kubeControllerManager: diff --git a/tests/integration/update_cluster/public-jwks/in-v1alpha2.yaml b/tests/integration/update_cluster/public-jwks/in-v1alpha2.yaml index d1ff60d319..492585df0f 100644 --- a/tests/integration/update_cluster/public-jwks/in-v1alpha2.yaml +++ b/tests/integration/update_cluster/public-jwks/in-v1alpha2.yaml @@ -28,6 +28,7 @@ spec: networking: cni: {} nonMasqueradeCIDR: 100.64.0.0/10 + publicDataStore: memfs://discovery.example.com/minimal.example.com sshAccess: - 0.0.0.0/0 topology: diff --git a/tests/integration/update_cluster/public-jwks/kubernetes.tf b/tests/integration/update_cluster/public-jwks/kubernetes.tf index 5452778830..e5b9f068c4 100644 --- a/tests/integration/update_cluster/public-jwks/kubernetes.tf +++ b/tests/integration/update_cluster/public-jwks/kubernetes.tf @@ -245,8 +245,8 @@ resource "aws_iam_instance_profile" "nodes-minimal-example-com" { resource "aws_iam_openid_connect_provider" "minimal-example-com" { client_id_list = ["amazonaws.com"] - thumbprint_list = ["a8de31f85544b9e73aeb26ded19330e0e996fb79"] - url = "https://api.minimal.example.com" + thumbprint_list = ["9e99a48a9960b14926bb7f3b02e22da2b0ab7280", "a9d53002e97e00e043244f3d170d6f4c414104fd"] + url = "https://discovery.example.com/minimal.example.com/oidc" } resource "aws_iam_role_policy" "dns-controller-kube-system-sa-minimal-example-com" { diff --git a/upup/pkg/fi/cloudup/bootstrapchannelbuilder_test.go b/upup/pkg/fi/cloudup/bootstrapchannelbuilder_test.go index 8dc3fd0375..26111fc1a7 100644 --- a/upup/pkg/fi/cloudup/bootstrapchannelbuilder_test.go +++ b/upup/pkg/fi/cloudup/bootstrapchannelbuilder_test.go @@ -53,6 +53,20 @@ func TestBootstrapChannelBuilder_BuildTasks(t *testing.T) { runChannelBuilderTest(t, "awsiamauthenticator", []string{"authentication.aws-k8s-1.12"}) } +func TestBootstrapChannelBuilder_PublicJWKS(t *testing.T) { + h := testutils.NewIntegrationTestHarness(t) + defer h.Close() + + h.SetupMockAWS() + + featureflag.ParseFlags("+PublicJWKS,+UseServiceAccountIAM") + unsetFeatureFlag := func() { + featureflag.ParseFlags("-PublicJWKS,-UseServiceAccountIAM") + } + defer unsetFeatureFlag() + runChannelBuilderTest(t, "public-jwks", []string{"dns-controller.addons.k8s.io-k8s-1.12", "kops-controller.addons.k8s.io-k8s-1.16", "anonymous-issuer-discovery.addons.k8s.io-k8s-1.16"}) +} + func TestBootstrapChannelBuilder_AWSCloudController(t *testing.T) { h := testutils.NewIntegrationTestHarness(t) defer h.Close() diff --git a/upup/pkg/fi/cloudup/tests/bootstrapchannelbuilder/public-jwks/cluster.yaml b/upup/pkg/fi/cloudup/tests/bootstrapchannelbuilder/public-jwks/cluster.yaml index 91d03333db..ccd5304ea4 100644 --- a/upup/pkg/fi/cloudup/tests/bootstrapchannelbuilder/public-jwks/cluster.yaml +++ b/upup/pkg/fi/cloudup/tests/bootstrapchannelbuilder/public-jwks/cluster.yaml @@ -30,6 +30,7 @@ spec: networking: kubenet: {} nonMasqueradeCIDR: 100.64.0.0/10 + publicDataStore: memfs://discovery.example.com/minimal.example.com sshAccess: - 0.0.0.0/0 topology: diff --git a/upup/pkg/fi/fitasks/managedfile.go b/upup/pkg/fi/fitasks/managedfile.go index 65b3a21f9d..3fb3226a22 100644 --- a/upup/pkg/fi/fitasks/managedfile.go +++ b/upup/pkg/fi/fitasks/managedfile.go @@ -105,11 +105,16 @@ func (_ *ManagedFile) Render(c *fi.Context, a, e, changes *ManagedFile) error { var acl vfs.ACL if fi.BoolValue(e.Public) { - switch p.(type) { + switch p := p.(type) { case *vfs.S3Path: acl = &vfs.S3Acl{ RequestACL: fi.String("public-read"), } + case *vfs.MemFSPath: + if !p.IsClusterReadable() { + return fmt.Errorf("the %q path is intended for use in tests", p.Path()) + } + acl = nil default: return fmt.Errorf("the %q path does not support public ACL", p.Path()) }