This repository was archived by the owner on Jan 6, 2023. It is now read-only.
Verify AppIdentityService signatures against all valid certificates#50
Open
wiz wants to merge 1 commit intoGoogleCloudPlatform:masterfrom
Open
Verify AppIdentityService signatures against all valid certificates#50wiz wants to merge 1 commit intoGoogleCloudPlatform:masterfrom
wiz wants to merge 1 commit intoGoogleCloudPlatform:masterfrom
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
Author
|
@googlebot I signed it |
|
CLAs look good, thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[Background]
The
appengine-pluginin this repo overrides the built-in uploading functionality of WordPress to use a GCS bucket for the WP media library storage backend. The plugin implements an authentication method using the AppIdentityService::signForApp() method to generate a signature using the GAE project's private keys, and verifies media uploads with these signatures being passed via a query string argument.[Issue]
The authentication method currently verifies the signatures in each media upload by generating the same data to be signed, calling signForApp() and signing it, and comparing the user-provided signature against the one it generates for the request. However, this incorrectly assumes that the generated signature will be the same every time. Since GAE projects can sometimes have more than 1 private key, there can be multiple valid signatures, so this causes the authentication method to randomly fail. As a result, when a user uploads multiple media files, the uploads will fail and the user will get logged out of the WP admin panel.
To quote the AppIdentityService PHP documentation: "Since private keys are rotated periodically, getPublicCertificates() could return a list of public certificates. It's the caller's responsibility to try these certificates one by one when doing signature verification."
[Fix]
This pull request modifies the authentication method to verify a given signature against the correct key.