Skip to content

Fix handling for X509 Certificates using EC algorithms#6522

Merged
danlapid merged 2 commits into
cloudflare:mainfrom
Madrona-Games:madrona/fix-ec-curves
Apr 20, 2026
Merged

Fix handling for X509 Certificates using EC algorithms#6522
danlapid merged 2 commits into
cloudflare:mainfrom
Madrona-Games:madrona/fix-ec-curves

Conversation

@AndrewKahr

@AndrewKahr AndrewKahr commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

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 AsymmetricKey type. When a key backed by the crypto.subtle type was used, it would result in a null. This commit now handles crypto.subtle-based keys in CryptoImpl::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 sign or signOneShot should be used, where sign properly handles the ECDSA case.

Testing

Functionality was verified by passing all Bazel tests and by locally running a build of workerd with the changes in this PR using wrangler and 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

@AndrewKahr AndrewKahr requested review from a team as code owners April 8, 2026 07:19
@github-actions

github-actions Bot commented Apr 8, 2026

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@AndrewKahr

Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

github-actions Bot added a commit that referenced this pull request Apr 8, 2026
@AndrewKahr AndrewKahr force-pushed the madrona/fix-ec-curves branch 2 times, most recently from 4b9bf4b to ed2e567 Compare April 9, 2026 02:44
@AndrewKahr AndrewKahr force-pushed the madrona/fix-ec-curves branch from ed2e567 to 489a4f4 Compare April 9, 2026 12:29
@AndrewKahr AndrewKahr changed the title Fix EC curve algorithm selection bug Fix handling for X509 Certificates using EC algorithms Apr 9, 2026
@AndrewKahr AndrewKahr force-pushed the madrona/fix-ec-curves branch from 489a4f4 to 27ea0a9 Compare April 9, 2026 13:09
@AndrewKahr

Copy link
Copy Markdown
Contributor Author

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!

@danlapid

Copy link
Copy Markdown
Collaborator

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.

@AndrewKahr

Copy link
Copy Markdown
Contributor Author

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!

@pat-in-a-hat

pat-in-a-hat commented Apr 17, 2026

Copy link
Copy Markdown

It would be great to get this merged, its a big issue if you're using Cloudflare Workers to handle Apple IAP processing

@Alkenso

Alkenso commented Apr 17, 2026

Copy link
Copy Markdown

+1, waiting for this fix a lot
It is also required for Apple AppAttest/DeviceCheck security checks

@AndrewKahr

Copy link
Copy Markdown
Contributor Author

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!

@AndrewKahr

Copy link
Copy Markdown
Contributor Author

Thanks for rerunning @npaun! Looks like it's all green now

@danlapid danlapid merged commit c2352c6 into cloudflare:main Apr 20, 2026
19 of 22 checks passed
@AndrewKahr

Copy link
Copy Markdown
Contributor Author

Thanks again @danlapid!

@Alkenso

Alkenso commented Apr 21, 2026

Copy link
Copy Markdown

@AndrewKahr , I still experience the error on such code

const APPLE_APP_ATTESTATION_ROOT_CA = new X509Certificate(
      "-----BEGIN CERTIFICATE-----\nMIICITCCAaegAwIBAgIQC/O+DvHN0uD7jG5yH2IXmDAKBggqhkjOPQQDAzBSMSYwJAYDVQQDDB1BcHBsZSBBcHAgQXR0ZXN0YXRpb24gUm9vdCBDQTETMBEGA1UECgwKQXBwbGUgSW5jLjETMBEGA1UECAwKQ2FsaWZvcm5pYTAeFw0yMDAzMTgxODMyNTNaFw00NTAzMTUwMDAwMDBaMFIxJjAkBgNVBAMMHUFwcGxlIEFwcCBBdHRlc3RhdGlvbiBSb290IENBMRMwEQYDVQQKDApBcHBsZSBJbmMuMRMwEQYDVQQIDApDYWxpZm9ybmlhMHYwEAYHKoZIzj0CAQYFK4EEACIDYgAERTHhmLW07ATaFQIEVwTtT4dyctdhNbJhFs/Ii2FdCgAHGbpphY3+d8qjuDngIN3WVhQUBHAoMeQ/cLiP1sOUtgjqK9auYen1mMEvRq9Sk3Jm5X8U62H+xTD3FE9TgS41o0IwQDAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBSskRBTM72+aEH/pwyp5frq5eWKoTAOBgNVHQ8BAf8EBAMCAQYwCgYIKoZIzj0EAwMDaAAwZQIwQgFGnByvsiVbpTKwSga0kP0e8EeDS4+sQmTvb7vn53O5+FRXgeLhpJ06ysC5PrOyAjEAp5U4xDgEgllF7En3VcE3iexZZtKeYnpqtijVoyFraWVIyd/dganmrduC1bmTBGwD\n-----END CERTIFICATE-----",
    );
console.log(APPLE_APP_ATTESTATION_ROOT_CA.publicKey);

error

NotSupportedError: Unrecognized or unimplemented EC curve "id-ecPublicKey" requested.
 ❯ X509Certificate.get publicKey [as publicKey] node-internal:crypto_x509:210:40

@AndrewKahr

Copy link
Copy Markdown
Contributor Author

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!

@Alkenso

Alkenso commented Apr 21, 2026

Copy link
Copy Markdown

Yes, seems my bad. Updated locally all dependencies and everything works.

@npaun

npaun commented Apr 21, 2026

Copy link
Copy Markdown
Member

I think it should be in prod within a week. I'll post here when it's live.

@pat-in-a-hat

pat-in-a-hat commented May 29, 2026

Copy link
Copy Markdown

I think it should be in prod within a week. I'll post here when it's live.

@npaun any update here?

@npaun

npaun commented May 29, 2026

Copy link
Copy Markdown
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐛 Bug Report — Runtime APIs: implement id-ec-PublicKey crypto curve to allow Apple App Store Server API

6 participants