diff --git a/app/models/identity/access_token.rb b/app/models/identity/access_token.rb index a02f2fcea..d9ab0db63 100644 --- a/app/models/identity/access_token.rb +++ b/app/models/identity/access_token.rb @@ -51,13 +51,6 @@ class Identity::AccessToken < ApplicationRecord # Tokens are stored only as digests (TokenDigester). The raw values exist # only in memory, on the record that issued or rotated them, to be handed to # their holder once. - # - # The plaintext `token` and `refresh_token` columns are ignored until they're - # dropped. Without partial_inserts, a model that knows a column names it on - # every INSERT, so only code that ignores it can keep creating tokens once - # it's gone. - self.ignored_columns += %w[ token refresh_token ] - attribute :raw_token, :string attribute :raw_refresh_token, :string diff --git a/db/migrate/20261017130000_drop_identity_access_token_plaintext.rb b/db/migrate/20261017130000_drop_identity_access_token_plaintext.rb new file mode 100644 index 000000000..0dbee629e --- /dev/null +++ b/db/migrate/20261017130000_drop_identity_access_token_plaintext.rb @@ -0,0 +1,37 @@ +# Drops the plaintext personal access tokens that BackfillTokenDigests and +# BackfillRemainingTokenDigests left beside their digests, and the plaintext +# refresh token column, empty since BackfillTokenDigests. +# +# Run it only once every process runs the code that shipped with +# BackfillRemainingTokenDigests, or later. That code finds tokens only by +# digest and ignores both columns, so it never names them. Code from before it +# still writes personal access tokens' plaintext, and falls back to reading +# it, so it fails once the column is gone. Code from before +# DropPlaintextOauthCredentials, which doesn't ignore refresh_token, names it +# on every INSERT. +# +# Undoing this restores both columns empty and nullable, as they were before +# any token was written. The tokens they held can't be recovered from their +# digests, so code that reads only plaintext can't authenticate them again. +class DropIdentityAccessTokenPlaintext < ActiveRecord::Migration[8.2] + def up + if connection.adapter_name.in?(%w[ Mysql2 Trilogy ]) + # MySQL would drop the columns INSTANT, from the data dictionary alone, + # leaving each row's plaintext in the table's pages until that row is + # next rewritten, which for most tokens is never. Rebuilding the table + # writes every row without them, still without blocking reads or writes. + execute "ALTER TABLE identity_access_tokens DROP INDEX index_identity_access_tokens_on_refresh_token, " \ + "DROP COLUMN refresh_token, DROP COLUMN token, ALGORITHM=INPLACE, LOCK=NONE" + else + remove_index :identity_access_tokens, :refresh_token, unique: true + remove_column :identity_access_tokens, :refresh_token + remove_column :identity_access_tokens, :token + end + end + + def down + add_column :identity_access_tokens, :token, :string + add_column :identity_access_tokens, :refresh_token, :string + add_index :identity_access_tokens, :refresh_token, unique: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 670c8e164..7e5ffc34f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.2].define(version: 2026_10_17_120000) do +ActiveRecord::Schema[8.2].define(version: 2026_10_17_130000) do create_table "accesses", id: :uuid, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.datetime "accessed_at" t.uuid "account_id", null: false @@ -359,15 +359,12 @@ t.uuid "identity_id", null: false t.uuid "oauth_client_id" t.string "permission" - t.string "refresh_token" t.string "refresh_token_digest", limit: 80 - t.string "token" t.string "token_digest", limit: 80 t.datetime "updated_at", null: false t.index ["authorization_code_jti"], name: "index_identity_access_tokens_on_authorization_code_jti", unique: true t.index ["identity_id"], name: "index_access_token_on_identity_id" t.index ["oauth_client_id"], name: "index_identity_access_tokens_on_oauth_client_id" - t.index ["refresh_token"], name: "index_identity_access_tokens_on_refresh_token", unique: true t.index ["refresh_token_digest"], name: "index_identity_access_tokens_on_refresh_token_digest", unique: true t.index ["token_digest"], name: "index_identity_access_tokens_on_token_digest", unique: true end diff --git a/db/schema_sqlite.rb b/db/schema_sqlite.rb index 14117a38f..a0145cf3f 100644 --- a/db/schema_sqlite.rb +++ b/db/schema_sqlite.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.2].define(version: 2026_10_17_120000) do +ActiveRecord::Schema[8.2].define(version: 2026_10_17_130000) do create_table "accesses", id: :uuid, force: :cascade do |t| t.datetime "accessed_at" t.uuid "account_id", null: false @@ -359,15 +359,12 @@ t.uuid "identity_id", null: false t.uuid "oauth_client_id" t.string "permission", limit: 255 - t.string "refresh_token", limit: 255 t.string "refresh_token_digest", limit: 80 - t.string "token", limit: 255 t.string "token_digest", limit: 80 t.datetime "updated_at", null: false t.index ["authorization_code_jti"], name: "index_identity_access_tokens_on_authorization_code_jti", unique: true t.index ["identity_id"], name: "index_access_token_on_identity_id" t.index ["oauth_client_id"], name: "index_identity_access_tokens_on_oauth_client_id" - t.index ["refresh_token"], name: "index_identity_access_tokens_on_refresh_token", unique: true t.index ["refresh_token_digest"], name: "index_identity_access_tokens_on_refresh_token_digest", unique: true t.index ["token_digest"], name: "index_identity_access_tokens_on_token_digest", unique: true end diff --git a/docs/docker-deployment.md b/docs/docker-deployment.md index b02508402..66f283256 100644 --- a/docs/docker-deployment.md +++ b/docs/docker-deployment.md @@ -48,7 +48,9 @@ Once you have one, set it in the `SECRET_KEY_BASE` environment variable: docker run --env SECRET_KEY_BASE=abcdefabcdef ... ``` -Personal access tokens and OAuth credentials are looked up by a keyed digest (HMAC-SHA256) rather than by the token itself. +Personal access tokens and OAuth credentials are stored only as a keyed digest (HMAC-SHA256), and looked up by it: the tokens themselves aren't kept. +The one exception is the token pair an OAuth refresh issues, which is kept encrypted, readable only for the 60 seconds in which a client may retry that refresh, and cleared within minutes after. +Tokens issued by versions that stored them in plaintext may still be readable in backups taken before the upgrade, and, on SQLite, in the database file's free pages until you run `VACUUM` (with the app stopped, and free disk space the size of the database). By default the key is derived from `SECRET_KEY_BASE`, so **changing `SECRET_KEY_BASE` invalidates every personal access token and every connected OAuth app**: people have to generate new tokens and reconnect their apps. To keep tokens working across a change of `SECRET_KEY_BASE`, set the key separately in `OAUTH_TOKEN_PEPPER` (at least 32 characters, for example from `bin/rails secret`). Set it before upgrading to a version that digests tokens, or before anyone creates one, and then keep it: changing, adding or removing it later invalidates tokens in the same way. diff --git a/docs/oauth/spec_conformance.md b/docs/oauth/spec_conformance.md index 2bd1bfbb3..c8a13d709 100644 --- a/docs/oauth/spec_conformance.md +++ b/docs/oauth/spec_conformance.md @@ -19,6 +19,7 @@ Snapshot: 2026-10-06, at the heads of the OAuth train, rebased onto main: | [#3159](https://github.com/basecamp/fizzy/pull/3159) | `oauth-client-sweep` | Sweep of abandoned DCR clients (on #3161) | | [#3192](https://github.com/basecamp/fizzy/pull/3192) | `oauth-token-digests` | Tokens and client secrets stored as digests, step 1 of 3 (on #3169) | | [#3202](https://github.com/basecamp/fizzy/pull/3202) | `oauth-token-digests-d2` | Personal access tokens stored and found only by digest, step 2 of 3 (on #3192) | +| [#3203](https://github.com/basecamp/fizzy/pull/3203) | `oauth-token-digests-d3` | Plaintext token columns dropped, step 3 of 3 (on #3202) | Commits are cited by subject, with the short SHA as of the snapshot. Rebases change the SHAs of #3080 and above; the subjects stay the same. @@ -90,7 +91,7 @@ Status key: | Requirement | Status | |---|---| -| Tokens are stored as HMAC-SHA256 digests under a server pepper, byte-compatible with bc3's `Oauth::TokenDigester`, and looked up by a unique index on the digest. Raw values are shown once: in the token response, the registration response, or the response that creates a personal access token | **Fixed** #3192 `oauth-token-digests`, step 1 of 3. OAuth access and refresh tokens and client secrets are digest-only, and a refresh retry within the grace window is answered from an encrypted copy of the successor pair that expires with the window. Step 2 (#3202 `oauth-token-digests-d2`) makes personal access tokens digest-only too: they're written and found only by digest, after a rerun of the backfill digests any that production wrote in plaintext before step 1 reached it. The plaintext they held before then stays in a column the code ignores, until step 3 drops it. | +| Tokens are stored as HMAC-SHA256 digests under a server pepper, byte-compatible with bc3's `Oauth::TokenDigester`, and looked up by a unique index on the digest. Raw values are shown once: in the token response, the registration response, or the response that creates a personal access token | **Fixed** #3192 `oauth-token-digests`, step 1 of 3. OAuth access and refresh tokens and client secrets are digest-only, and a refresh retry within the grace window is answered from an encrypted copy of the successor pair that expires with the window. Step 2 (#3202 `oauth-token-digests-d2`) makes personal access tokens digest-only too: they're written and found only by digest, after a rerun of the backfill digests any that production wrote in plaintext before step 1 reached it. Step 3 (#3203 `oauth-token-digests-d3`) drops the plaintext columns (personal access tokens', and refresh tokens', empty since step 1), and with them the plaintext of tokens issued before digests: no token is stored in plaintext. | | Newly issued tokens carry a distinctive prefix naming their kind: `fizzy_pat_` (personal access token), `fizzy_at_` (OAuth access token), `fizzy_rt_` (OAuth refresh token). Tokens issued earlier have no prefix and still work | **Fixed** #3163. bc3 prefixes similarly (`bc_at_`, `bc_rt_`, and PATs as `bc_at_`). Client secrets carry `fizzy_cs_`, as bc3's carry `bc_cs_`. New tokens and secrets carry 44 base58 characters after the prefix, 257 bits, at least bc3's 32 bytes (rubric TK-1); earlier ones are 24 characters and keep working. Nobody has registered with GitHub's secret-scanning partner program, either here or in bc3. | ## Refresh tokens (RFC 6749 §6, OAuth 2.1 §4.3, RFC 9700 §4.14) diff --git a/test/migrations/backfill_remaining_token_digests_test.rb b/test/migrations/backfill_remaining_token_digests_test.rb deleted file mode 100644 index 31d516ed2..000000000 --- a/test/migrations/backfill_remaining_token_digests_test.rb +++ /dev/null @@ -1,103 +0,0 @@ -require "test_helper" -require Rails.root.join("db/migrate/20261017120000_backfill_remaining_token_digests") - -class BackfillRemainingTokenDigestsTest < ActiveSupport::TestCase - # Production code that stores only plaintext kept writing personal access - # tokens after the first backfill, and one not used since has no digest. - setup do - @personal = identities(:david).access_tokens.create!(description: "Before digests") - store_plaintext_only @personal, "legacyPersonalToken0123456789", updated_at: 3.days.ago.round - - @grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) - store_plaintext_only @grant, "legacyAccessToken0123456789", updated_at: 80.days.ago.round - end - - test "digests every undigested token, keeping a personal token's plaintext and clearing a grant's" do - backfill - - @personal.reload - assert_equal TokenDigester.digest("legacyPersonalToken0123456789"), @personal.token_digest - assert_equal "legacyPersonalToken0123456789", stored_plaintext(@personal) - - @grant.reload - assert_equal TokenDigester.digest("legacyAccessToken0123456789"), @grant.token_digest - assert_nil stored_plaintext(@grant) - end - - test "keeps the plaintext of a token issued by the personal access client" do - store_plaintext_only @grant, "legacyAccessToken0123456789", oauth_client_id: personal_access_client.id - - backfill - - assert_equal TokenDigester.digest("legacyAccessToken0123456789"), @grant.reload.token_digest - assert_equal "legacyAccessToken0123456789", stored_plaintext(@grant) - end - - # A grant's updated_at is its idle-lapse clock. - test "leaves updated_at alone" do - assert_no_changes -> { [ @personal.reload.updated_at, @grant.reload.updated_at ] } do - backfill - end - end - - test "leaves digested tokens alone, so it can run again" do - digested = identity_access_tokens(:davids_api_token) - - assert_no_changes -> { digested.reload.attributes } do - backfill - backfill - end - end - - test "the backfilled token authenticates by digest" do - backfill - - assert_equal @personal, Identity::AccessToken.find_honored("legacyPersonalToken0123456789") - assert_equal @grant, Identity::AccessToken.find_by_raw_token("legacyAccessToken0123456789") - end - - # The first backfill kept personal access tokens' plaintext beside their - # digests, so a later token can duplicate one already digested. - test "refuses duplicate tokens before digesting anything, without naming them" do - copy = identities(:kevin).access_tokens.create!(description: "Digested", raw_token: "legacyPersonalToken0123456789") - BackfillRemainingTokenDigests::AccessToken.where(id: copy.id).update_all token: "legacyPersonalToken0123456789" - - error = assert_raises(RuntimeError) { backfill } - - assert_match @personal.id, error.message - assert_match copy.id, error.message - assert_no_match(/legacyPersonalToken/, error.message) - assert_nil @personal.reload.token_digest - assert_nil @grant.reload.token_digest - end - - test "does nothing once the plaintext column is gone" do - migration = BackfillRemainingTokenDigests.new.tap { |migration| migration.verbose = false } - migration.stubs(:column_exists?).with(:identity_access_tokens, :token).returns(false) - - migration.up - - assert_nil @personal.reload.token_digest - end - - private - # Identity::AccessToken ignores the plaintext column, so it's written and - # read through the migration's own model. - def store_plaintext_only(access_token, raw, **attributes) - BackfillRemainingTokenDigests::AccessToken.where(id: access_token.id).update_all token: raw, token_digest: nil, **attributes - end - - # The personal access client may not exist yet, so the grant's client stands in for it. - def personal_access_client - Oauth::Client.find_by(client_id: BackfillRemainingTokenDigests::PERSONAL_ACCESS_CLIENT_ID) || - oauth_clients(:mcp_client).tap { |client| client.update_columns client_id: BackfillRemainingTokenDigests::PERSONAL_ACCESS_CLIENT_ID } - end - - def stored_plaintext(access_token) - BackfillRemainingTokenDigests::AccessToken.find(access_token.id).token - end - - def backfill - BackfillRemainingTokenDigests.new.tap { |migration| migration.verbose = false }.up - end -end diff --git a/test/migrations/backfill_token_digests_test.rb b/test/migrations/backfill_token_digests_test.rb deleted file mode 100644 index 41f3b2504..000000000 --- a/test/migrations/backfill_token_digests_test.rb +++ /dev/null @@ -1,85 +0,0 @@ -require "test_helper" -require Rails.root.join("db/migrate/20261015120100_backfill_token_digests") - -class BackfillTokenDigestsTest < ActiveSupport::TestCase - setup do - @personal = identities(:david).access_tokens.create!(description: "Before digests") - store_plaintext_only @personal, "legacyPersonalToken0123456789", updated_at: 3.days.ago.round - - @grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) - store_plaintext_only @grant, "legacyAccessToken0123456789", updated_at: 80.days.ago.round - end - - test "digests every undigested token, keeping a personal token's plaintext and clearing a grant's" do - backfill - - @personal.reload - assert_equal TokenDigester.digest("legacyPersonalToken0123456789"), @personal.token_digest - assert_equal "legacyPersonalToken0123456789", stored_plaintext(@personal) - - @grant.reload - assert_equal TokenDigester.digest("legacyAccessToken0123456789"), @grant.token_digest - assert_nil stored_plaintext(@grant) - end - - # A grant's updated_at is its idle-lapse clock. - test "leaves updated_at alone" do - assert_no_changes -> { [ @personal.reload.updated_at, @grant.reload.updated_at ] } do - backfill - end - end - - test "leaves digested tokens alone, so it can run again" do - digested = identity_access_tokens(:davids_api_token) - - assert_no_changes -> { digested.reload.attributes } do - backfill - backfill - end - end - - test "the backfilled token authenticates by digest" do - backfill - - assert_equal @personal, Identity::AccessToken.find_honored("legacyPersonalToken0123456789") - assert_equal @grant, Identity::AccessToken.find_by_raw_token("legacyAccessToken0123456789") - end - - # The plaintext refresh token column outlives this stack, so it's emptied. - test "digests a grant's refresh token and clears its plaintext" do - BackfillTokenDigests::AccessToken.where(id: @grant.id).update_all refresh_token: "legacyRefreshToken0123456789", refresh_token_digest: nil - - backfill - - assert_equal @grant, Identity::AccessToken.find_by_refresh_token("legacyRefreshToken0123456789") - assert_nil BackfillTokenDigests::AccessToken.find(@grant.id).refresh_token - end - - test "refuses duplicate tokens before digesting anything, without naming them" do - copy = identities(:kevin).access_tokens.create!(description: "Copy") - store_plaintext_only copy, "legacyPersonalToken0123456789" - - error = assert_raises(RuntimeError) { backfill } - - assert_match @personal.id, error.message - assert_match copy.id, error.message - assert_no_match(/legacyPersonalToken/, error.message) - assert_nil @personal.reload.token_digest - assert_nil @grant.reload.token_digest - end - - private - # Identity::AccessToken ignores the plaintext column, so it's written and - # read through the migration's own model. - def store_plaintext_only(access_token, raw, **attributes) - BackfillTokenDigests::AccessToken.where(id: access_token.id).update_all token: raw, token_digest: nil, **attributes - end - - def stored_plaintext(access_token) - BackfillTokenDigests::AccessToken.find(access_token.id).token - end - - def backfill - BackfillTokenDigests.new.tap { |migration| migration.verbose = false }.up - end -end diff --git a/test/migrations/drop_identity_access_token_plaintext_test.rb b/test/migrations/drop_identity_access_token_plaintext_test.rb new file mode 100644 index 000000000..845a62e16 --- /dev/null +++ b/test/migrations/drop_identity_access_token_plaintext_test.rb @@ -0,0 +1,50 @@ +require "test_helper" +require Rails.root.join("db/migrate/20261017130000_drop_identity_access_token_plaintext") + +# DropIdentityAccessTokenPlaintext runs while processes still serve the code +# that shipped with BackfillRemainingTokenDigests, which ignores both plaintext +# columns. With partial_inserts off, a process names every column it has cached +# on each INSERT, so one that loaded identity_access_tokens' columns before the +# migration must still create tokens after it. +class DropIdentityAccessTokenPlaintextTest < ActiveSupport::TestCase + # Each step is DDL, which MySQL commits on its own. + self.use_transactional_tests = false + + # identity_access_tokens as the code before this migration models it. + class PreviousAccessToken < ApplicationRecord + self.table_name = "identity_access_tokens" + self.ignored_columns = %w[ token refresh_token ] + end + + setup do + migrate :down + PreviousAccessToken.reset_column_information.then { PreviousAccessToken.columns } + end + + teardown do + migrate :up if ActiveRecord::Base.connection.column_exists?(:identity_access_tokens, :token) + [ PreviousAccessToken, Identity::AccessToken ].each(&:reset_column_information) + PreviousAccessToken.where(description: "Cached before the migration").delete_all + end + + test "drops both plaintext columns" do + migrate :up + + assert_not ActiveRecord::Base.connection.column_exists?(:identity_access_tokens, :token) + assert_not ActiveRecord::Base.connection.column_exists?(:identity_access_tokens, :refresh_token) + end + + test "a process that cached identity_access_tokens' columns before it creates tokens after it" do + migrate :up + + assert_difference -> { PreviousAccessToken.count } do + PreviousAccessToken.create! identity_id: identities(:david).id, permission: "write", description: "Cached before the migration", + token_digest: TokenDigester.digest(SecureRandom.base58(24)) + end + end + + private + def migrate(direction) + DropIdentityAccessTokenPlaintext.new.tap { |migration| migration.verbose = false }.migrate(direction) + end +end diff --git a/test/models/identity/access_token_test.rb b/test/models/identity/access_token_test.rb index 9582012a8..3302ae64a 100644 --- a/test/models/identity/access_token_test.rb +++ b/test/models/identity/access_token_test.rb @@ -119,7 +119,6 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase assert_equal TokenDigester.digest(raw_token), token.token_digest assert_nil token.refresh_token_digest - assert_nil stored_plaintext(token) end test "an OAuth grant stores only digests of its tokens" do @@ -127,7 +126,6 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase raw_token, raw_refresh_token = grant.raw_token, grant.raw_refresh_token grant.reload - assert_nil stored_plaintext(grant) assert_equal TokenDigester.digest(raw_token), grant.token_digest assert_equal TokenDigester.digest(raw_refresh_token), grant.refresh_token_digest assert_nil grant.raw_token @@ -139,7 +137,6 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase raw_token, raw_refresh_token = grant.raw_token, grant.raw_refresh_token grant.reload - assert_nil stored_plaintext(grant) assert_equal TokenDigester.digest(raw_token), grant.token_digest assert_equal TokenDigester.digest(raw_refresh_token), grant.refresh_token_digest end @@ -166,28 +163,9 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase assert statements.none? { |sql| sql.match?(/\Wtoken\W*=/) }, statements.join("\n") end - # BackfillRemainingTokenDigests digests any token stored only in plaintext - # before this code reads by digest alone. - test "a token stored only in plaintext doesn't authenticate" do - token = identities(:david).access_tokens.create!(description: "Before digests") - Identity::AccessToken.where(id: token.id).update_all token: "legacyPersonalToken0123456789", token_digest: nil - - statements = capture_sql do - assert_nil Identity::AccessToken.find_honored("legacyPersonalToken0123456789") - end - assert statements.none? { |sql| sql.match?(/\Wtoken\W*=/) }, statements.join("\n") - assert_nil token.reload.token_digest - end - - test "the plaintext column is ignored" do - assert_not_includes Identity::AccessToken.column_names, "token" - assert_not Identity::AccessToken.new.respond_to?(:token) - end - - # Dropped beside `token` only once every process runs code that ignores it. - test "the plaintext refresh token column is ignored" do - assert_not_includes Identity::AccessToken.column_names, "refresh_token" - assert_not Identity::AccessToken.new.respond_to?(:refresh_token) + test "no token is stored in plaintext" do + assert_not Identity::AccessToken.connection.column_exists?(:identity_access_tokens, :token) + assert_not Identity::AccessToken.connection.column_exists?(:identity_access_tokens, :refresh_token) end test "two tokens can't share a digest" do @@ -204,10 +182,6 @@ class Identity::AccessTokenTest < ActiveSupport::TestCase end private - def stored_plaintext(access_token) - Identity::AccessToken.where(id: access_token.id).pick(Arel.sql("token")) - end - def capture_sql(&block) statements = [] subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") { |*, payload| statements << payload[:sql] }