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.08k stars 1.2k forks source link

identity: ClientCertificateCredential with secret key in TPM? #22011

Closed nwf-msr closed 5 months ago

nwf-msr commented 2 years ago

Is your feature request related to a problem? Please describe.

The ClientCertificateCredential implementation requires that the application itself handle secret keying material, either as a string directly or indirectly as a file name that is, ultimately, opened and turned into a string internally. This, in turn, implies that the system on which the application is running has cleartext access to the secret material, at least while running, even if it's stored under encryption at rest. This creates low-hanging risks for the secret key: it could be disclosed due to system misconfiguration (such as being accessible to a co-located service or user account) or due to bugs in the application (and/or its runtime).

Describe the solution you'd like

For (virtual) machines with (v)TPMs, HSMs, or other PKCS#11 providers, it would be a significant improvement to have the application not handle secret key material at all, only exchanging key identifiers and messages to be signed with the secure hardware. FWIW, Azure VMs generally support vTPMs (https://docs.microsoft.com/en-us/azure/virtual-machines/trusted-launch), as does HyperV (under which my motivating application is running).

Rather than implement this functionality directly in JS or linked native code, as identity authentication is performed only rarely, it may be simplest to offer a new Credential type that spawns an external program, writes the JWT to be signed to (and then closes) its stdin, and reads the signed response from its stdout. If spawning a program is too much overhead, then instead making a socket connection (to a SOCK_STREAM UNIX domain socket, especially) and using write(); shutdown(); read(); close() similarly, would also work fine.

This will probably require cooperation with msal, which looks like it ultimately outsources JWT work to the jsonwebtoken package?

I am unsure where, exactly, in this stack is the right intercept. In node-jsonwebtoken itself might be ideal, for broadest impact, but it looks like several places have their own ideas about the well-formedness of PEM-encoded keys, which will necessitate rework.

Describe alternatives you've considered

At the moment, because the secret keys are stored in files, they are roughly equivalent to client secrets for most threat models (nobody is realistically MITMing or decrypting our TLS session with Azure), and so we are using a ClientSecretCredential instead, with the secret held in a file available only to the application (and the root user), but this is less than ideal.

xirzec commented 2 years ago

@mpodwysocki @KarishmaGhiya this sounds like an interesting scenario - can you help with guidance?

nwf-msr commented 2 years ago

A follow-up with a more specific avenue that may be worth considering. The OpenSSH agent protocol is widely spoken by cryptographic service providers, apparently fairly easy to speak from JS, as seen in https://github.com/131/ssh-agent-js for example, and capable of signing JWTs, as demonstrated by https://github.com/ptxmac/ssh-jwt. There's even support in OpenSSH's implementation for using PKCS#11 tokens and specifically the TPM as such: https://blog.ledger.com/ssh-with-tpm/ .

KarishmaGhiya commented 2 years ago

@nwf-msr We do not currently support protected certificate or key storage and we don't have any immediate plans to add support. But I will convert this issue to a feature request for a new credential type that creates the assertion with a callback to allow the application to sign the assertion.

We will be releasing a new GA for @azure/identity 2.1.0 with client assertion credential support. With that you could build and sign the assertion yourself for the hardware protected key. Let me know if you have any questions.

ghost commented 2 years ago

Hi, we're sending this friendly reminder because we haven't heard back from you in a while. We need more information about this issue to help address it. Please be sure to give us your input within the next 7 days. If we don't hear back from you within 14 days of this comment the issue will be automatically closed. Thank you!

nwf-msr commented 2 years ago

Pardon, I am not sure why this is tagged as needs-author-feedback. Thank you for converting this into an appropriate feature request. Thanks, too, for the heads up about @azure/identity 2.1.0; could you link me to / will there be an example of using client assertion credential support this way?

KarishmaGhiya commented 2 years ago

Hi @nwf-msr I will add code samples for using client assertion credential. We are planning to release it in July.

nwf-msr commented 2 years ago

I've thought about this a bit more, off and on, and have proposed extending the node.js crypto API to handle the case of keys in HSMs more directly. As I wrote over there, this approach looks like it would work well all the way up through msal, but the ad-hoc validation logic in https://github.com/Azure/azure-sdk-for-js/blob/191e4ce330acd60e014c13412204b9598870cfa1/sdk/identity/identity/src/msal/nodeFlows/msalClientCertificate.ts#L83-L90 poses a challenge. I suspect this means that the SDK will need a new nodeFlow class to take advantage of this new API when and if it lands, and that SDK consumers will have to explicitly support this flow. (By contrast, GitHub's OctoKit just plumbs the private key object all the way down to the JWT signer and so, at a glance, seems to need no or very little change to support this potential new functionality.)

KarishmaGhiya commented 2 years ago

@nwf-msr We have released the ClientAssertion Credential - https://www.npmjs.com/package/@azure/identity in the 2.1.0 release. Please feel free to check it out and give us feedback. It won't actually solve the scenario that you are looking for, but your feedback will help us formulate the feature request that you initially asked for.

KarishmaGhiya commented 1 year ago

@nwf-msr Wanted to follow up, does ClientAssertionCredential provide some help with your scenario? Let me know if you have questions

nwf-msr commented 1 year ago

With apologies, I haven't had time to investigate. It seems like it should, but I haven't jumped back into trying to make the TPM and nodejs play nicely together.

github-actions[bot] commented 1 year ago

Hi @nwf-msr. 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.

github-actions[bot] commented 1 year ago

Hi @nwf-msr, we're sending this friendly reminder because we haven't heard back from you in 7 days. We need more information about this issue to help address it. Please be sure to give us your input. If we don't hear back from you within 14 days of this comment the issue will be automatically closed. Thank you!

nwf-msr commented 1 year ago

Sorry, with the recent redeployments, I've been transferred to another team and have had all my work in this area deprioritized and/or cancelled, so I'm afraid I probably will not have time to spend on it. Which is a shame, this feature could have been a really nice byproduct for Azure customers.

xirzec commented 1 year ago

Sorry, with the recent redeployments, I've been transferred to another team and have had all my work in this area deprioritized and/or cancelled, so I'm afraid I probably will not have time to spend on it. Which is a shame, this feature could have been a really nice byproduct for Azure customers.

Sorry to hear, but we'll keep this feature request open since I could see a very security-conscious cloud customer being interested in such functionality in the future.

github-actions[bot] commented 5 months ago

Hi @nwf-msr, we deeply appreciate your input into this project. Regrettably, this issue has remained unresolved for over 2 years and inactive for 30 days, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support.