Automattic / Automattic/wordpress-activitypub

Signature::get_key_pair() persists during a read and has no in-process memo

Open
#3,766 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
PHP
Stars
579
Forks
92
Avg merge
13h 5m
Merged PRs (30d)
85

Description

### Description

`Signature::get_key_pair()` (`includes/class-signature.php`) performs an `update_option()` as part of what is otherwise a read:

```php
$key_pair = \get_option( $option_key );

if ( $key_pair ) {
return $key_pair;
}

$key_pair = $legacy_callback ? $legacy_callback() : false;

if ( ! $key_pair ) {
$key_pair = self::generate_key_pair();
// ...
}

\update_option( $option_key, $key_pair );

return $key_pair;
```

The stored option row is the only cache. There is no static memo, object-cache entry or transient in `Signature`, `Application` or `Actors` — `Application::get_keypair()` and `Actors::get_keypair()` are thin pass-throughs, and `Application::get_public_key()` / `get_private_key()` each call `get_keypair()` separately, so a caller that needs both pays twice.

Two consequences:

**1. A read can create state, on whichever site happens to be current.** `Http::get()` builds `key_id` / `private_key` from `Application::get_private_key()` eagerly on every uncached request, so an ordinary outbound fetch can mint a keypair. On a multisite host, plugin code may run before the request has been switched to the site it is serving — in that window the row is written to the wrong site. Because generation is non-deterministic, that site ends up with a *different* keypair rather than a stray copy of the right one.

**2. If the write does not stick, generation repeats forever.** Since `get_option()` is the only memo, a persist that is filtered, blocked or simply fails means `generate_key_pair()` runs again on the very next call. That is an unconditional 2048-bit RSA keygen (`openssl_pkey_new`, `sha512`) — I measured ~280 ms per keypair, with the long tail RSA keygen normally has. Inbound signature verification resolves remote actors through several `Http::get()` calls per request, so this compounds quickly.

### Suggested change

- Add a static memo keyed on `$option_key`, so repeated calls within a request cost nothing regardless of whether the row exists.
- Separate "read the keypair" from "generate and persist one", so a caller that only wants to read cannot trigger a write as a side effect.

The memo alone is a clear win independent of the multisite angle.

Contributor guide

Open the contributing guide

Research direction

Start in includes/class-signature.php, then trace the pass-through methods in Application and Actors and the eager key access in Http::get(). Verify repeated calls reuse the in-process result, read-only access does not persist or generate a keypair, and generation still persists one when required.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
backend, security
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.