From 6ab0981e890a2121ab2fdeb3b3a46cb429868f3f Mon Sep 17 00:00:00 2001 From: scabell Date: Fri, 23 Jan 2026 09:27:09 -0700 Subject: [PATCH 1/5] Implement client secret rotation --- client/client.go | 29 ++- client/client_secret_rotation_test.go | 45 ++++ client/handler.go | 106 +++++++++ client/secret_rotation_integration_test.go | 201 ++++++++++++++++++ ..._client_rotated_secrets.cockroach.down.sql | 1 + ...00_client_rotated_secrets.cockroach.up.sql | 1 + ...0909000000_client_rotated_secrets.down.sql | 1 + ...0000_client_rotated_secrets.mysql.down.sql | 1 + ...000000_client_rotated_secrets.mysql.up.sql | 1 + ...0_client_rotated_secrets.postgres.down.sql | 1 + ...000_client_rotated_secrets.postgres.up.sql | 1 + ...000_client_rotated_secrets.sqlite.down.sql | 1 + ...00000_client_rotated_secrets.sqlite.up.sql | 1 + ...110909000000_client_rotated_secrets.up.sql | 1 + persistence/sql/persister_client.go | 23 ++ 15 files changed, 412 insertions(+), 2 deletions(-) create mode 100644 client/client_secret_rotation_test.go create mode 100644 client/secret_rotation_integration_test.go create mode 100644 persistence/sql/migrations/20260122110909000000_client_rotated_secrets.cockroach.down.sql create mode 100644 persistence/sql/migrations/20260122110909000000_client_rotated_secrets.cockroach.up.sql create mode 100644 persistence/sql/migrations/20260122110909000000_client_rotated_secrets.down.sql create mode 100644 persistence/sql/migrations/20260122110909000000_client_rotated_secrets.mysql.down.sql create mode 100644 persistence/sql/migrations/20260122110909000000_client_rotated_secrets.mysql.up.sql create mode 100644 persistence/sql/migrations/20260122110909000000_client_rotated_secrets.postgres.down.sql create mode 100644 persistence/sql/migrations/20260122110909000000_client_rotated_secrets.postgres.up.sql create mode 100644 persistence/sql/migrations/20260122110909000000_client_rotated_secrets.sqlite.down.sql create mode 100644 persistence/sql/migrations/20260122110909000000_client_rotated_secrets.sqlite.up.sql create mode 100644 persistence/sql/migrations/20260122110909000000_client_rotated_secrets.up.sql diff --git a/client/client.go b/client/client.go index 4d04c899c58..a18152c202b 100644 --- a/client/client.go +++ b/client/client.go @@ -4,6 +4,7 @@ package client import ( + "encoding/json" "strconv" "strings" "time" @@ -21,8 +22,9 @@ import ( ) var ( - _ fosite.OpenIDConnectClient = (*Client)(nil) - _ fosite.Client = (*Client)(nil) + _ fosite.OpenIDConnectClient = (*Client)(nil) + _ fosite.Client = (*Client)(nil) + _ fosite.ClientWithSecretRotation = (*Client)(nil) ) // OAuth 2.0 Client @@ -51,6 +53,12 @@ type Client struct { // never again. The secret is kept in hashed format and is not recoverable once lost. Secret string `json:"client_secret,omitempty" db:"client_secret"` + // OAuth 2.0 Client Rotated Secrets + // + // RotatedSecrets holds previously rotated secrets that are still valid for authentication. + // This allows for secret rotation without downtime. Secrets are stored in hashed format. + RotatedSecrets string `json:"rotated_secrets,omitempty" db:"rotated_secrets"` + // OAuth 2.0 Client Redirect URIs // // RedirectURIs is an array of allowed redirect urls for the client. @@ -432,6 +440,23 @@ func (c *Client) GetHashedSecret() []byte { return []byte(c.Secret) } +func (c *Client) GetRotatedHashes() [][]byte { + if c.RotatedSecrets == "" { + return nil + } + + var rotated []string + if err := json.Unmarshal([]byte(c.RotatedSecrets), &rotated); err != nil { + return nil + } + + hashes := make([][]byte, len(rotated)) + for i, secret := range rotated { + hashes[i] = []byte(secret) + } + return hashes +} + func (c *Client) GetScopes() fosite.Arguments { return strings.Fields(c.Scope) } diff --git a/client/client_secret_rotation_test.go b/client/client_secret_rotation_test.go new file mode 100644 index 00000000000..02bb43ce444 --- /dev/null +++ b/client/client_secret_rotation_test.go @@ -0,0 +1,45 @@ +// Copyright © 2022 Ory Corp +// SPDX-License-Identifier: Apache-2.0 + +package client + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestClient_GetRotatedHashes(t *testing.T) { + t.Run("returns nil when no rotated secrets", func(t *testing.T) { + c := &Client{ + RotatedSecrets: "", + } + hashes := c.GetRotatedHashes() + assert.Nil(t, hashes) + }) + + t.Run("returns hashes when rotated secrets exist", func(t *testing.T) { + secrets := []string{"hash1", "hash2", "hash3"} + secretsJSON, err := json.Marshal(secrets) + require.NoError(t, err) + + c := &Client{ + RotatedSecrets: string(secretsJSON), + } + hashes := c.GetRotatedHashes() + require.Len(t, hashes, 3) + assert.Equal(t, []byte("hash1"), hashes[0]) + assert.Equal(t, []byte("hash2"), hashes[1]) + assert.Equal(t, []byte("hash3"), hashes[2]) + }) + + t.Run("returns nil on invalid JSON", func(t *testing.T) { + c := &Client{ + RotatedSecrets: "invalid json", + } + hashes := c.GetRotatedHashes() + assert.Nil(t, hashes) + }) +} diff --git a/client/handler.go b/client/handler.go index 2de4afb768f..c2f161079a2 100644 --- a/client/handler.go +++ b/client/handler.go @@ -47,6 +47,8 @@ func (h *Handler) SetAdminRoutes(r *httprouterx.RouterAdmin) { r.PATCH(ClientsHandlerPath+"/{id}", h.patchOAuth2Client) r.DELETE(ClientsHandlerPath+"/{id}", h.deleteOAuth2Client) r.PUT(ClientsHandlerPath+"/{id}/lifespans", h.setOAuth2ClientLifespans) + r.POST(ClientsHandlerPath+"/{id}/secret/rotate", h.rotateOAuth2ClientSecret) + r.DELETE(ClientsHandlerPath+"/{id}/secret/rotated", h.deleteRotatedOAuth2ClientSecrets) } func (h *Handler) SetPublicRoutes(r *httprouterx.RouterPublic) { @@ -850,3 +852,107 @@ func (h *Handler) requireDynamicAuth(r *http.Request) *herodot.DefaultError { } return nil } + +// Rotate OAuth2 Client Secret Parameters +// +// swagger:parameters rotateOAuth2ClientSecret +// +//lint:ignore U1000 Used to generate Swagger and OpenAPI definitions +type rotateOAuth2ClientSecret struct { + // OAuth 2.0 Client ID + // + // in: path + // required: true + ID string `json:"id"` +} + +// swagger:route POST /admin/clients/{id}/secret/rotate oAuth2 rotateOAuth2ClientSecret +// +// # Rotate OAuth 2.0 Client Secret +// +// Rotates the client secret for an OAuth 2.0 client. The old secret will be moved to the rotated_secrets +// array and will remain valid for authentication, allowing for zero-downtime secret rotation. +// A new secret will be generated and returned in the response. +// +// Produces: +// - application/json +// +// Schemes: http, https +// +// Responses: +// 200: oAuth2Client +// 404: errorOAuth2NotFound +// default: errorOAuth2Default +func (h *Handler) rotateOAuth2ClientSecret(w http.ResponseWriter, r *http.Request) { + id := r.PathValue("id") + c, err := h.r.ClientManager().GetConcreteClient(r.Context(), id) + if err != nil { + h.r.Writer().WriteError(w, r, err) + return + } + + secretb, err := x.GenerateSecret(26) + if err != nil { + h.r.Writer().WriteError(w, r, err) + return + } + newSecret := string(secretb) + c.Secret = newSecret + + if err := h.updateClient(r.Context(), c, h.r.ClientValidator().Validate); err != nil { + h.r.Writer().WriteError(w, r, err) + return + } + + c.Secret = newSecret + h.r.Writer().Write(w, r, c) +} + +// Delete Rotated OAuth2 Client Secrets Parameters +// +// swagger:parameters deleteRotatedOAuth2ClientSecrets +// +//lint:ignore U1000 Used to generate Swagger and OpenAPI definitions +type deleteRotatedOAuth2ClientSecrets struct { + // OAuth 2.0 Client ID + // + // in: path + // required: true + ID string `json:"id"` +} + +// swagger:route DELETE /admin/clients/{id}/secret/rotated oAuth2 deleteRotatedOAuth2ClientSecrets +// +// # Delete Rotated OAuth 2.0 Client Secrets +// +// Removes all rotated secrets from an OAuth 2.0 client. This should be called after all services +// have been updated to use the new secret and the old secrets are no longer needed. +// +// Produces: +// - application/json +// +// Schemes: http, https +// +// Responses: +// 200: oAuth2Client +// 404: errorOAuth2NotFound +// default: errorOAuth2Default +func (h *Handler) deleteRotatedOAuth2ClientSecrets(w http.ResponseWriter, r *http.Request) { + id := r.PathValue("id") + c, err := h.r.ClientManager().GetConcreteClient(r.Context(), id) + if err != nil { + h.r.Writer().WriteError(w, r, err) + return + } + + c.RotatedSecrets = "[]" + c.Secret = "" + + if err := h.updateClient(r.Context(), c, h.r.ClientValidator().Validate); err != nil { + h.r.Writer().WriteError(w, r, err) + return + } + + c.Secret = "" + h.r.Writer().Write(w, r, c) +} diff --git a/client/secret_rotation_integration_test.go b/client/secret_rotation_integration_test.go new file mode 100644 index 00000000000..fa9d31c407e --- /dev/null +++ b/client/secret_rotation_integration_test.go @@ -0,0 +1,201 @@ +// Copyright © 2022 Ory Corp +// SPDX-License-Identifier: Apache-2.0 + +package client_test + +import ( + "context" + "encoding/json" + "testing" + + "github.com/gofrs/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + goauth2 "golang.org/x/oauth2" + "golang.org/x/oauth2/clientcredentials" + + "github.com/ory/hydra/v2/client" + "github.com/ory/hydra/v2/internal/testhelpers" +) + +func TestSecretRotation_E2E(t *testing.T) { + ctx := context.Background() + reg := testhelpers.NewRegistryMemory(t) + public, _ := testhelpers.NewOAuth2Server(ctx, t, reg) + + t.Run("case=complete_secret_rotation_workflow", func(t *testing.T) { + // Step 1: Create a client with a known secret + originalSecret := uuid.Must(uuid.NewV4()).String() + c := &client.Client{ + Secret: originalSecret, + RedirectURIs: []string{public.URL + "/callback"}, + ResponseTypes: []string{"token"}, + GrantTypes: []string{"client_credentials"}, + Scope: "foobar", + } + require.NoError(t, reg.ClientManager().CreateClient(ctx, c)) + clientID := c.GetID() + + conf := clientcredentials.Config{ + ClientID: clientID, + ClientSecret: originalSecret, + TokenURL: reg.Config().OAuth2TokenURL(ctx).String(), + AuthStyle: goauth2.AuthStyleInHeader, + } + + // Step 2: Verify original secret works for authentication + t.Run("step=authenticate_with_original_secret", func(t *testing.T) { + token, err := conf.Token(ctx) + require.NoError(t, err) + assert.NotEmpty(t, token.AccessToken) + }) + + // Step 3: Rotate the secret + newSecret := uuid.Must(uuid.NewV4()).String() + t.Run("step=rotate_secret", func(t *testing.T) { + c, err := reg.ClientManager().GetConcreteClient(ctx, clientID) + require.NoError(t, err) + + c.Secret = newSecret + err = reg.ClientManager().UpdateClient(ctx, c) + require.NoError(t, err) + }) + + // Step 4: Verify NEW secret works + t.Run("step=authenticate_with_new_secret", func(t *testing.T) { + newConf := clientcredentials.Config{ + ClientID: clientID, + ClientSecret: newSecret, + TokenURL: reg.Config().OAuth2TokenURL(ctx).String(), + AuthStyle: goauth2.AuthStyleInHeader, + } + token, err := newConf.Token(ctx) + require.NoError(t, err) + assert.NotEmpty(t, token.AccessToken) + }) + + // Step 5: Verify OLD secret STILL works (key test for rotation) + t.Run("step=authenticate_with_old_secret_after_rotation", func(t *testing.T) { + token, err := conf.Token(ctx) + require.NoError(t, err, "Old secret should still work after rotation") + assert.NotEmpty(t, token.AccessToken) + }) + + // Step 6: Verify rotated secrets are stored + t.Run("step=verify_rotated_secrets_stored", func(t *testing.T) { + c, err := reg.ClientManager().GetConcreteClient(ctx, clientID) + require.NoError(t, err) + + assert.NotEmpty(t, c.RotatedSecrets, "Rotated secrets should be stored") + + var rotated []string + err = json.Unmarshal([]byte(c.RotatedSecrets), &rotated) + require.NoError(t, err) + assert.Len(t, rotated, 1, "Should have one rotated secret") + }) + + // Step 7: Clear rotated secrets + t.Run("step=clear_rotated_secrets", func(t *testing.T) { + c, err := reg.ClientManager().GetConcreteClient(ctx, clientID) + require.NoError(t, err) + + c.RotatedSecrets = "[]" + c.Secret = "" + err = reg.ClientManager().UpdateClient(ctx, c) + require.NoError(t, err) + + c, err = reg.ClientManager().GetConcreteClient(ctx, clientID) + require.NoError(t, err) + assert.Equal(t, "[]", c.RotatedSecrets) + }) + + // Step 8: Verify OLD secret no longer works after cleanup + t.Run("step=old_secret_fails_after_cleanup", func(t *testing.T) { + _, err := conf.Token(ctx) + require.Error(t, err, "Old secret should not work after cleanup") + }) + + // Step 9: Verify NEW secret still works after cleanup + t.Run("step=new_secret_works_after_cleanup", func(t *testing.T) { + newConf := clientcredentials.Config{ + ClientID: clientID, + ClientSecret: newSecret, + TokenURL: reg.Config().OAuth2TokenURL(ctx).String(), + AuthStyle: goauth2.AuthStyleInHeader, + } + token, err := newConf.Token(ctx) + require.NoError(t, err) + assert.NotEmpty(t, token.AccessToken) + }) + }) + + t.Run("case=multiple_rotations", func(t *testing.T) { + // Create client with first secret + secret1 := uuid.Must(uuid.NewV4()).String() + c := &client.Client{ + Secret: secret1, + RedirectURIs: []string{public.URL + "/callback"}, + ResponseTypes: []string{"token"}, + GrantTypes: []string{"client_credentials"}, + Scope: "foobar", + } + require.NoError(t, reg.ClientManager().CreateClient(ctx, c)) + clientID := c.GetID() + + // Rotate to secret2 + secret2 := uuid.Must(uuid.NewV4()).String() + c, err := reg.ClientManager().GetConcreteClient(ctx, clientID) + require.NoError(t, err) + c.Secret = secret2 + err = reg.ClientManager().UpdateClient(ctx, c) + require.NoError(t, err) + + // Rotate to secret3 + secret3 := uuid.Must(uuid.NewV4()).String() + c, err = reg.ClientManager().GetConcreteClient(ctx, clientID) + require.NoError(t, err) + c.Secret = secret3 + err = reg.ClientManager().UpdateClient(ctx, c) + require.NoError(t, err) + + // Verify rotated secrets array has 2 entries + c, err = reg.ClientManager().GetConcreteClient(ctx, clientID) + require.NoError(t, err) + + var rotated []string + err = json.Unmarshal([]byte(c.RotatedSecrets), &rotated) + require.NoError(t, err) + assert.Len(t, rotated, 2, "Should have two rotated secrets") + + // Test all three secrets work + conf1 := clientcredentials.Config{ + ClientID: clientID, + ClientSecret: secret1, + TokenURL: reg.Config().OAuth2TokenURL(ctx).String(), + AuthStyle: goauth2.AuthStyleInHeader, + } + token1, err := conf1.Token(ctx) + require.NoError(t, err) + assert.NotEmpty(t, token1.AccessToken) + + conf2 := clientcredentials.Config{ + ClientID: clientID, + ClientSecret: secret2, + TokenURL: reg.Config().OAuth2TokenURL(ctx).String(), + AuthStyle: goauth2.AuthStyleInHeader, + } + token2, err := conf2.Token(ctx) + require.NoError(t, err) + assert.NotEmpty(t, token2.AccessToken) + + conf3 := clientcredentials.Config{ + ClientID: clientID, + ClientSecret: secret3, + TokenURL: reg.Config().OAuth2TokenURL(ctx).String(), + AuthStyle: goauth2.AuthStyleInHeader, + } + token3, err := conf3.Token(ctx) + require.NoError(t, err) + assert.NotEmpty(t, token3.AccessToken) + }) +} diff --git a/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.cockroach.down.sql b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.cockroach.down.sql new file mode 100644 index 00000000000..d0a25e6152a --- /dev/null +++ b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.cockroach.down.sql @@ -0,0 +1 @@ +ALTER TABLE hydra_client DROP COLUMN rotated_secrets; diff --git a/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.cockroach.up.sql b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.cockroach.up.sql new file mode 100644 index 00000000000..87700334af9 --- /dev/null +++ b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.cockroach.up.sql @@ -0,0 +1 @@ +ALTER TABLE hydra_client ADD COLUMN rotated_secrets TEXT NOT NULL DEFAULT ''; diff --git a/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.down.sql b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.down.sql new file mode 100644 index 00000000000..d0a25e6152a --- /dev/null +++ b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.down.sql @@ -0,0 +1 @@ +ALTER TABLE hydra_client DROP COLUMN rotated_secrets; diff --git a/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.mysql.down.sql b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.mysql.down.sql new file mode 100644 index 00000000000..d0a25e6152a --- /dev/null +++ b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.mysql.down.sql @@ -0,0 +1 @@ +ALTER TABLE hydra_client DROP COLUMN rotated_secrets; diff --git a/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.mysql.up.sql b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.mysql.up.sql new file mode 100644 index 00000000000..d249fc6243e --- /dev/null +++ b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.mysql.up.sql @@ -0,0 +1 @@ +ALTER TABLE hydra_client ADD rotated_secrets TEXT NOT NULL; diff --git a/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.postgres.down.sql b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.postgres.down.sql new file mode 100644 index 00000000000..d0a25e6152a --- /dev/null +++ b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.postgres.down.sql @@ -0,0 +1 @@ +ALTER TABLE hydra_client DROP COLUMN rotated_secrets; diff --git a/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.postgres.up.sql b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.postgres.up.sql new file mode 100644 index 00000000000..87700334af9 --- /dev/null +++ b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.postgres.up.sql @@ -0,0 +1 @@ +ALTER TABLE hydra_client ADD COLUMN rotated_secrets TEXT NOT NULL DEFAULT ''; diff --git a/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.sqlite.down.sql b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.sqlite.down.sql new file mode 100644 index 00000000000..d0a25e6152a --- /dev/null +++ b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.sqlite.down.sql @@ -0,0 +1 @@ +ALTER TABLE hydra_client DROP COLUMN rotated_secrets; diff --git a/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.sqlite.up.sql b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.sqlite.up.sql new file mode 100644 index 00000000000..c72f29d0d71 --- /dev/null +++ b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.sqlite.up.sql @@ -0,0 +1 @@ +ALTER TABLE hydra_client ADD rotated_secrets TEXT NOT NULL DEFAULT ''; diff --git a/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.up.sql b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.up.sql new file mode 100644 index 00000000000..c72f29d0d71 --- /dev/null +++ b/persistence/sql/migrations/20260122110909000000_client_rotated_secrets.up.sql @@ -0,0 +1 @@ +ALTER TABLE hydra_client ADD rotated_secrets TEXT NOT NULL DEFAULT ''; diff --git a/persistence/sql/persister_client.go b/persistence/sql/persister_client.go index 24dd672aa86..23f84c52be2 100644 --- a/persistence/sql/persister_client.go +++ b/persistence/sql/persister_client.go @@ -5,6 +5,7 @@ package sql import ( "context" + "encoding/json" "github.com/gofrs/uuid" "go.opentelemetry.io/otel/trace" @@ -77,11 +78,33 @@ func (p *Persister) UpdateClient(ctx context.Context, cl *client.Client) (err er if cl.Secret == "" { cl.Secret = string(o.GetHashedSecret()) + if cl.RotatedSecrets == "" { + cl.RotatedSecrets = o.RotatedSecrets + } } else { h, err := p.r.ClientHasher().Hash(ctx, []byte(cl.Secret)) if err != nil { return err } + + oldSecret := string(o.GetHashedSecret()) + if oldSecret != "" && oldSecret != string(h) { + var rotated []string + if o.RotatedSecrets != "" { + if err := json.Unmarshal([]byte(o.RotatedSecrets), &rotated); err != nil { + rotated = []string{} + } + } + rotated = append(rotated, oldSecret) + rotatedJSON, err := json.Marshal(rotated) + if err != nil { + return err + } + cl.RotatedSecrets = string(rotatedJSON) + } else { + cl.RotatedSecrets = o.RotatedSecrets + } + cl.Secret = string(h) } From e1a119317716434b73ca4937b70585ef375b63f0 Mon Sep 17 00:00:00 2001 From: scabell Date: Fri, 23 Jan 2026 09:59:46 -0700 Subject: [PATCH 2/5] run formatter --- internal/testhelpers/sql_schemas/cockroach_dump.sql | 3 ++- internal/testhelpers/sql_schemas/mysql_dump.sql | 3 ++- internal/testhelpers/sql_schemas/postgres_dump.sql | 5 +++-- internal/testhelpers/sql_schemas/sqlite3_dump.sql | 4 ++-- oryx/httpx/resilient_client.go | 3 ++- oryx/tlsx/termination.go | 3 ++- 6 files changed, 13 insertions(+), 8 deletions(-) diff --git a/internal/testhelpers/sql_schemas/cockroach_dump.sql b/internal/testhelpers/sql_schemas/cockroach_dump.sql index a8a7dfbd493..d144abf4c24 100644 --- a/internal/testhelpers/sql_schemas/cockroach_dump.sql +++ b/internal/testhelpers/sql_schemas/cockroach_dump.sql @@ -1,4 +1,4 @@ --- migrations hash: 55b905a5cead652db5b4a5e317d60ad18bd9e6de61d4b43edcdb79244947fbf9b106783f10765f2ffa4d7952c745900c2406f522a101dbed17e5785396484e41 +-- migrations hash: 60e5750fe86fae4e22072fccb11f8a65bc6f434f595b6db62091a30af66e7dc942ab810b1b22aa837e3a357a3fcced7d7447bdab9bb280dca67fcff7353effcd CREATE TABLE public.schema_migration ( version VARCHAR(48) NOT NULL, @@ -70,6 +70,7 @@ CREATE TABLE public.hydra_client ( device_authorization_grant_id_token_lifespan INT8 NULL, device_authorization_grant_access_token_lifespan INT8 NULL, device_authorization_grant_refresh_token_lifespan INT8 NULL, + rotated_secrets STRING NOT NULL DEFAULT '':::STRING, CONSTRAINT hydra_client_pkey PRIMARY KEY (id ASC, nid ASC), UNIQUE INDEX hydra_client_id_key (id ASC, nid ASC), UNIQUE INDEX hydra_client_pk_key (pk ASC) diff --git a/internal/testhelpers/sql_schemas/mysql_dump.sql b/internal/testhelpers/sql_schemas/mysql_dump.sql index 46a6b435da4..a201085e63c 100644 --- a/internal/testhelpers/sql_schemas/mysql_dump.sql +++ b/internal/testhelpers/sql_schemas/mysql_dump.sql @@ -1,4 +1,4 @@ --- migrations hash: 55b905a5cead652db5b4a5e317d60ad18bd9e6de61d4b43edcdb79244947fbf9b106783f10765f2ffa4d7952c745900c2406f522a101dbed17e5785396484e41 +-- migrations hash: 60e5750fe86fae4e22072fccb11f8a65bc6f434f595b6db62091a30af66e7dc942ab810b1b22aa837e3a357a3fcced7d7447bdab9bb280dca67fcff7353effcd /*!40101 SET @OLD_CHARACTER_SET_CLIENT=@@CHARACTER_SET_CLIENT */; @@ -71,6 +71,7 @@ CREATE TABLE `hydra_client` ( `device_authorization_grant_id_token_lifespan` bigint DEFAULT NULL, `device_authorization_grant_access_token_lifespan` bigint DEFAULT NULL, `device_authorization_grant_refresh_token_lifespan` bigint DEFAULT NULL, + `rotated_secrets` text NOT NULL, PRIMARY KEY (`id`,`nid`), UNIQUE KEY `hydra_client_id_key` (`id`,`nid`), KEY `pk_deprecated` (`pk_deprecated`), diff --git a/internal/testhelpers/sql_schemas/postgres_dump.sql b/internal/testhelpers/sql_schemas/postgres_dump.sql index 071e0691a35..c38f4d389dd 100644 --- a/internal/testhelpers/sql_schemas/postgres_dump.sql +++ b/internal/testhelpers/sql_schemas/postgres_dump.sql @@ -1,4 +1,4 @@ --- migrations hash: 55b905a5cead652db5b4a5e317d60ad18bd9e6de61d4b43edcdb79244947fbf9b106783f10765f2ffa4d7952c745900c2406f522a101dbed17e5785396484e41 +-- migrations hash: 60e5750fe86fae4e22072fccb11f8a65bc6f434f595b6db62091a30af66e7dc942ab810b1b22aa837e3a357a3fcced7d7447bdab9bb280dca67fcff7353effcd @@ -76,7 +76,8 @@ CREATE TABLE public.hydra_client ( skip_logout_consent boolean, device_authorization_grant_id_token_lifespan bigint, device_authorization_grant_access_token_lifespan bigint, - device_authorization_grant_refresh_token_lifespan bigint + device_authorization_grant_refresh_token_lifespan bigint, + rotated_secrets text DEFAULT ''::text NOT NULL ); ALTER TABLE public.hydra_client OWNER TO postgres; diff --git a/internal/testhelpers/sql_schemas/sqlite3_dump.sql b/internal/testhelpers/sql_schemas/sqlite3_dump.sql index 76af9d2d5ab..2988c14dccc 100644 --- a/internal/testhelpers/sql_schemas/sqlite3_dump.sql +++ b/internal/testhelpers/sql_schemas/sqlite3_dump.sql @@ -1,4 +1,4 @@ --- migrations hash: 55b905a5cead652db5b4a5e317d60ad18bd9e6de61d4b43edcdb79244947fbf9b106783f10765f2ffa4d7952c745900c2406f522a101dbed17e5785396484e41 +-- migrations hash: 60e5750fe86fae4e22072fccb11f8a65bc6f434f595b6db62091a30af66e7dc942ab810b1b22aa837e3a357a3fcced7d7447bdab9bb280dca67fcff7353effcd CREATE TABLE "hydra_client" ( @@ -52,7 +52,7 @@ CREATE TABLE "hydra_client" refresh_token_grant_access_token_lifespan BIGINT NULL DEFAULT NULL, refresh_token_grant_refresh_token_lifespan BIGINT NULL DEFAULT NULL, skip_consent BOOLEAN NOT NULL DEFAULT false, - nid CHAR(36) NOT NULL, skip_logout_consent BOOLEAN NULL, device_authorization_grant_id_token_lifespan BIGINT NULL DEFAULT NULL, device_authorization_grant_access_token_lifespan BIGINT NULL DEFAULT NULL, device_authorization_grant_refresh_token_lifespan BIGINT NULL DEFAULT NULL, + nid CHAR(36) NOT NULL, skip_logout_consent BOOLEAN NULL, device_authorization_grant_id_token_lifespan BIGINT NULL DEFAULT NULL, device_authorization_grant_access_token_lifespan BIGINT NULL DEFAULT NULL, device_authorization_grant_refresh_token_lifespan BIGINT NULL DEFAULT NULL, rotated_secrets TEXT NOT NULL DEFAULT '', PRIMARY KEY (id, nid) ); CREATE TABLE "hydra_jwk" ( diff --git a/oryx/httpx/resilient_client.go b/oryx/httpx/resilient_client.go index 47f287675bc..a78bbbf865c 100644 --- a/oryx/httpx/resilient_client.go +++ b/oryx/httpx/resilient_client.go @@ -10,9 +10,10 @@ import ( "net/http" "time" - "github.com/ory/herodot" "golang.org/x/oauth2" + "github.com/ory/herodot" + "github.com/hashicorp/go-retryablehttp" "github.com/ory/x/logrusx" diff --git a/oryx/tlsx/termination.go b/oryx/tlsx/termination.go index 790d9f267dd..61b42cbc85d 100644 --- a/oryx/tlsx/termination.go +++ b/oryx/tlsx/termination.go @@ -8,10 +8,11 @@ import ( "net/http" "strings" - "github.com/ory/x/httpx" "github.com/pkg/errors" "github.com/urfave/negroni" + "github.com/ory/x/httpx" + "github.com/ory/x/healthx" "github.com/ory/x/logrusx" "github.com/ory/x/prometheusx" From 170365587e267ca5768295511e8e620dfc02ce89 Mon Sep 17 00:00:00 2001 From: scabell Date: Fri, 23 Jan 2026 10:03:52 -0700 Subject: [PATCH 3/5] change delete rotated route --- client/handler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/handler.go b/client/handler.go index c2f161079a2..ab0057d3507 100644 --- a/client/handler.go +++ b/client/handler.go @@ -48,7 +48,7 @@ func (h *Handler) SetAdminRoutes(r *httprouterx.RouterAdmin) { r.DELETE(ClientsHandlerPath+"/{id}", h.deleteOAuth2Client) r.PUT(ClientsHandlerPath+"/{id}/lifespans", h.setOAuth2ClientLifespans) r.POST(ClientsHandlerPath+"/{id}/secret/rotate", h.rotateOAuth2ClientSecret) - r.DELETE(ClientsHandlerPath+"/{id}/secret/rotated", h.deleteRotatedOAuth2ClientSecrets) + r.DELETE(ClientsHandlerPath+"/{id}/secret/rotate", h.deleteRotatedOAuth2ClientSecrets) } func (h *Handler) SetPublicRoutes(r *httprouterx.RouterPublic) { From 0720c3597495c0541e62c4b5137b85c6fa5949de Mon Sep 17 00:00:00 2001 From: scabell Date: Fri, 23 Jan 2026 10:33:01 -0700 Subject: [PATCH 4/5] add new reotated secret field to test fixtures --- client/handler_test.go | 2 +- .../sql/migratest/fixtures/hydra_client/client-0001.json | 1 + .../sql/migratest/fixtures/hydra_client/client-0002.json | 1 + .../sql/migratest/fixtures/hydra_client/client-0003.json | 1 + .../sql/migratest/fixtures/hydra_client/client-0004.json | 1 + .../sql/migratest/fixtures/hydra_client/client-0005.json | 1 + .../sql/migratest/fixtures/hydra_client/client-0006.json | 1 + .../sql/migratest/fixtures/hydra_client/client-0007.json | 1 + .../sql/migratest/fixtures/hydra_client/client-0008.json | 1 + .../sql/migratest/fixtures/hydra_client/client-0009.json | 1 + .../sql/migratest/fixtures/hydra_client/client-0010.json | 1 + .../sql/migratest/fixtures/hydra_client/client-0011.json | 1 + .../sql/migratest/fixtures/hydra_client/client-0012.json | 1 + .../sql/migratest/fixtures/hydra_client/client-0013.json | 1 + .../sql/migratest/fixtures/hydra_client/client-0014.json | 1 + .../sql/migratest/fixtures/hydra_client/client-0015.json | 1 + persistence/sql/migratest/fixtures/hydra_client/client-20.json | 1 + .../sql/migratest/fixtures/hydra_client/client-2005.json | 1 + persistence/sql/migratest/fixtures/hydra_client/client-21.json | 1 + persistence/sql/migratest/fixtures/hydra_client/client-22.json | 1 + persistence/sql/migratest/fixtures/hydra_client/client-23.json | 1 + 21 files changed, 21 insertions(+), 1 deletion(-) diff --git a/client/handler_test.go b/client/handler_test.go index 9a3d763176a..db4ae7d9b1b 100644 --- a/client/handler_test.go +++ b/client/handler_test.go @@ -561,7 +561,7 @@ func TestHandler(t *testing.T) { payload, _ := sjson.Set(expected, "redirect_uris", []string{"http://localhost:3000/cb", "https://foobar.com"}) body, res := makeJSON(t, adminTs, "PUT", urlx.MustJoin(client.ClientsHandlerPath, url.PathEscape(expectedID)), json.RawMessage(payload)) assert.Equal(t, http.StatusOK, res.StatusCode) - snapshotx.SnapshotT(t, newResponseSnapshot(body, res), snapshotx.ExceptPaths("body.created_at", "body.updated_at", "body.client_id", "body.registration_client_uri", "body.registration_access_token")) + snapshotx.SnapshotT(t, newResponseSnapshot(body, res), snapshotx.ExceptPaths("body.created_at", "body.updated_at", "body.client_id", "body.registration_client_uri", "body.registration_access_token", "body.rotated_secrets")) }) t.Run("endpoint=dynamic client registration", func(t *testing.T) { diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-0001.json b/persistence/sql/migratest/fixtures/hydra_client/client-0001.json index ab2abe3cb72..830a0f31be2 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-0001.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-0001.json @@ -99,6 +99,7 @@ "ResponseTypes": [ "response-0001_1" ], + "RotatedSecrets": "", "Scope": "scope-0001", "Secret": "secret-0001", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-0002.json b/persistence/sql/migratest/fixtures/hydra_client/client-0002.json index cabd794188b..91683363a67 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-0002.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-0002.json @@ -99,6 +99,7 @@ "ResponseTypes": [ "response-0002_1" ], + "RotatedSecrets": "", "Scope": "scope-0002", "Secret": "secret-0002", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-0003.json b/persistence/sql/migratest/fixtures/hydra_client/client-0003.json index 2b2abbddb1d..f7d4db2d9bd 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-0003.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-0003.json @@ -99,6 +99,7 @@ "ResponseTypes": [ "response-0003_1" ], + "RotatedSecrets": "", "Scope": "scope-0003", "Secret": "secret-0003", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-0004.json b/persistence/sql/migratest/fixtures/hydra_client/client-0004.json index 6e8168cd547..993d7eedbd5 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-0004.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-0004.json @@ -101,6 +101,7 @@ "ResponseTypes": [ "response-0004_1" ], + "RotatedSecrets": "", "Scope": "scope-0004", "Secret": "secret-0004", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-0005.json b/persistence/sql/migratest/fixtures/hydra_client/client-0005.json index 95a12e2de6b..34bef2965e6 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-0005.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-0005.json @@ -101,6 +101,7 @@ "ResponseTypes": [ "response-0005_1" ], + "RotatedSecrets": "", "Scope": "scope-0005", "Secret": "secret-0005", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-0006.json b/persistence/sql/migratest/fixtures/hydra_client/client-0006.json index b95628a9cd1..2999b9055c4 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-0006.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-0006.json @@ -101,6 +101,7 @@ "ResponseTypes": [ "response-0006_1" ], + "RotatedSecrets": "", "Scope": "scope-0006", "Secret": "secret-0006", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-0007.json b/persistence/sql/migratest/fixtures/hydra_client/client-0007.json index bd274e5f843..137a1c6323b 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-0007.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-0007.json @@ -101,6 +101,7 @@ "ResponseTypes": [ "response-0007_1" ], + "RotatedSecrets": "", "Scope": "scope-0007", "Secret": "secret-0007", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-0008.json b/persistence/sql/migratest/fixtures/hydra_client/client-0008.json index 8f069fe472e..06c6f3106d3 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-0008.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-0008.json @@ -103,6 +103,7 @@ "ResponseTypes": [ "response-0008_1" ], + "RotatedSecrets": "", "Scope": "scope-0008", "Secret": "secret-0008", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-0009.json b/persistence/sql/migratest/fixtures/hydra_client/client-0009.json index 2e85ab549b7..c463e856b96 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-0009.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-0009.json @@ -103,6 +103,7 @@ "ResponseTypes": [ "response-0009_1" ], + "RotatedSecrets": "", "Scope": "scope-0009", "Secret": "secret-0009", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-0010.json b/persistence/sql/migratest/fixtures/hydra_client/client-0010.json index f060a6f6337..9ae8b9a9b46 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-0010.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-0010.json @@ -103,6 +103,7 @@ "ResponseTypes": [ "response-0010_1" ], + "RotatedSecrets": "", "Scope": "scope-0010", "Secret": "secret-0010", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-0011.json b/persistence/sql/migratest/fixtures/hydra_client/client-0011.json index 44ec40aea43..8569dcfc568 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-0011.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-0011.json @@ -105,6 +105,7 @@ "ResponseTypes": [ "response-0011_1" ], + "RotatedSecrets": "", "Scope": "scope-0011", "Secret": "secret-0011", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-0012.json b/persistence/sql/migratest/fixtures/hydra_client/client-0012.json index fbd6313598f..8600cf04a3f 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-0012.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-0012.json @@ -105,6 +105,7 @@ "ResponseTypes": [ "response-0012_1" ], + "RotatedSecrets": "", "Scope": "scope-0012", "Secret": "secret-0012", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-0013.json b/persistence/sql/migratest/fixtures/hydra_client/client-0013.json index 2148a8ec4f6..f7d2be8a0d7 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-0013.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-0013.json @@ -107,6 +107,7 @@ "ResponseTypes": [ "response-0013_1" ], + "RotatedSecrets": "", "Scope": "scope-0013", "Secret": "secret-0013", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-0014.json b/persistence/sql/migratest/fixtures/hydra_client/client-0014.json index 94003d09e18..8feb5b66c6f 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-0014.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-0014.json @@ -109,6 +109,7 @@ "ResponseTypes": [ "response-0014_1" ], + "RotatedSecrets": "", "Scope": "scope-0014", "Secret": "secret-0014", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-0015.json b/persistence/sql/migratest/fixtures/hydra_client/client-0015.json index 5f28970a4d4..57bb32ee7c0 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-0015.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-0015.json @@ -109,6 +109,7 @@ "ResponseTypes": [ "response-0015_1" ], + "RotatedSecrets": "", "Scope": "scope-0015", "Secret": "secret-0015", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-20.json b/persistence/sql/migratest/fixtures/hydra_client/client-20.json index 3ef897475cd..15a921ca542 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-20.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-20.json @@ -109,6 +109,7 @@ "ResponseTypes": [ "response-20_1" ], + "RotatedSecrets": "", "Scope": "scope-20", "Secret": "secret-20", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-2005.json b/persistence/sql/migratest/fixtures/hydra_client/client-2005.json index ffad8036db3..3b72487b972 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-2005.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-2005.json @@ -109,6 +109,7 @@ "ResponseTypes": [ "response-2005_1" ], + "RotatedSecrets": "", "Scope": "scope-2005", "Secret": "secret-2005", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-21.json b/persistence/sql/migratest/fixtures/hydra_client/client-21.json index bb97eae4d17..96e0bf991e6 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-21.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-21.json @@ -117,6 +117,7 @@ "response-21_1", "response-21_2" ], + "RotatedSecrets": "", "Scope": "scope-21", "Secret": "secret-21", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-22.json b/persistence/sql/migratest/fixtures/hydra_client/client-22.json index 6030e003246..98dbebb0081 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-22.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-22.json @@ -117,6 +117,7 @@ "response-22_1", "response-22_2" ], + "RotatedSecrets": "", "Scope": "scope-22", "Secret": "secret-22", "SecretExpiresAt": 0, diff --git a/persistence/sql/migratest/fixtures/hydra_client/client-23.json b/persistence/sql/migratest/fixtures/hydra_client/client-23.json index ed9e225ea88..bbdb4713e3e 100644 --- a/persistence/sql/migratest/fixtures/hydra_client/client-23.json +++ b/persistence/sql/migratest/fixtures/hydra_client/client-23.json @@ -117,6 +117,7 @@ "response-23_1", "response-23_2" ], + "RotatedSecrets": "", "Scope": "scope-23", "Secret": "secret-23", "SecretExpiresAt": 0, From 2a6f7f3812cf512ac2b826b37e6342043dba9738 Mon Sep 17 00:00:00 2001 From: scabell Date: Fri, 23 Jan 2026 11:13:37 -0700 Subject: [PATCH 5/5] prevent update client from rotating secret --- client/handler.go | 18 +++++++++ client/secret_rotation_integration_test.go | 47 ++++++++++++++++++++++ persistence/sql/persister_client.go | 26 ++++-------- 3 files changed, 72 insertions(+), 19 deletions(-) diff --git a/client/handler.go b/client/handler.go index ab0057d3507..358359b3484 100644 --- a/client/handler.go +++ b/client/handler.go @@ -891,6 +891,24 @@ func (h *Handler) rotateOAuth2ClientSecret(w http.ResponseWriter, r *http.Reques return } + // Move current secret to rotated secrets + oldSecret := string(c.GetHashedSecret()) + if oldSecret != "" { + var rotated []string + if c.RotatedSecrets != "" { + if err := json.Unmarshal([]byte(c.RotatedSecrets), &rotated); err != nil { + rotated = []string{} + } + } + rotated = append(rotated, oldSecret) + rotatedJSON, err := json.Marshal(rotated) + if err != nil { + h.r.Writer().WriteError(w, r, err) + return + } + c.RotatedSecrets = string(rotatedJSON) + } + secretb, err := x.GenerateSecret(26) if err != nil { h.r.Writer().WriteError(w, r, err) diff --git a/client/secret_rotation_integration_test.go b/client/secret_rotation_integration_test.go index fa9d31c407e..f47af79022d 100644 --- a/client/secret_rotation_integration_test.go +++ b/client/secret_rotation_integration_test.go @@ -56,6 +56,21 @@ func TestSecretRotation_E2E(t *testing.T) { c, err := reg.ClientManager().GetConcreteClient(ctx, clientID) require.NoError(t, err) + // Move current secret to rotated secrets (simulating what the rotate endpoint does) + oldSecret := string(c.GetHashedSecret()) + if oldSecret != "" { + var rotated []string + if c.RotatedSecrets != "" { + if err := json.Unmarshal([]byte(c.RotatedSecrets), &rotated); err != nil { + rotated = []string{} + } + } + rotated = append(rotated, oldSecret) + rotatedJSON, err := json.Marshal(rotated) + require.NoError(t, err) + c.RotatedSecrets = string(rotatedJSON) + } + c.Secret = newSecret err = reg.ClientManager().UpdateClient(ctx, c) require.NoError(t, err) @@ -146,6 +161,22 @@ func TestSecretRotation_E2E(t *testing.T) { secret2 := uuid.Must(uuid.NewV4()).String() c, err := reg.ClientManager().GetConcreteClient(ctx, clientID) require.NoError(t, err) + + // Move current secret to rotated secrets + oldSecret := string(c.GetHashedSecret()) + if oldSecret != "" { + var rotated []string + if c.RotatedSecrets != "" { + if err := json.Unmarshal([]byte(c.RotatedSecrets), &rotated); err != nil { + rotated = []string{} + } + } + rotated = append(rotated, oldSecret) + rotatedJSON, err := json.Marshal(rotated) + require.NoError(t, err) + c.RotatedSecrets = string(rotatedJSON) + } + c.Secret = secret2 err = reg.ClientManager().UpdateClient(ctx, c) require.NoError(t, err) @@ -154,6 +185,22 @@ func TestSecretRotation_E2E(t *testing.T) { secret3 := uuid.Must(uuid.NewV4()).String() c, err = reg.ClientManager().GetConcreteClient(ctx, clientID) require.NoError(t, err) + + // Move current secret to rotated secrets + oldSecret = string(c.GetHashedSecret()) + if oldSecret != "" { + var rotated []string + if c.RotatedSecrets != "" { + if err := json.Unmarshal([]byte(c.RotatedSecrets), &rotated); err != nil { + rotated = []string{} + } + } + rotated = append(rotated, oldSecret) + rotatedJSON, err := json.Marshal(rotated) + require.NoError(t, err) + c.RotatedSecrets = string(rotatedJSON) + } + c.Secret = secret3 err = reg.ClientManager().UpdateClient(ctx, c) require.NoError(t, err) diff --git a/persistence/sql/persister_client.go b/persistence/sql/persister_client.go index 23f84c52be2..479f46734cf 100644 --- a/persistence/sql/persister_client.go +++ b/persistence/sql/persister_client.go @@ -5,7 +5,6 @@ package sql import ( "context" - "encoding/json" "github.com/gofrs/uuid" "go.opentelemetry.io/otel/trace" @@ -77,35 +76,24 @@ func (p *Persister) UpdateClient(ctx context.Context, cl *client.Client) (err er } if cl.Secret == "" { + // Secret not being changed, preserve it and rotated secrets cl.Secret = string(o.GetHashedSecret()) if cl.RotatedSecrets == "" { cl.RotatedSecrets = o.RotatedSecrets } } else { + // New secret provided - hash it and clear rotated secrets h, err := p.r.ClientHasher().Hash(ctx, []byte(cl.Secret)) if err != nil { return err } - oldSecret := string(o.GetHashedSecret()) - if oldSecret != "" && oldSecret != string(h) { - var rotated []string - if o.RotatedSecrets != "" { - if err := json.Unmarshal([]byte(o.RotatedSecrets), &rotated); err != nil { - rotated = []string{} - } - } - rotated = append(rotated, oldSecret) - rotatedJSON, err := json.Marshal(rotated) - if err != nil { - return err - } - cl.RotatedSecrets = string(rotatedJSON) - } else { - cl.RotatedSecrets = o.RotatedSecrets - } - cl.Secret = string(h) + // Clear rotated secrets when secret is replaced (not rotated) + // Only preserve if explicitly set by the rotate endpoint + if cl.RotatedSecrets == "" { + cl.RotatedSecrets = "" + } } // Ensure ID is the same