-
Notifications
You must be signed in to change notification settings - Fork 81
Signatures: Add basic support for RFC-9421 #1849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
Refactored the signature verification logic in Signature::verify_http_signature to delegate to new standard-specific classes. Added Draft_Cavage_Signature and Http_Message_Signature classes implementing a new Signature_Standard interface, supporting both the legacy draft-cavage and new HTTP Message Signature (RFC 9421) standards. Deprecated and stubbed out legacy parsing and verification helpers in class-signature.php. This improves maintainability and prepares for broader signature standard support.
Refactored the signature verification logic to support multiple signature labels by parsing and verifying each label individually. Extracted parsing and verification steps into dedicated private methods for better modularity and maintainability. Improved error handling and signature base string construction.
Replaces unqualified WP_Error references with fully qualified \WP_Error in the Signature class.
Renamed class-draft-cavenage-signature.php to class-draft-cavage-signature.php to correct a typo. Updated date header handling for better clarity and reliability, and fixed regex modifiers for 'created' and 'expires' fields.
Moved digest verification logic into a new private method verify_content_digest, which now supports the Content-Digest header with multiple algorithms.
This update adds handling for the '@scheme' and '@request-target' pseudo-headers in the HTTP message signature base construction. It also improves query string handling and ensures correct formatting of signature parameters, quoting non-numeric values as needed.
Adds support for the @query-param component in signature base string construction and ensures all components are quoted in the @signature-params field. Also updates parameter quoting to properly escape backslashes and double quotes per RFC 9421.
Done:
Skipped for now:
What do you think @mediaformat? |
Introduced a call to reset the $_SERVER superglobal in tear_down to ensure test isolation. Also set specific $_SERVER values in the signature header test to better simulate HTTP request context.
Refines parsing of signature components to support query parameters per RFC-9421, updates signature base construction, and enhances test coverage for GET requests with query parameters. Also updates test URIs to use dynamic REST prefixes and namespaces for better compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds modular support for both the legacy Draft Cavage and new RFC-9421 HTTP Message Signature standards by refactoring the centralized verification logic into standard‐specific classes.
- Delegates signature verification in
Signature::verify_http_signature
toDraft_Cavage_Signature
andHttp_Message_Signature
via a newSignature_Standard
interface. - Implements
Http_Message_Signature
(RFC-9421) andDraft_Cavage_Signature
classes, each handling parsing, validation, and verification for their respective standards. - Deprecates and stubs out the legacy parsing and verification helpers in
class-signature.php
.
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
tests/includes/rest/class-test-signature-verification.php | Removed legacy HTTP signature tests; needs replacement for new classes |
tests/fixtures/http-signature-keys.json | Added key fixtures for EC and RSA variants used by new signature tests |
includes/signature/interface-signature-standard.php | Introduced Signature_Standard interface for pluggable verification |
includes/signature/class-http-message-signature.php | Added RFC-9421 HTTP Message Signature implementation |
includes/signature/class-draft-cavage-signature.php | Added Draft Cavage HTTP Signature implementation |
includes/class-signature.php | Updated verify_http_signature to dispatch to new standards; stubbed legacy methods |
Comments suppressed due to low confidence (2)
includes/class-signature.php:251
- [nitpick] The variable name
$signature
shadows the concept of the header and may be confusing; consider renaming it to$verifier
or$signature_standard
for clarity.
$signature = isset( $headers['signature_input'] ) ? new Http_Message_Signature() : new Draft_Cavage_Signature();
Co-authored-by: Copilot <[email protected]>
@@ -59,6 +59,7 @@ jobs: | |||
{"path": "includes/class-blocks.php", "label": "[Focus] Editor"}, | |||
{"path": "includes/collection", "label": "[Feature] Collections"}, | |||
{"path": "includes/rest", "label": "[Feature] REST API"}, | |||
{"path": "includes/signature", "label": "[Feature] Signature"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{"path": "includes/signature", "label": "[Feature] Signature"}, | |
{"path": "includes/signature", "label": "[Feature] Signature"}, | |
{"path": "includes/class-signature.php", "label": "[Feature] Signature"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was planning on moving class-signature.php
into /includes/signature
in a follow-up PR, would that work for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think best practice is to have the main class (or factory or however you want to call it) in the root and only the special cases in the subfolder, so I quite like the structure as is. that said, we havent't done that with transformers, so it would be at least consistent to move it 🫣
Updated both Draft_Cavage_Signature and Http_Message_Signature classes to consistently use fully qualified PHP function names (e.g., \is_wp_error, \explode, \array_map). Changed several protected methods to private for encapsulation. Moved RSA-PSS algorithm version check into the verify_algorithm method for better separation of concerns and renamed resolve_algorithm to verify_algorithm for clarity.
Moved strtolower call to the start of verify_algorithm to ensure the algorithm string is always compared in lowercase. This prevents case sensitivity issues when matching algorithm names.
Only looked at the code so far, but looks great! |
It was missing wrapping colons.
@pfefferle I think this is ready for another look. Unit tests look firm now and #1858 helped polish out some remaining bugs. As long as legacy signatures still work this should be safe to merge. |
Fixes #1808.
Proposed changes:
Signature::verify_http_signature
to delegate to new standard-specific classes.Draft_Cavage_Signature
andHttp_Message_Signature
classes implementing a newSignature_Standard
interface, supporting both the legacy draft-cavage and new HTTP Message Signature (RFC 9421) standards.class-signature.php
.Other information:
Testing instructions:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Added basic support for RFC-9421 style signatures for incoming activities.