Azure / azure-sdk-for-js

This repository is for active development of the Azure SDK for JavaScript (NodeJS & Browser). For consumers of the SDK we recommend visiting our public developer docs at https://docs.microsoft.com/javascript/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-js.
MIT License
2.09k stars 1.21k forks source link

Add `workerd` conditional export to allow ClientCertificateCredential #31467

Open Manouchehri opened 1 month ago

Manouchehri commented 1 month ago

Is your feature request related to a problem? Please describe. I'm trying to use ClientCertificateCredential from a Cloudflare Worker, but cannot due to the fact that ClientCertificateCredential insists reading from a file (which a Worker doesn't have).

Describe the solution you'd like Allow passing in a string of the private certificate key instead of a file path for ClientCertificateCredential.

Describe alternatives you've considered I can currently use ClientSecretCredential in my Worker, but this isn't great as I have to remember to renew the secret key every other year.

github-actions[bot] commented 1 month ago

@KarishmaGhiya @maorleger

github-actions[bot] commented 1 month ago

Thank you for your feedback. Tagging and routing to the team member best able to assist.

xirzec commented 1 month ago

It feels like cloudflare workers should be treated more like a server environment rather than a browser one, given that we don't have a node fs API here there should be a mechanism to read in stuff from their secrets system: https://developers.cloudflare.com/workers/configuration/secrets/

Manouchehri commented 1 month ago

It feels like cloudflare workers should be treated more like a server environment rather than a browser one,

Agreed.

there should be a mechanism to read in stuff from their secrets system:

Secrets are read in just like normal strings. Nothing special needs to be done, aside from allowing the private key to be passed in as a string.

mpodwysocki commented 1 month ago

@Manouchehri this should be possible with the following:

const tenantId = "<tenant-id>";
const clientId = "<client-id>";

const credential = new ClientCertificateCredential(tenantId, clientId, {
  certificate: "<contents-of-certificate>",
  certificatePassword: "<optional-password>"
});

Is this what you are looking for?

Manouchehri commented 1 month ago

Is this what you are looking for?

Yes. It doesn't actually work though when you try to use your code in Cloudflare Worker.

Error: ClientCertificateCredential is not supported in the browser.
mpodwysocki commented 4 weeks ago

@Manouchehri how are you bringing in the package? In order to pull in the browser version, you must have some sort of bundler issue where it's pulling in the browser version of the library instead of the Node version.

Manouchehri commented 4 weeks ago

Cloudflare Workers will be bringing in the browser version, as they do not use the Node.js runtime. They use directly V8 as their engine, and are more similar to a browser than Node.

mpodwysocki commented 3 weeks ago

@Manouchehri yes but you can use the npm compat as shown here https://developers.cloudflare.com/workers/runtime-apis/nodejs/ which would allow you to use our Node bundle

Manouchehri commented 3 weeks ago

Enabling Node.js compatibility doesn't fix the error mentioned in https://github.com/Azure/azure-sdk-for-js/issues/31467#issuecomment-2430383711.

Manouchehri commented 3 weeks ago

Even if I force the node package for Azure, and enable nodejs_compat, that brings a new error to the table. I feel this is really the wrong path to go down and will be much more difficult to fix than just allowing ClientCertificateCredential in the browser.

✘ [ERROR] service core:user:azure-openai-entra-demo-worker-dev: Uncaught ReferenceError: __dirname is not defined

    at null.<anonymous> (index.js:31417:38) in node_modules/open/index.js
    at null.<anonymous> (index.js:25:50) in __require
    at null.<anonymous> (index.js:31752:17) in node_modules/@azure/identity/dist/index.js
    at null.<anonymous> (index.js:25:50) in __require
    at null.<anonymous> (index.js:40954:31)
xirzec commented 3 weeks ago

@Manouchehri the main reason we don't allow ClientCertificateCredential in the browser is that if you deployed that credential as part of an app bundle that was served to a real web browser, you would be giving away your app credentials to every user of your web application. In general, we do not treat the browser as a trusted environment as a developer does not have the ability to securely store secrets.

The way we resolve this for other non-Node environments is providing a conditional export in package.json, e.g. for react-native: https://github.com/Azure/azure-sdk-for-js/blob/2404c52da2f86ef7bad7ec84dca33023d6335093/sdk/core/core-util/package.json#L18

It looks like we can use the workerd key to help Cloudflare bundle the right (non-browser) version of this credential: https://developers.cloudflare.com/workers/wrangler/bundling/#conditional-exports

Manouchehri commented 3 weeks ago

It looks like we can use the workerd key to help Cloudflare bundle the right (non-browser) version of this credential:

Did you manage to get that to work? :)

Manouchehri commented 1 week ago

Any progress on this? =)

KarishmaGhiya commented 3 days ago

@Manouchehri I am working on this right now. Will give you updates on it.

Can you have a look at ClientAssertionCredential - we believe that might be more secure than extending ClientCertificateCredential for passing in a string of the private certificate key. You'll need to provide a callback function to ClientAssertionCredential that returns an assertion from the certificate. This will solve the hassle of needing to rotate secrets periodically.

github-actions[bot] commented 3 days ago

Hi @Manouchehri. Thank you for opening this issue and giving us the opportunity to assist. To help our team better understand your issue and the details of your scenario please provide a response to the question asked above or the information requested above. This will help us more accurately address your issue.

Manouchehri commented 3 days ago

@KarishmaGhiya I have no idea how I would use ClientAssertionCredential with a private key. Are you suggesting I re-implement all of ClientCertificateCredential myself...? Rolling my own crypto is really not a good idea..

xirzec commented 2 days ago

@KarishmaGhiya I have no idea how I would use ClientAssertionCredential with a private key. Are you suggesting I re-implement all of ClientCertificateCredential myself...? Rolling my own crypto is really not a good idea..

We have an example of how you might implement the callback here: https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/identity/identity/samples/AzureIdentityExamples.md#creating-a-callback-function-to-return-assertion

You could skip reading it from a file if you have direct access to the certificate containing the private key and most of the heavy lifting for creating the JWT is handled by jsonwebtoken.

I also found this project which seems like it could be helpful: https://www.npmjs.com/package/@tsndr/cloudflare-worker-jwt

Manouchehri commented 2 days ago

This looks way too complicated. To be blunt, nobody is going to do that. If I wanted to implement my own callback function, I wouldn't be using this SDK to begin with.

Yes, I have seen that repo before. I have also written my own OIDC server (and client) in pure TypeScript running on Workers as well.. But for this project, I just want to use Azure's SDK with no fuss.