Repository navigation
Conversation
When the WordPress Secrets API is available (WordPress 7.2+, or the secrets-api feature plugin), store each user's TOTP shared secret in it, encrypted, instead of in plaintext user meta. Sites without the API behave exactly as before. - Two_Factor_Secrets adapter: the only caller of the Secrets API. Always uses network scope; detects the API at call time. - Lazy, verified migration of existing plaintext secrets, plus `wp two-factor secrets status|migrate|export`. - Fail closed: an unreadable secret never authenticates. Core now forces the fallback provider (or refuses login) when every enrolled provider is registered but unusable, instead of allowing a password-only login. - Admin notice and Site Health test when migrated secrets become unreachable; lifecycle hooks keep working when TOTP is disabled. - Opt out of writes and migration with the two_factor_use_secrets_api filter. Closes WordPress#455. Supersedes WordPress#819.
--user is a WP-CLI global parameter: WP-CLI consumed it to switch the current user and the command never saw it, so 'secrets export --user=X --yes' exported every user's secret to plaintext instead of just X's. Accept the user positionally, matching 'status <user>' and 'backup-codes generate <user>'.
Two-Factor should never pull a secret back out of the Secrets API into plaintext user meta. Remove Two_Factor_Totp::export_user_totp_key() and the 'wp two-factor secrets export' action. The worst case after removing the Secrets API is that affected users re-enrol their authenticator app: an administrator resets it from the profile, which works while the API is missing. The admin notice and Site Health result now point to restoring the Secrets API or resetting affected users instead of to export.
The dry run ended with "Dry run: Migrated 0, failed 0, skipped N", which is accurate but reads as if nothing would happen. Report "Dry run: would migrate N, skipped M. Nothing was changed." instead, and only list users who actually hold a plaintext secret, so a dry run for a single user without one is reported as a skip.
The Secrets API wraps one root key with the site key (WP_SECRETS_KEY). If that key changes without the API's own rotation, the root key can't be unwrapped and every read and write fails, including writes of brand new secrets. Re-enrolling after an admin reset then returned a generic 500 "re-scan the QR code" db_error, and the reset itself had already left the user with no second factor. - TOTP setup now returns a 503 two_factor_secrets_key_unavailable that says to restore WP_SECRETS_KEY, or two_factor_secrets_write_failed for other Secrets API write failures. Nothing is stored, and there is still no plaintext fallback while the API is present. - While the key is unusable, the profile says to restore the key and offers no reset, and the REST reset is refused, so the user stays enrolled and kept out instead of being left with a password alone. - Two_Factor_Secrets::is_key_unavailable_error() keeps the Secrets API error code inside the adapter.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @calvinalkan. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
|
||
| = How are authenticator (TOTP) secrets stored? = | ||
|
|
||
| By default, and on WordPress versions without the Secrets API, they are stored in plaintext user meta. When the WordPress [Secrets API](https://github.com/ericmann/secrets-api) is available, new secrets are stored encrypted through it automatically, and existing plaintext secrets are migrated the next time each user logs in. To migrate everyone at once run `wp two-factor secrets migrate`. To opt out of writing to the Secrets API, return `false` from the `two_factor_use_secrets_api` filter; users who were already migrated are still read from it. |
There was a problem hiding this comment.
Open Question for discussion: shall we add a one-time notice for administrators with an information on how to migrate everyone at once?
There was a problem hiding this comment.
Site Health already covers this: once storage is turned on and plaintext secrets remain, it shows a recommendation naming wp two-factor secrets migrate. I'd rather not add another admin notice on top, but we could add the same pointer to the settings page next to the opt-in if you think Site Health is too easy to miss.
There was a problem hiding this comment.
@ericmann i think the site health option is quite easy to miss and as this is a great feature we should make people aware of it so they could do it manually.
|
I'd love to see us adopt the Secrets API in v0.18 of Two Factor. The earlier proof of concept has since landed in the wordpress/ai plugin as an opt-in experiment (WordPress/ai#560), which encrypts AI connector API keys using a bundled copy of Displace Secrets Manager. So there's already real-world usage to learn from. Adopting it now would also let us help test the API and gather feedback ahead of the WP 7.2 release. A migration later to the core version of the wp-secrets-api should be rather straight forward. And for users below WP 7.2 we could still ship this bundled integration. Thank you for your work on this, Eric! |
|
100% in favor of getting support ready for this so we've got it integrated before 7.2 lands. I need to review it more closely, but at the moment do we have any toggle enabling it if the secrets api is there, or is the site just forced into it? I think (if there isn't already) an initial opt-in from a site admin acknowledging that they understand that rotating the keys improperly could blow away all existing secrets and the like, so they aren't just silently upgraded and then locked out. |
kasparsd
left a comment
kasparsd
left a comment
There was a problem hiding this comment.
This looks good. Had just a few questions and suggestion -- all optional. I think we should merge and iterate.
Storing TOTP secrets with the Secrets API was automatic whenever the API was present. Because migration is one-directional and a changed secrets key makes every stored secret unreadable, an administrator now has to turn it on under Settings > Two-Factor and acknowledge that first. The choice is a network option, since secrets are network scoped; on multisite only a network administrator can change it. Until it is on, nothing is written or migrated, Site Health recommends it with a link to the settings screen, and `wp two-factor secrets migrate` refuses. Turning it off stops new writes and migration only. The two_factor_use_secrets_api filter now receives the administrator's choice as its default, so it can turn storage off or force it on. `wp two-factor secrets status` reports opted_in and enabled in place of filter_enabled.
@georgestephanis Good call. It was automatic; as of b7642ca it's opt-in. An admin has to turn it on under Settings → Two-Factor and tick an acknowledgement that it's one-way and depends on the site's secrets key. Until then nothing is written or migrated, and Site Health points them at the setting. One note: the |
Changelogs are generated at release time.
The plugin requires PHP 7.4, so the adapter's methods now declare scalar parameter and return types. Methods returning string|null|WP_Error or true|WP_Error keep an untyped return, as 7.4 has no union types. Two_Factor_Totp casts the user ID (and the key) where it calls the adapter, so its long-standing loosely typed public methods keep accepting what they did before.
The feature plugin was mapped into the tests environment by .wp-env.json. It is now wordpress/secrets-api 0.2.1 in require-dev, and the test bootstrap loads it from vendor/, so the tests no longer depend on the wp-env plugin mapping. The package is not on Packagist, so composer.json declares it as a package repository pointing at the tagged zip. A vcs repository is not usable here: the wp-env container has no git, and the GitHub API it would fall back to is rate limited for unauthenticated CI runs.
The filter now receives whether the Secrets API is actually loaded as a second argument. It still cannot force a missing API to count as present.
Two_Factor_Secrets was all static, so tests reached into it through a public $test_overrides array and the two_factor_secrets_api_present filter, and most of them needed the real Secrets API loaded. The logic now lives in Two_Factor_Secrets_Manager, which is constructed with a Two_Factor_Secrets_Store. Two_Factor_Secrets_Api_Store is the only code that calls the Secrets API. Two_Factor_Secrets keeps every public static method, forwarding to a shared manager, so callers are unchanged. Tests install an in-memory store instead, which removes $test_overrides and the filter and lets the secrets tests run without the feature plugin. A small set of tests still exercises the real store.
All Secrets API handling for TOTP lived in Two_Factor_Totp, and the CLI called that class by name, so a custom provider got none of it. Providers now declare the user meta keys that hold secrets through the static Two_Factor_Provider::user_secret_meta_keys(). The storage logic (verified writes, lazy one-way migration, deletion, storage state) moves into Two_Factor_Secrets_Manager, generalised over a slug and a meta key. Two_Factor_Secrets_Lifecycle handles deletion with the user and on uninstall, the unreachable-secrets notice, Site Health and the counts for every declared secret, registered from Two_Factor_Core::add_hooks(). Two_Factor_Totp's public methods keep their signatures and become wrappers. Its slug, marker key and secret name are unchanged, so nothing already stored moves. Two_Factor_Totp::register_secret_lifecycle_hooks() is removed. Two_Factor_Core::get_provider_secrets() lists secrets from every provider class, including ones turned off site-wide, without instantiating any. A duplicate slug is refused. wp two-factor secrets migrate covers every declared secret, and secrets status prints one row per secret.
|
Heads up: there's a follow-up stacked on this branch at ericmann#3. It lives on my fork because a PR here can't target a branch that only exists there. It answers @kasparsd's two larger comments:
It's a refactor. Nothing already stored moves and login behaviour is unchanged. I'd like to merge it into this branch before this PR lands, because it renames and removes a few things introduced here that have never been released (the test overrides, the lifecycle-hooks method, the Site Health test ID, the |
Let providers declare their secrets; move secrets logic behind an injectable store
|
The changes from ericmann#3 have been merged down into this PR - we're good to go. |
Resolve the composer.lock conflict: keep master's phpstan 2.3.0 bump and this branch's wordpress/secrets-api entry, and regenerate the content hash.
Closes #455. Supersedes #819.
When the WordPress Secrets API is available, an administrator can choose to store TOTP secrets encrypted through it instead of in plaintext user meta. The API is headed for core in 7.2 and is available today as the
secrets-apifeature plugin (v0.2.1). Without it, or until an administrator turns the setting on, nothing changes.Opt-in
Encrypted storage is off until an administrator turns it on. Nobody is upgraded silently.
WP_SECRETS_KEY, or the salts when it isn't defined) makes stored secrets unreadable. Saving without the acknowledgement is refused.wp two-factor secrets migraterefuses and points at the settings screen. Site Health shows a recommendation with a link there.Demo
Recorded on a wp-env site running the
secrets-apifeature plugin using Mac OS (say) narrations:01-secrets-api-overview.mp4
02-broken-key-recovery.mp4
secrets status,migrate --dry-run, thenmigrate.03-bulk-migration.mp4
Why
TOTP secrets live in plaintext user meta, so anyone with a database dump or backup can generate valid codes for every TOTP user. #389 and #819 encrypted them inside Two-Factor, which made the plugin responsible for cryptography and key management. Reviewing #819, @georgestephanis preferred a shared architecture that other plugins could use too. The Secrets API is that architecture: Trac #66187, WordPress/wordpress-develop#13759.
Failing closed
On #819, @georgestephanis asked whether a bad or rotated key could fail open. It can't, and that's deliberate; it fixes an oversight in my earlier design.
no_available_2fa_methods.WP_SECRETS_KEY, the user is locked out until the original key is restored. Backup codes still work. Two-Factor doesn't offer a reset in that state, because no new secret can be stored until the key is back, and a reset would leave the user with a password alone. A broken key should get noticed, not quietly routed to email.Two_Factor_Provider::is_enrolled_but_unavailable_for_user()lets core force the fallback instead.Migration
wp two-factor secrets migrate [<user>] [--batch-size=<n>] [--dry-run]migrates everyone at once.wp two-factor secrets status, and thetotp_storagefield onwp two-factor status <user>, show where things stand.two_factor_use_secrets_apifilter receives the administrator's choice as its default. Returningfalsestops new writes and migration; users who were already migrated are still read from the Secrets API. Returningtrueturns storage on without the settings screen, which is meant for hosts that manage the key for the site.Design notes
Two_Factor_Secretsis the only code that calls the Secrets API, and it checks for the API at call time.two-factor/totp-{user_id}. The API has no user scope, and site scope is tied to the blog ID.two_factor_secrets_api_enabled, absent until an administrator turns it on and removed on uninstall._two_factor_totp_key_networkrecords which network holds the secret. That marker is how "not migrated" is told apart from "migrated but unreadable".get_user_totp_key_state()returns the key,null, or aWP_Error.get_user_totp_key()keeps its signature.is_enrolled_but_unavailable_for_user()and the staticuninstall_user_data(), so core has no TOTP-specific logic.two_factor_secrets_migrated,two_factor_secrets_migration_failedandtwo_factor_secret_unavailable. None of them receive the secret.wpmu_delete_user). Uninstall removes all secrets when the API is available. If it isn't, the secrets are left behind.Compatibility
There's no
Requires at leastbump and no new dependency. PublicTwo_Factor_Totpmethod signatures are unchanged, andincludes/is untouched.