Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

identity: ClientCertificateCredential with secret key in TPM? #22011

Closed
nwf-msr opened this issue May 26, 2022 · 15 comments
Closed

identity: ClientCertificateCredential with secret key in TPM? #22011

nwf-msr opened this issue May 26, 2022 · 15 comments
Assignees
Labels
Azure.Identity Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. feature-request This issue requires a new behavior in the product in order be resolved.
Milestone

Comments

@nwf-msr
Copy link
Contributor

nwf-msr commented May 26, 2022

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.

@ghost ghost added needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels May 26, 2022
@azure-sdk azure-sdk added Azure.Identity Client This issue points to a problem in the data-plane of the library. needs-team-triage Workflow: This issue needs the team to triage. labels May 26, 2022
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label May 26, 2022
@xirzec xirzec removed the needs-team-triage Workflow: This issue needs the team to triage. label May 26, 2022
@xirzec
Copy link
Member

xirzec commented May 26, 2022

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

@nwf-msr
Copy link
Contributor Author

nwf-msr commented May 27, 2022

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
Copy link
Contributor

KarishmaGhiya commented Jun 3, 2022

@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.

@KarishmaGhiya KarishmaGhiya added the feature-request This issue requires a new behavior in the product in order be resolved. label Jun 3, 2022
@ghost ghost added the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Jun 3, 2022
@KarishmaGhiya KarishmaGhiya added needs-author-feedback Workflow: More information is needed from author to address the issue. and removed needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team labels Jun 3, 2022
@ghost ghost added the no-recent-activity There has been no recent activity on this issue. label Jun 11, 2022
@ghost
Copy link

ghost commented Jun 11, 2022

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
Copy link
Contributor Author

nwf-msr commented Jun 12, 2022

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?

@ghost ghost added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team and removed needs-author-feedback Workflow: More information is needed from author to address the issue. no-recent-activity There has been no recent activity on this issue. labels Jun 12, 2022
@KarishmaGhiya
Copy link
Contributor

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

@KarishmaGhiya KarishmaGhiya removed the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Jun 13, 2022
@nwf-msr
Copy link
Contributor Author

nwf-msr commented Jul 3, 2022

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

// Match all possible certificates, in the order they are in the file. These will form the chain that is used for x5c
let match;
do {
match = certificatePattern.exec(certificateParts.certificateContents);
if (match) {
publicKeys.push(match[3]);
}
} while (match);
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
Copy link
Contributor

@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 KarishmaGhiya added this to the 2022-12 milestone Jul 18, 2022
@KarishmaGhiya KarishmaGhiya modified the milestones: 2022-12, Backlog Jul 18, 2022
@KarishmaGhiya
Copy link
Contributor

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

@nwf-msr
Copy link
Contributor Author

nwf-msr commented Dec 8, 2022

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.

@KarishmaGhiya KarishmaGhiya added the needs-author-feedback Workflow: More information is needed from author to address the issue. label Jun 20, 2023
@github-actions
Copy link

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
Copy link

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!

@github-actions github-actions bot added the no-recent-activity There has been no recent activity on this issue. label Jun 28, 2023
@nwf-msr
Copy link
Contributor Author

nwf-msr commented Jun 28, 2023

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.

@github-actions github-actions bot added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team and removed needs-author-feedback Workflow: More information is needed from author to address the issue. no-recent-activity There has been no recent activity on this issue. labels Jun 28, 2023
@xirzec xirzec removed question The issue doesn't require a change to the product in order to be resolved. Most issues start as that needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team labels Jun 28, 2023
@xirzec
Copy link
Member

xirzec commented Jun 28, 2023

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.

@joshfree joshfree assigned maorleger and unassigned mpodwysocki Feb 5, 2024
Copy link

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.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 27, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Identity Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. feature-request This issue requires a new behavior in the product in order be resolved.
Projects
Development

No branches or pull requests

6 participants