Fix handling for X509 Certificates using EC algorithms#6522
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
4b9bf4b to
ed2e567
Compare
ed2e567 to
489a4f4
Compare
489a4f4 to
27ea0a9
Compare
|
Hey @jasnell, thanks for taking the time to review my PR! Are there any additional actions required on my end to get this pushed through? Eager to get this merged so we are unblocked on our worker development. Thanks! |
|
Hey @AndrewKahr , I just approved the CI workflow to run, if it everything besides "Run internal build" passes then we should be good to go here. |
|
Hey @danlapid, thanks for getting the workflow going! Looks like everything passed except the coverage job. The logs seem to point to a 500 error from the cache server that caused a cascading failure in the Bazel build, so I'm thinking it just got unlucky and needs a rerun? If you could restart the job, that would be super helpful! Unfortunately, it doesn't look like I can rerun jobs even if the original check job was approved. If this needs code changes, please let me know what to look for, as I'm a bit lost on what else could be wrong since the tests themselves pass. Thanks! |
|
It would be great to get this merged, its a big issue if you're using Cloudflare Workers to handle Apple IAP processing |
|
+1, waiting for this fix a lot |
|
Hey @danlapid, sorry to poke again, just wanted to bump this. Would you be able to retry the coverage workflow since it looks like Bazel itself crashed and not a test failure? I was able to locally run coverage and it passed so I'm thinking this is just a transient issue. Thanks! |
|
Thanks for rerunning @npaun! Looks like it's all green now |
|
Thanks again @danlapid! |
|
@AndrewKahr , I still experience the error on such code error |
|
I'd assume at a minimum cloudflare/workers-sdk#13615 would need to be merged and released for us to at least start seeing it work locally. Not entirely sure how long it takes for releases in this repo to roll out to the cloud hosted version though. I unfortunately could not find anything online about the lag between releases in this repo and the changes reflected in the hosted version. If a Cloudflare engineer familiar with this could color that in, that would be super helpful! |
|
Yes, seems my bad. Updated locally all dependencies and everything works. |
|
I think it should be in prod within a week. I'll post here when it's live. |
@npaun any update here? |
|
My apologies for the late reply -- it's been a really busy period for us. It looks like this change has been live in production since 22 April 2026. |
Overview
This PR resolves issues involving X509 Certificates using Elliptic Curve algorithms. The motivation for this implementation is driven by the NodeJS Apple App Store Server Library not functioning in the workerd environment due to a couple of bugs in handling ECDSA certificates. This PR is composed of 2 commits, each of which resolves a portion of ECDSA certificate handling, and combined, brings compatibility for the App Store Server Library in Cloudflare Workers.
Commit 1
The original implementation only inspected the top-level key, which, for EC algorithms, only provided a generic value:
id-ecPublicKey. The actual curve spec is actually nested a layer below. This commit correctly inspects the inner field and pulls the correct algorithm. Additionally, a test for a test ECDSA cert with curve P-384 was created, mimicking the RSA-based test.Commit 2
The original implementation only handled certs using the
AsymmetricKeytype. When a key backed by thecrypto.subtletype was used, it would result in a null. This commit now handlescrypto.subtle-based keys inCryptoImpl::tryGetKey, which by extension allows them to be used in sign/verify functions.Additionally, while introducing round-trip sign/verify tests for ECDSA-based keys, it was discovered that the original implementation would fail for ECDSA keys because their DER-encoded signatures are potentially shorter than the maximum estimated size. This commit resolves this by checking the key type and determining if
signorsignOneShotshould be used, wheresignproperly handles the ECDSA case.Testing
Functionality was verified by passing all Bazel tests and by locally running a build of
workerdwith the changes in this PR usingwranglerand validating that the various functions in the App Store Server Library that previously failed due to certificate validation errors are now working properly.This should resolve #3622