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] Reduce scope of native dependency "keytar" #13950

Closed
mikeharder opened this issue Feb 23, 2021 · 3 comments
Closed

[Identity] Reduce scope of native dependency "keytar" #13950

mikeharder opened this issue Feb 23, 2021 · 3 comments
Assignees
Labels
Azure.Identity blocking-release Blocks release Client This issue points to a problem in the data-plane of the library.
Milestone

Comments

@mikeharder
Copy link
Member

The "Rush" build orchestrator recently added a dependency on @azure/identity, but they would like to avoid taking a transitive dependency on keytar which is a native module. (microsoft/rushstack#2492)

Other users have reported issues caused by the keytar dependency (example #9288). As a result of this issue, keytar was changed from a required to optional dependency (#10142).

This does make it possible to use @azure/identity without installing keytar, but the dependency is still problematic. NPM tries to install optional dependencies by default. They can be skipped by specifying --no-optional, but this isn't very useful in practice since every user installing the package (or anything that depends on it) would need to set this.

If the keytar dependency cannot be removed completely, perhaps @azure/identity could be split into multiple packages, so only the package(s) containing APIs requiring keytar would have the dependency. @azure/identity itself would become a meta-package to avoid a breaking change.

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Feb 23, 2021
@sadasant sadasant self-assigned this Feb 24, 2021
@sadasant sadasant added this to the [2021] April milestone Feb 24, 2021
@octogonz
Copy link

We would also be fine with a design where the keytar object is passed to the APIs that need to use it. Something like:

const keytar = require('keytar');
const { DefaultAzureCredential } = require("@azure/identity");

const credential = new DefaultAzureCredential({
  keytarLibrary: keytar
});

This way the consumer of @azure/identity could decide whether they want the keytar dependency or not (or under what conditions they want to install it). In Rush's case I don't believe that we access any credentials from the OS key vault, so we don't need keytar at all.

CC @iclanton

@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Feb 24, 2021
@ramya-rao-a ramya-rao-a added the Client This issue points to a problem in the data-plane of the library. label Feb 24, 2021
@octogonz
Copy link

volta-cli/volta#906 (comment) is saying that Volta cannot be used with Rush unless we can eliminate the native dependencies.

@witemple-msft
Copy link
Member

Closing in favor of tracking #14346

We'll handle this in the same way as we handle msal-node-extensions, since it's shaping up to be that we'll have a separate package that allows us to add these native dependency modules dynamically.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Identity blocking-release Blocks release Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

No branches or pull requests

5 participants