Repository navigation
feat: move Key Encryption onto Secrets API - #1107
Pranjal1423 wants to merge 17 commits into
Conversation
|
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 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. |
dkotter
left a comment
dkotter
left a comment
There was a problem hiding this comment.
If I'm following correctly, looks like the changes here require someone to manually install the new Secrets API plugin, correct? If so, that's not the direction we were wanting to go, ideally we can bundle that plugin ourselves, matching what we already do with the older version of that plugin.
|
Thanks for clarifying, @dkotter! That makes sense. I’ll update the PR to bundle the new Secrets API ( I’ll:
I’ll push the updated changes shortly! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1107 +/- ##
=============================================
- Coverage 82.17% 82.13% -0.05%
- Complexity 3399 3448 +49
=============================================
Files 136 137 +1
Lines 13187 13286 +99
=============================================
+ Hits 10837 10912 +75
- Misses 2350 2374 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…to feat/secrets-api-integration
dkotter
left a comment
dkotter
left a comment
There was a problem hiding this comment.
On a quick review, this seems to be headed in the right direction, thanks for all the work here!
As I continue to test, one initial question is are we purposely leaving the old Displace plugin code behind? Is there a reason to keep that? I guess I would expect for all of those files to be removed and replaced with the new code
|
One other quick thing as I'm reviewing, the Secrets API made it's way into WordPress Core just a few days ago. We should ensure the code we are shipping here includes the latest version of that, noting the code hosted at https://github.com/ericmann/secrets-api may not be the exact same code that was shipped in Core. Should look at the upstream commits to Core to see if there's anything new we need to pull in |
whyisjake
left a comment
whyisjake
left a comment
There was a problem hiding this comment.
Thanks for this, @Pranjal1423. I tested it against core trunk now that the Secrets API has landed (5744bc5803, Trac #66187), and it holds up well on the two things I was most worried about:
No name conflicts on trunk. Core loads wp-includes/secrets.php unconditionally, so a second copy of any wp_*secret* function would fatal. includes/Vendor/Secrets/load.php returns early on function_exists( 'wp_get_secret' ). On 7.2-alpha-64345 the bundled copy never loads, and wp_get_secret resolves to core's file. The option names don't overlap either: the plugin's legacy rows are _secret_ai/* and _secrets_master_key, and core's are _wp_secret_* and _wp_secrets_root_key. ai/<connector>_api_key passes wp_secrets_validate_name().
Existing keys carry over. On trunk I saved a fake key with Key Encryption on using develop (it landed in _secret_ai/anthropic_api_key), switched to this branch, and read it again. It was promoted to _wp_secret_ai/anthropic_api_key, wp_get_secret()->reveal() matched, and get_option() returned the key.
The inline suggestions cover what I found:
- A
WP_Errorfromwp_get_secret()is treated as "not set", so a key core can't decrypt (salt or site-key change, broken drop-in) silently disappears, and opt-out restores nothing. This is the one that matters for users. - Read-time promotion and
set_secret()delete the legacy row without the read-back check the migration already does. - Uninstall can miss secrets for connectors that are no longer registered, or that a drop-in stores.
- Tests for each.
One follow-up that isn't a suggestion: when decrypt_all() leaves a key in get_unreadable_secrets(), the opt-out should probably show an admin notice instead of quietly turning off.
Tested on WordPress 7.2-alpha-64345 (trunk), PHP in wp-env, with the suggestions applied:
Key_EncryptionTest,V1_5_0Test,UninstallTest,DeactivationTest: 41 tests, 0 failures (3 skipped).- Full PHPUnit suite: 1929 tests, 0 failures, single site and multisite.
- PHPStan: no errors. PHPCS: clean on the changed lines.
- Without the suggestions, the full suite also passes on trunk (1926 tests, single site and multisite).
I didn't run the e2e suite against trunk, and I didn't test the bundled path on 7.0.4.
AI assistance: Yes. Tool(s): Claude Code. Used for: reproducing the trunk behavior, drafting these suggestions and tests, and running the checks above.
| /** | ||
| * Retrieves a secret value, preferring the global Secrets API when present, | ||
| * falling back to the bundled provider. | ||
| * | ||
| * Transparently promotes any legacy prototype row to the new API on read. | ||
| * | ||
| * @since 1.5.0 | ||
| * | ||
| * @param string $secret_key Namespaced secret key. | ||
| * @return string|null Decrypted secret value, or null if not found. | ||
| */ | ||
| public function get_secret( string $secret_key ): ?string { | ||
| $this->ensure_secrets_api(); | ||
|
|
||
| if ( function_exists( 'wp_get_secret' ) ) { | ||
| $secret = wp_get_secret( $secret_key ); | ||
| if ( is_object( $secret ) && method_exists( $secret, 'reveal' ) ) { | ||
| return $secret->reveal(); | ||
| } | ||
| if ( is_string( $secret ) && '' !== $secret ) { | ||
| return $secret; | ||
| } | ||
| } | ||
|
|
||
| // If absent in the new API (or Secrets API unavailable), check if legacy store holds it: | ||
| if ( $this->is_legacy_provider_available() ) { | ||
| try { | ||
| $legacy = Secrets::get( $secret_key, $this->secret_context() ); | ||
| } catch ( \Throwable $e ) { | ||
| $legacy = null; | ||
| } | ||
|
|
||
| if ( null !== $legacy && '' !== $legacy ) { | ||
| // Transparent read-time promotion if new API is present: | ||
| if ( function_exists( 'wp_set_secret' ) ) { | ||
| $stored = wp_set_secret( $secret_key, $legacy ); | ||
| if ( false !== $stored && ! is_wp_error( $stored ) ) { | ||
| delete_option( Secrets_Provider_Encrypted_Options::OPTION_PREFIX . $secret_key ); | ||
| } | ||
| } | ||
| return $legacy; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
wp_get_secret() returning a WP_Error is treated as "not set", so a saved key silently disappears.
Core returns a WP_Error when a secret is stored but can't be opened: after WP_SECRETS_KEY, LOGGED_IN_KEY or LOGGED_IN_SALT change, or with a broken secrets.php drop-in. I reproduced it on trunk (7.2-alpha-64345) by saving a key, then reading it with a different WP_SECRETS_KEY:
wp_get_secret()→WP_Errorsecret_key_unavailableget_option( 'connectors_ai_anthropic_api_key' )→'', so the provider just looks unconfigureddecrypt_all()on opt-out → restores 0 keys, with nothing reported
The secret itself is intact; it reads again once the original key is back. So it should be reported, not dropped. This keeps the legacy fallback, never promotes over an unreadable secret, warns once per request through wp_trigger_error(), and exposes get_unreadable_secrets() so the opt-out can show a notice for any key it couldn't restore.
It also routes read-time promotion through a write_and_verify() helper: today the legacy row is deleted as soon as wp_set_secret() returns, without the read-back check maybe_migrate_legacy_secrets() already does.
(The $unreadable property is included here only so the suggestion is self-contained. It's probably better moved up next to $bypass_read_filter.)
| /** | |
| * Retrieves a secret value, preferring the global Secrets API when present, | |
| * falling back to the bundled provider. | |
| * | |
| * Transparently promotes any legacy prototype row to the new API on read. | |
| * | |
| * @since 1.5.0 | |
| * | |
| * @param string $secret_key Namespaced secret key. | |
| * @return string|null Decrypted secret value, or null if not found. | |
| */ | |
| public function get_secret( string $secret_key ): ?string { | |
| $this->ensure_secrets_api(); | |
| if ( function_exists( 'wp_get_secret' ) ) { | |
| $secret = wp_get_secret( $secret_key ); | |
| if ( is_object( $secret ) && method_exists( $secret, 'reveal' ) ) { | |
| return $secret->reveal(); | |
| } | |
| if ( is_string( $secret ) && '' !== $secret ) { | |
| return $secret; | |
| } | |
| } | |
| // If absent in the new API (or Secrets API unavailable), check if legacy store holds it: | |
| if ( $this->is_legacy_provider_available() ) { | |
| try { | |
| $legacy = Secrets::get( $secret_key, $this->secret_context() ); | |
| } catch ( \Throwable $e ) { | |
| $legacy = null; | |
| } | |
| if ( null !== $legacy && '' !== $legacy ) { | |
| // Transparent read-time promotion if new API is present: | |
| if ( function_exists( 'wp_set_secret' ) ) { | |
| $stored = wp_set_secret( $secret_key, $legacy ); | |
| if ( false !== $stored && ! is_wp_error( $stored ) ) { | |
| delete_option( Secrets_Provider_Encrypted_Options::OPTION_PREFIX . $secret_key ); | |
| } | |
| } | |
| return $legacy; | |
| } | |
| } | |
| return null; | |
| } | |
| /** | |
| * Secrets found stored but unreadable during this request, keyed by secret key. | |
| * | |
| * @since 1.5.0 | |
| * @var array<string, \WP_Error> | |
| */ | |
| private array $unreadable = array(); | |
| /** | |
| * Retrieves a secret value, preferring the global Secrets API when present, | |
| * falling back to the bundled provider. | |
| * | |
| * Transparently promotes any legacy prototype row to the new API on read. | |
| * | |
| * @since 1.5.0 | |
| * | |
| * @param string $secret_key Namespaced secret key. | |
| * @return string|null Decrypted secret value, or null if not found. | |
| */ | |
| public function get_secret( string $secret_key ): ?string { | |
| $this->ensure_secrets_api(); | |
| $unreadable = null; | |
| if ( function_exists( 'wp_get_secret' ) ) { | |
| $secret = wp_get_secret( $secret_key ); | |
| if ( is_wp_error( $secret ) ) { | |
| // Stored, but not readable: the site key or salts changed, or a drop-in is broken. | |
| $unreadable = $secret; | |
| } elseif ( is_object( $secret ) && method_exists( $secret, 'reveal' ) ) { | |
| unset( $this->unreadable[ $secret_key ] ); | |
| return $secret->reveal(); | |
| } elseif ( is_string( $secret ) && '' !== $secret ) { | |
| return $secret; | |
| } | |
| } | |
| // If absent in the new API (or Secrets API unavailable), check if legacy store holds it: | |
| if ( $this->is_legacy_provider_available() ) { | |
| try { | |
| $legacy = Secrets::get( $secret_key, $this->secret_context() ); | |
| } catch ( \Throwable $e ) { | |
| $legacy = null; | |
| } | |
| if ( null !== $legacy && '' !== $legacy ) { | |
| // Promote on read, but never over an unreadable secret, and delete the | |
| // legacy row only once the new copy reads back intact. | |
| if ( null === $unreadable && $this->write_and_verify( $secret_key, $legacy ) ) { | |
| delete_option( Secrets_Provider_Encrypted_Options::OPTION_PREFIX . $secret_key ); | |
| } | |
| return $legacy; | |
| } | |
| } | |
| if ( null !== $unreadable ) { | |
| $this->record_unreadable( $secret_key, $unreadable ); | |
| } | |
| return null; | |
| } | |
| /** | |
| * Returns the secret keys found stored but unreadable during this request. | |
| * | |
| * Lets callers such as the Key Encryption opt-out report a key that could not be | |
| * restored instead of treating it as never set. | |
| * | |
| * @since 1.5.0 | |
| * | |
| * @return array<string, \WP_Error> Map of secret key to the error the Secrets API returned. | |
| */ | |
| public function get_unreadable_secrets(): array { | |
| return $this->unreadable; | |
| } | |
| /** | |
| * Records an unreadable secret and warns once per request. | |
| * | |
| * @since 1.5.0 | |
| * | |
| * @param string $secret_key Namespaced secret key. | |
| * @param \WP_Error $error The error the Secrets API returned. | |
| */ | |
| private function record_unreadable( string $secret_key, \WP_Error $error ): void { | |
| if ( isset( $this->unreadable[ $secret_key ] ) ) { | |
| return; | |
| } | |
| $this->unreadable[ $secret_key ] = $error; | |
| wp_trigger_error( | |
| __METHOD__, | |
| sprintf( | |
| /* translators: 1: Secret name, 2: Error code. */ | |
| __( 'The secret "%1$s" is stored but could not be read (%2$s). Restore the site key or salts it was stored with, or save the key again.', 'ai' ), | |
| $secret_key, | |
| $error->get_error_code() | |
| ), | |
| E_USER_WARNING | |
| ); | |
| } | |
| /** | |
| * Writes a secret through the Secrets API and confirms it reads back unchanged. | |
| * | |
| * @since 1.5.0 | |
| * | |
| * @param string $secret_key Namespaced secret key. | |
| * @param string $value Plaintext secret value. | |
| * @return bool True when the stored secret reads back as `$value`. | |
| */ | |
| private function write_and_verify( string $secret_key, string $value ): bool { | |
| if ( ! function_exists( 'wp_set_secret' ) || ! function_exists( 'wp_get_secret' ) ) { | |
| return false; | |
| } | |
| $stored = wp_set_secret( $secret_key, $value ); | |
| if ( false === $stored || is_wp_error( $stored ) ) { | |
| return false; | |
| } | |
| $secret = wp_get_secret( $secret_key ); | |
| return is_object( $secret ) && method_exists( $secret, 'reveal' ) && hash_equals( $value, (string) $secret->reveal() ); | |
| } |
| if ( function_exists( 'wp_set_secret' ) ) { | ||
| $result = wp_set_secret( $secret_key, $value ); | ||
| if ( false !== $result && ! is_wp_error( $result ) ) { | ||
| delete_option( Secrets_Provider_Encrypted_Options::OPTION_PREFIX . $secret_key ); | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Same read-back check before deleting the legacy row, so a write that doesn't read back falls through to the legacy store instead of removing the only readable copy.
| if ( function_exists( 'wp_set_secret' ) ) { | |
| $result = wp_set_secret( $secret_key, $value ); | |
| if ( false !== $result && ! is_wp_error( $result ) ) { | |
| delete_option( Secrets_Provider_Encrypted_Options::OPTION_PREFIX . $secret_key ); | |
| return true; | |
| } | |
| } | |
| if ( $this->write_and_verify( $secret_key, $value ) ) { | |
| delete_option( Secrets_Provider_Encrypted_Options::OPTION_PREFIX . $secret_key ); | |
| return true; | |
| } |
| $stored = wp_set_secret( $secret_key, $plaintext ); | ||
| if ( false === $stored || is_wp_error( $stored ) ) { | ||
| continue; | ||
| } | ||
|
|
||
| // Verify the new secret was written and matches the plaintext before deleting the old row. | ||
| $secret = wp_get_secret( $secret_key ); | ||
| $readback = is_object( $secret ) && method_exists( $secret, 'reveal' ) ? $secret->reveal() : $secret; | ||
| if ( $readback !== $plaintext ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This already does the read-back check. Reusing the helper keeps the three paths identical (and compares with hash_equals()).
| $stored = wp_set_secret( $secret_key, $plaintext ); | |
| if ( false === $stored || is_wp_error( $stored ) ) { | |
| continue; | |
| } | |
| // Verify the new secret was written and matches the plaintext before deleting the old row. | |
| $secret = wp_get_secret( $secret_key ); | |
| $readback = is_object( $secret ) && method_exists( $secret, 'reveal' ) ? $secret->reveal() : $secret; | |
| if ( $readback !== $plaintext ) { | |
| continue; | |
| } | |
| // Delete the old row only once the new secret reads back intact. | |
| if ( ! $this->write_and_verify( $secret_key, $plaintext ) ) { | |
| continue; | |
| } |
| if ( function_exists( 'wp_delete_secret' ) ) { | ||
| $bridge = new Secrets_Bridge(); | ||
| foreach ( $bridge->get_connector_setting_names() as $connector_id => $setting_name ) { | ||
| wp_delete_secret( Secrets_Bridge::SECRET_NAMESPACE . '/' . $connector_id . '_api_key' ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Deleting by registered connector misses secrets for connectors that are no longer registered. With a secrets.php drop-in those rows aren't in wp_options, so the _wp_secret_ai/ prefix sweep won't catch them either. wp_list_secrets( 'ai' ) covers both. The bundled copy defines it too.
| if ( function_exists( 'wp_delete_secret' ) ) { | |
| $bridge = new Secrets_Bridge(); | |
| foreach ( $bridge->get_connector_setting_names() as $connector_id => $setting_name ) { | |
| wp_delete_secret( Secrets_Bridge::SECRET_NAMESPACE . '/' . $connector_id . '_api_key' ); | |
| } | |
| } | |
| // List through the Secrets API so secrets kept by a `secrets.php` drop-in are | |
| // deleted too, including those of connectors that are no longer registered. | |
| if ( function_exists( 'wp_list_secrets' ) && function_exists( 'wp_delete_secret' ) ) { | |
| $secrets = wp_list_secrets( Secrets_Bridge::SECRET_NAMESPACE ); | |
| if ( is_array( $secrets ) ) { | |
| foreach ( $secrets as $secret ) { | |
| wp_delete_secret( $secret['name'] ); | |
| } | |
| } | |
| } |
| $bridge->ensure_secrets_api(); | ||
| $bridge->ensure_secrets_api(); | ||
| $this->assertTrue( function_exists( 'wp_get_secret' ) ); | ||
| } |
There was a problem hiding this comment.
Tests for the cases above, plus one that proves the bundled copy stands down when core provides the API. On trunk with the suggestions applied, all three pass. Without them, the first two fail:
test_bridge_reports_an_unreadable_secret: undefinedget_unreadable_secrets().test_bridge_promotion_keeps_the_legacy_row_until_it_reads_back: the legacy row is deleted.
They use an option__wp_secret_* filter to make the stored record unreadable, which makes core return secret_record_malformed. They also use wp_trigger_error_always_run, which has been available since WordPress 7.0, so they don't depend on WP_DEBUG.
| } | |
| } | |
| /** | |
| * Tests that a secret core stores but cannot read is reported, not treated as never set. | |
| * | |
| * @since 1.5.0 | |
| */ | |
| public function test_bridge_reports_an_unreadable_secret(): void { | |
| $bridge = Key_Encryption::get_bridge(); | |
| $this->assertTrue( $bridge->set_secret( self::SECRET_KEY, 'sk-unreadable' ) ); | |
| // A record the provider cannot open, as after a salt or site-key change. | |
| $corrupt = static function () { | |
| return 'not-a-secret-record'; | |
| }; | |
| $warnings = array(); | |
| $capture = static function ( $function_name, $message ) use ( &$warnings ) { | |
| $warnings[] = $message; | |
| }; | |
| add_filter( 'option__wp_secret_' . self::SECRET_KEY, $corrupt ); | |
| add_action( 'wp_trigger_error_always_run', $capture, 10, 2 ); | |
| add_filter( 'wp_trigger_error_trigger_error', '__return_false' ); | |
| try { | |
| $this->assertNull( $bridge->get_secret( self::SECRET_KEY ) ); | |
| $this->assertNull( $bridge->get_secret( self::SECRET_KEY ) ); | |
| $this->assertArrayHasKey( self::SECRET_KEY, $bridge->get_unreadable_secrets() ); | |
| $this->assertCount( 1, $warnings, 'Warns once per request, not on every read.' ); | |
| } finally { | |
| remove_filter( 'option__wp_secret_' . self::SECRET_KEY, $corrupt ); | |
| remove_action( 'wp_trigger_error_always_run', $capture, 10 ); | |
| remove_filter( 'wp_trigger_error_trigger_error', '__return_false' ); | |
| } | |
| // The secret was never deleted: it reads again once the record is intact. | |
| $this->assertSame( 'sk-unreadable', $bridge->get_secret( self::SECRET_KEY ) ); | |
| $this->assertArrayNotHasKey( self::SECRET_KEY, $bridge->get_unreadable_secrets() ); | |
| $bridge->delete_secret( self::SECRET_KEY ); | |
| } | |
| /** | |
| * Tests that read-time promotion keeps the legacy row when the new secret does not read back. | |
| * | |
| * @since 1.5.0 | |
| */ | |
| public function test_bridge_promotion_keeps_the_legacy_row_until_it_reads_back(): void { | |
| $bridge = Key_Encryption::get_bridge(); | |
| $legacy_key = 'ai/readback_key'; | |
| $legacy_row = '_secret_' . $legacy_key; | |
| $bridge->is_legacy_provider_available(); | |
| $this->assertTrue( Secrets::set( $legacy_key, 'sk-readback', self::SECRET_CONTEXT ) ); | |
| // The write succeeds, but the stored record cannot be read back. | |
| $corrupt = static function () { | |
| return 'not-a-secret-record'; | |
| }; | |
| add_filter( 'option__wp_secret_' . $legacy_key, $corrupt ); | |
| try { | |
| $this->assertSame( 'sk-readback', $bridge->get_secret( $legacy_key ) ); | |
| $this->assertNotFalse( get_option( $legacy_row, false ), 'The only readable copy must not be deleted.' ); | |
| } finally { | |
| remove_filter( 'option__wp_secret_' . $legacy_key, $corrupt ); | |
| } | |
| $bridge->delete_secret( $legacy_key ); | |
| } | |
| /** | |
| * Tests that the vendored Secrets API stands down when core provides it. | |
| * | |
| * @since 1.5.0 | |
| */ | |
| public function test_vendored_secrets_api_stands_down_on_core(): void { | |
| if ( ! file_exists( ABSPATH . WPINC . '/secrets.php' ) ) { | |
| $this->markTestSkipped( 'This WordPress version does not include the Secrets API.' ); | |
| } | |
| Key_Encryption::get_bridge()->ensure_secrets_api(); | |
| $this->assertSame( ABSPATH . WPINC . '/secrets.php', ( new \ReflectionFunction( 'wp_get_secret' ) )->getFileName() ); | |
| $this->assertFalse( class_exists( 'Secrets_API_Prototype_Fallback_Store', false ) ); | |
| } |
4 participants
What?
Closes #1012
Moves the Key Encryption experiment onto the proposed WordPress Secrets API feature plugin (slated for WordPress 7.2 Core).
Why?
The Key Encryption experiment originally vendored a subset of
ericmann/displace-secrets-manager. That upstream repository has been archived and superseded byericmann/secrets-api.To align with the WordPress 7.2 Core roadmap and avoid data loss for existing sites:
_secret_ai/*) need an automated migration because the new Secrets API prototype fallback only maps unnamespaced names.wp_get_secret()and friends when present, while retaining the bundled internal provider as a zero-dependency fallback so the "zero-install" experience remains intact.How?
Secrets_Bridge Integration:
get_secret(),set_secret(), anddelete_secret()methods toSecrets_Bridge.wp_get_secret(),wp_set_secret(), andwp_delete_secret()under theai/namespace with caller context['plugin' => 'ai'].Automated Migration (
V1_5_0):V1_5_0upgrade routine inincludes/Admin/Upgrades/V1_5_0.phpand registered it inUpgrades.php._secret_ai/*options into memory, writes towp_set_secret(), verifies read-back matches, and only then deletes the legacy option row.Uninstall Cleanup:
wp_delete_secret()upon plugin uninstallation.Tests & Docs:
V1_5_0Test.phpand updatedKey_EncryptionTest.php.docs/experiments/key-encryption.mdandincludes/Vendor/Secrets/README.md.Use of AI Tools
AI assistance: Yes
Tool(s): Google Antigravity
Model(s): Gemini 3.8 Flash
Used for: Implementation, migration routine, and integration tests;
I have reviewed and verified it locally with unit tests and static analysis.
Testing Instructions
wp_get_secret()._secret_ai/*rows: verify theV1_5_0migration moves keys to the new Secrets API without data loss or user intervention.decrypt_all().Changelog Entry