From e2a92c583e208e11be9e3c1f5b39a3813a4a8c03 Mon Sep 17 00:00:00 2001 From: Noah Stride Date: Tue, 21 Jan 2025 12:31:46 +0000 Subject: [PATCH 1/4] Remove feature flags for new WorkloadIdentity UX (#51233) * Add "unstable" disable join attrs flag * Remove experimental ff * Remove CLI hidden flags * Americanize spelling * "1" -> "yes" --- .../experiment/experiment.go | 41 ------------------- .../workloadidentityv1/issuer_service.go | 9 ---- .../workloadidentityv1_test.go | 13 ++---- .../service_workload_identity_api_test.go | 5 +-- .../service_workload_identity_jwt_test.go | 5 +-- .../service_workload_identity_x509_test.go | 5 +-- lib/tlsca/ca.go | 10 ++++- 7 files changed, 15 insertions(+), 73 deletions(-) delete mode 100644 lib/auth/machineid/workloadidentityv1/experiment/experiment.go diff --git a/lib/auth/machineid/workloadidentityv1/experiment/experiment.go b/lib/auth/machineid/workloadidentityv1/experiment/experiment.go deleted file mode 100644 index fafe51ea83d1f..0000000000000 --- a/lib/auth/machineid/workloadidentityv1/experiment/experiment.go +++ /dev/null @@ -1,41 +0,0 @@ -// Teleport -// Copyright (C) 2024 Gravitational, Inc. -// -// This program is free software: you can redistribute it and/or modify -// it under the terms of the GNU Affero General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. -// -// This program is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU Affero General Public License for more details. -// -// You should have received a copy of the GNU Affero General Public License -// along with this program. If not, see . - -package experiment - -import ( - "os" - "sync" -) - -var mu sync.Mutex - -var experimentEnabled = os.Getenv("TELEPORT_WORKLOAD_IDENTITY_UX_EXPERIMENT") == "1" - -// Enabled returns true if the workload identity UX experiment is -// enabled. -func Enabled() bool { - mu.Lock() - defer mu.Unlock() - return experimentEnabled -} - -// SetEnabled sets the experiment enabled flag. -func SetEnabled(enabled bool) { - mu.Lock() - defer mu.Unlock() - experimentEnabled = enabled -} diff --git a/lib/auth/machineid/workloadidentityv1/issuer_service.go b/lib/auth/machineid/workloadidentityv1/issuer_service.go index 6842ae01632ec..73694e0de54e8 100644 --- a/lib/auth/machineid/workloadidentityv1/issuer_service.go +++ b/lib/auth/machineid/workloadidentityv1/issuer_service.go @@ -39,7 +39,6 @@ import ( "github.com/gravitational/teleport/api/observability/tracing" "github.com/gravitational/teleport/api/types" apievents "github.com/gravitational/teleport/api/types/events" - "github.com/gravitational/teleport/lib/auth/machineid/workloadidentityv1/experiment" "github.com/gravitational/teleport/lib/authz" "github.com/gravitational/teleport/lib/events" "github.com/gravitational/teleport/lib/jwt" @@ -147,10 +146,6 @@ func (s *IssuanceService) IssueWorkloadIdentity( ctx context.Context, req *workloadidentityv1pb.IssueWorkloadIdentityRequest, ) (*workloadidentityv1pb.IssueWorkloadIdentityResponse, error) { - if !experiment.Enabled() { - return nil, trace.AccessDenied("workload identity issuance experiment is disabled") - } - switch { case req.GetName() == "": return nil, trace.BadParameter("name: is required") @@ -240,10 +235,6 @@ func (s *IssuanceService) IssueWorkloadIdentities( ctx context.Context, req *workloadidentityv1pb.IssueWorkloadIdentitiesRequest, ) (*workloadidentityv1pb.IssueWorkloadIdentitiesResponse, error) { - if !experiment.Enabled() { - return nil, trace.AccessDenied("workload identity issuance experiment is disabled") - } - switch { case len(req.LabelSelectors) == 0: return nil, trace.BadParameter("label_selectors: at least one label selector must be specified") diff --git a/lib/auth/machineid/workloadidentityv1/workloadidentityv1_test.go b/lib/auth/machineid/workloadidentityv1/workloadidentityv1_test.go index 59f6ced156e58..3feee4fb792d6 100644 --- a/lib/auth/machineid/workloadidentityv1/workloadidentityv1_test.go +++ b/lib/auth/machineid/workloadidentityv1/workloadidentityv1_test.go @@ -52,7 +52,6 @@ import ( "github.com/gravitational/teleport/lib/auth" "github.com/gravitational/teleport/lib/auth/authclient" "github.com/gravitational/teleport/lib/auth/join" - "github.com/gravitational/teleport/lib/auth/machineid/workloadidentityv1/experiment" "github.com/gravitational/teleport/lib/auth/native" "github.com/gravitational/teleport/lib/auth/state" "github.com/gravitational/teleport/lib/auth/testauthority" @@ -153,9 +152,7 @@ func newIssuanceTestPack(t *testing.T, ctx context.Context) *issuanceTestPack { // APIs necessary for a bot to join and then issue a workload identity are // functioning correctly. func TestIssueWorkloadIdentityE2E(t *testing.T) { - experimentStatus := experiment.Enabled() - defer experiment.SetEnabled(experimentStatus) - experiment.SetEnabled(true) + t.Parallel() ctx := context.Background() tp := newIssuanceTestPack(t, ctx) @@ -341,9 +338,7 @@ func TestIssueWorkloadIdentityE2E(t *testing.T) { } func TestIssueWorkloadIdentity(t *testing.T) { - experimentStatus := experiment.Enabled() - defer experiment.SetEnabled(experimentStatus) - experiment.SetEnabled(true) + t.Parallel() ctx := context.Background() tp := newIssuanceTestPack(t, ctx) @@ -731,9 +726,7 @@ func TestIssueWorkloadIdentity(t *testing.T) { } func TestIssueWorkloadIdentities(t *testing.T) { - experimentStatus := experiment.Enabled() - defer experiment.SetEnabled(experimentStatus) - experiment.SetEnabled(true) + t.Parallel() ctx := context.Background() tp := newIssuanceTestPack(t, ctx) diff --git a/lib/tbot/service_workload_identity_api_test.go b/lib/tbot/service_workload_identity_api_test.go index 6570b343f5686..921b70f398037 100644 --- a/lib/tbot/service_workload_identity_api_test.go +++ b/lib/tbot/service_workload_identity_api_test.go @@ -35,16 +35,13 @@ import ( workloadidentityv1pb "github.com/gravitational/teleport/api/gen/proto/go/teleport/workloadidentity/v1" "github.com/gravitational/teleport/api/types" apiutils "github.com/gravitational/teleport/api/utils" - "github.com/gravitational/teleport/lib/auth/machineid/workloadidentityv1/experiment" "github.com/gravitational/teleport/lib/tbot/config" "github.com/gravitational/teleport/lib/utils" "github.com/gravitational/teleport/tool/teleport/testenv" ) func TestBotWorkloadIdentityAPI(t *testing.T) { - experimentStatus := experiment.Enabled() - defer experiment.SetEnabled(experimentStatus) - experiment.SetEnabled(true) + t.Parallel() ctx := context.Background() log := utils.NewSlogLoggerForTests() diff --git a/lib/tbot/service_workload_identity_jwt_test.go b/lib/tbot/service_workload_identity_jwt_test.go index dacd495ee6959..abb785171dac6 100644 --- a/lib/tbot/service_workload_identity_jwt_test.go +++ b/lib/tbot/service_workload_identity_jwt_test.go @@ -30,16 +30,13 @@ import ( workloadidentityv1pb "github.com/gravitational/teleport/api/gen/proto/go/teleport/workloadidentity/v1" "github.com/gravitational/teleport/api/types" apiutils "github.com/gravitational/teleport/api/utils" - "github.com/gravitational/teleport/lib/auth/machineid/workloadidentityv1/experiment" "github.com/gravitational/teleport/lib/tbot/config" "github.com/gravitational/teleport/lib/utils" "github.com/gravitational/teleport/tool/teleport/testenv" ) func TestBotWorkloadIdentityJWT(t *testing.T) { - experimentStatus := experiment.Enabled() - defer experiment.SetEnabled(experimentStatus) - experiment.SetEnabled(true) + t.Parallel() ctx := context.Background() log := utils.NewSlogLoggerForTests() diff --git a/lib/tbot/service_workload_identity_x509_test.go b/lib/tbot/service_workload_identity_x509_test.go index 81d0823cc7f4c..00e4317b7a3eb 100644 --- a/lib/tbot/service_workload_identity_x509_test.go +++ b/lib/tbot/service_workload_identity_x509_test.go @@ -32,16 +32,13 @@ import ( workloadidentityv1pb "github.com/gravitational/teleport/api/gen/proto/go/teleport/workloadidentity/v1" "github.com/gravitational/teleport/api/types" apiutils "github.com/gravitational/teleport/api/utils" - "github.com/gravitational/teleport/lib/auth/machineid/workloadidentityv1/experiment" "github.com/gravitational/teleport/lib/tbot/config" "github.com/gravitational/teleport/lib/utils" "github.com/gravitational/teleport/tool/teleport/testenv" ) func TestBotWorkloadIdentityX509(t *testing.T) { - experimentStatus := experiment.Enabled() - defer experiment.SetEnabled(experimentStatus) - experiment.SetEnabled(true) + t.Parallel() ctx := context.Background() log := utils.NewSlogLoggerForTests() diff --git a/lib/tlsca/ca.go b/lib/tlsca/ca.go index 4fdaea26901c7..02c5b4d379766 100644 --- a/lib/tlsca/ca.go +++ b/lib/tlsca/ca.go @@ -29,6 +29,7 @@ import ( "fmt" "math/big" "net" + "os" "strconv" "time" @@ -885,7 +886,7 @@ func (id *Identity) Subject() (pkix.Name, error) { ) } - if id.JoinAttributes != nil { + if id.JoinAttributes != nil && shouldPersistJoinAttrs() { encoded, err := protojson.MarshalOptions{ // Use the proto field names as this is what we use in the // templating engine and this being consistent for any user who @@ -1318,3 +1319,10 @@ func (ca *CertAuthority) GenerateCertificate(req CertificateRequest) ([]byte, er return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certBytes}), nil } + +// shouldPersistJoinAttrs returns true if the join attributes should be persisted +// into the X509 identity. This provides an emergency "off" handle for this +// new behavior until we are confident it is working as expected. +func shouldPersistJoinAttrs() bool { + return os.Getenv("TELEPORT_UNSTABLE_DISABLE_JOIN_ATTRS") != "yes" +} From c3de24ced9713c8ce3902f28c0c371da3e81681e Mon Sep 17 00:00:00 2001 From: Noah Stride Date: Tue, 21 Jan 2025 13:33:28 +0000 Subject: [PATCH 2/4] Fix experimetn --- lib/auth/bot.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/lib/auth/bot.go b/lib/auth/bot.go index e2294e9f5b552..3d9498b565604 100644 --- a/lib/auth/bot.go +++ b/lib/auth/bot.go @@ -38,7 +38,6 @@ import ( apiutils "github.com/gravitational/teleport/api/utils" "github.com/gravitational/teleport/api/utils/keys" "github.com/gravitational/teleport/lib/auth/machineid/machineidv1" - "github.com/gravitational/teleport/lib/auth/machineid/workloadidentityv1/experiment" "github.com/gravitational/teleport/lib/authz" "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/events" @@ -536,11 +535,6 @@ func (a *Server) generateInitialBotCerts( loginIP: loginIP, botName: botName, } - // Feature flag with workload id experiment env var to allow us to test this - // in master/v18 without enabling it for everyone. - if experiment.Enabled() { - certReq.joinAttributes = joinAttrs - } if existingInstanceID == "" { // If no existing instance ID is known, create a new one. From 4f719f8c990f6d75416b97d0c2ce0559ea0b66a5 Mon Sep 17 00:00:00 2001 From: Noah Stride Date: Mon, 27 Jan 2025 14:06:21 +0000 Subject: [PATCH 3/4] Fix backport --- lib/auth/bot_test.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/lib/auth/bot_test.go b/lib/auth/bot_test.go index 270bb9581101b..4634642ae6754 100644 --- a/lib/auth/bot_test.go +++ b/lib/auth/bot_test.go @@ -57,7 +57,6 @@ import ( "github.com/gravitational/teleport/lib/auth/authclient" "github.com/gravitational/teleport/lib/auth/join" "github.com/gravitational/teleport/lib/auth/machineid/machineidv1" - "github.com/gravitational/teleport/lib/auth/machineid/workloadidentityv1/experiment" "github.com/gravitational/teleport/lib/auth/state" "github.com/gravitational/teleport/lib/auth/testauthority" "github.com/gravitational/teleport/lib/cloud/azure" @@ -224,10 +223,6 @@ func TestRegisterBotCertificateGenerationCheck(t *testing.T) { // Whilst this specifically tests the Kubernetes join method, it tests by proxy // the implementation for most of the join methods. func TestBotJoinAttrs_Kubernetes(t *testing.T) { - experimentStatus := experiment.Enabled() - defer experiment.SetEnabled(experimentStatus) - experiment.SetEnabled(true) - srv := newTestTLSServer(t) ctx := context.Background() From a950ef7956b288681a1ad62b71ee771eb5991760 Mon Sep 17 00:00:00 2001 From: Noah Stride Date: Tue, 28 Jan 2025 14:15:10 +0000 Subject: [PATCH 4/4] fix backport --- lib/auth/bot.go | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/auth/bot.go b/lib/auth/bot.go index 3d9498b565604..8d4418b0dc44e 100644 --- a/lib/auth/bot.go +++ b/lib/auth/bot.go @@ -525,15 +525,16 @@ func (a *Server) generateInitialBotCerts( // Generate certificate certReq := certRequest{ - user: userState, - ttl: expires.Sub(a.GetClock().Now()), - publicKey: pubKey, - checker: checker, - traits: accessInfo.Traits, - renewable: renewable, - includeHostCA: true, - loginIP: loginIP, - botName: botName, + user: userState, + ttl: expires.Sub(a.GetClock().Now()), + publicKey: pubKey, + checker: checker, + traits: accessInfo.Traits, + renewable: renewable, + includeHostCA: true, + loginIP: loginIP, + botName: botName, + joinAttributes: joinAttrs, } if existingInstanceID == "" {