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

[rush] Can we eliminate Rush's dependency on the "keytar" package? #2492

Closed
octogonz opened this issue Feb 17, 2021 · 15 comments · Fixed by #2647
Closed

[rush] Can we eliminate Rush's dependency on the "keytar" package? #2492

octogonz opened this issue Feb 17, 2021 · 15 comments · Fixed by #2647
Labels
bug Something isn't working as intended external issue The root cause is with an external component that needs a fix or workaround

Comments

@octogonz
Copy link
Collaborator

Summary

In #2467 (comment) , @HipsterZipster wrote:

Please eliminate the keytar dependency somehow. For those of us working in locked-down corp environments that can't easily pull prebuilt native libraries from the internet, this is a major pain. We specifically avoid libraries that require native dependencies unless absolutely necessary. Rush's pure JS approach was one of our big draws!

Details

I agree that Node.js native dependencies are not very desirable. Besides the security concerns with native code (which cannot be audited like .js files), there's also the problem that maintainers may not provide prebuilt binaries for newer Node.js releases as we encountered in #2467.

Up until recently, Rush has had no native dependencies. The keytar dependency was introduced recently by the new cloud build cache feature, for the purpose of authenticating with Azure.

Some questions:

  • Does Rush actually need keytar at all? Keytar provides access to an OS-specific credential store, but if I understand correctly, Rush's cloud build cache stores its credentials in a text file. So it seems we're not actually using the Azure SDK feature that requires keytar.
  • Does Rush actually need @azure/identity either? That package facilitates REST calls, but could we use a different API to make the REST calls, thus eliminating these dependencies? @azure/identity depends on 124 other packages comprising 17.3 MB and 2,518 files. So it would be generally beneficial to eliminate it from our install footprint.

Standard questions

Question Answer
@microsoft/rush globally installed version? 5.38.0
Operating system? Windows
Would you consider contributing a PR? Yes
Node.js version (node -v)? 14.15.3

CC @iclanton

@partounian
Copy link

This would be hugely appreciated. 🙏🏼 This also has broken the build for us on Netlify

@octogonz
Copy link
Collaborator Author

The upstream issue is Azure/azure-sdk-for-js#13950

@octogonz
Copy link
Collaborator Author

Another option: @mikeharder suggested that maybe we could use the browser-specific package @azure/storage-blob which is smaller than @azure/identity family of packages. Assuming that it will work with the Node.js runtime.

@kaiyoma
Copy link

kaiyoma commented Apr 22, 2021

I've run into this as well while trying to install Rush into a Docker image. No matter what I try, keytar throws permissions errors and refuses to build correctly. Can be repro'd with a pretty simple Docker image:

FROM node:14.15

SHELL ["/bin/bash", "-o", "pipefail", "-c"]

RUN npm install -g @microsoft/rush

Output includes these stubborn errors:

#5 40.14 > keytar@7.6.0 install /usr/local/lib/node_modules/@microsoft/rush/node_modules/keytar
#5 40.14 > prebuild-install || npm run build
#5 40.14
#5 40.38 prebuild-install WARN install EACCES: permission denied, access '/root/.npm'
#5 40.67 npm ERR! code EACCES
#5 40.68 npm ERR! syscall scandir
#5 40.68 npm ERR! path /root/.npm/_logs
#5 40.68 npm ERR! errno -13
#5 40.68 npm ERR!
#5 40.68 npm ERR! Your cache folder contains root-owned files, due to a bug in
#5 40.68 npm ERR! previous versions of npm which has since been addressed.
#5 40.68 npm ERR!
#5 40.68 npm ERR! To permanently fix this problem, please run:
#5 40.68 npm ERR!   sudo chown -R 65534:0 "/root/.npm"
#5 40.68 glob error [Error: EACCES: permission denied, scandir '/root/.npm/_logs'] {
#5 40.68   errno: -13,
#5 40.68   code: 'EACCES',
#5 40.68   syscall: 'scandir',
#5 40.68   path: '/root/.npm/_logs'
#5 40.68 }
#5 40.70
#5 40.70 > keytar@7.6.0 build /usr/local/lib/node_modules/@microsoft/rush/node_modules/keytar
#5 40.70 > node-gyp rebuild
#5 40.70
#5 41.10 gyp WARN EACCES current user ("nobody") does not have permission to access the dev dir "/root/.cache/node-gyp/14.15.0"
#5 41.10 gyp WARN EACCES attempting to reinstall using temporary dev dir "/usr/local/lib/node_modules/@microsoft/rush/node_modules/keytar/.node-gyp"
#5 41.10 gyp WARN install got an error, rolling back install
#5 41.10 gyp WARN install got an error, rolling back install
#5 41.10 gyp ERR! configure error
#5 41.11 gyp ERR! stack Error: EACCES: permission denied, mkdir '/usr/local/lib/node_modules/@microsoft/rush/node_modules/keytar/.node-gyp'
#5 41.11 gyp ERR! System Linux 5.4.72-microsoft-standard-WSL2
#5 41.11 gyp ERR! command "/usr/local/bin/node" "/usr/local/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
#5 41.11 gyp ERR! cwd /usr/local/lib/node_modules/@microsoft/rush/node_modules/keytar
#5 41.11 gyp ERR! node -v v14.15.0
#5 41.11 gyp ERR! node-gyp -v v5.1.0
#5 41.11 gyp ERR! not ok
#5 41.12 npm ERR! code ELIFECYCLE
#5 41.12 npm ERR! errno 1
#5 41.12 npm ERR! keytar@7.6.0 build: `node-gyp rebuild`
#5 41.12 npm ERR! Exit status 1

Making it easy for Rush to be included in Docker images would definitely be a plus, so dropping keytar (if possible) sounds good to me.

@octogonz
Copy link
Collaborator Author

The upstream issue has changed to: Azure/azure-sdk-for-js#14346

@kaiyoma
Copy link

kaiyoma commented Apr 23, 2021

Oh BTW, I did finally figure out how to "fix" the install issue, though it's probably not great:

npm install -g @microsoft/rush@5.43.0 --unsafe

The --unsafe flag allows the keytar installation to proceed and seems to suppress all the permissions warnings.

@octogonz octogonz added bug Something isn't working as intended external issue The root cause is with an external component that needs a fix or workaround labels Apr 23, 2021
@octogonz
Copy link
Collaborator Author

octogonz commented Apr 23, 2021

Tagging this as a bug because in @kaiyoma's case Rush fails to install (despite the fact that a keytar native binary is available for his Node.js environment).

Tagging this as external issue because in Azure/azure-sdk-for-js#14346 the upstream project has agreed to solve this, and their timeframe is now "within a couple of weeks." 🎉

@ujwal-setlur
Copy link

My docker builds work just fine, but keytar dependency does throw a warning.

@mikeharder
Copy link
Contributor

mikeharder commented Apr 23, 2021

I can confirm what @ujwal-setlur is reporting. Despite all the errors and warnings, rush does seem to install and run correctly (at least enough to run rush init).

$ docker run -it --rm --entrypoint bash node:14

# npm install -g @microsoft/rush
[errors and warnings]
+ @microsoft/rush@5.45.2
added 301 packages from 312 contributors in 19.209s

# rush init
Rush Multi-Project Build Tool 5.45.2 (unmanaged) - https://rushjs.io
Node.js version is 14.16.1 (LTS)

Starting "rush init"

Generating: /app/rush.json
Generating: /app/.gitattributes
Generating: /app/.gitignore
Generating: /app/.travis.yml
Generating: /app/common/config/rush/.npmrc
Generating: /app/common/config/rush/.npmrc-publish
Generating: /app/common/config/rush/artifactory.json
Generating: /app/common/config/rush/command-line.json
Generating: /app/common/config/rush/common-versions.json
Generating: /app/common/config/rush/experiments.json
Generating: /app/common/config/rush/pnpmfile.js
Generating: /app/common/config/rush/version-policies.json
Generating: /app/common/git-hooks/commit-msg.sample

@ujwal-setlur
Copy link

Yeah, rush install, rush build, and rush deploy all work for me.

@kaiyoma
Copy link

kaiyoma commented Apr 23, 2021

It does seem like Rush gets installed, but do all the warnings/errors mean that keytar wasn't installed? If that's the case, then the cloud build cache wouldn't work, right? Still seems like a problem if all the dependencies can't be installed correctly.

@ujwal-setlur
Copy link

ujwal-setlur commented Apr 23, 2021

True, keytar likely doesn’t get installed properly. I don’t use that feature, so I wouldn’t know. But the basics do seem to work. But those red messages during installing rush itself sure do look ugly and worrisome.

@octogonz
Copy link
Collaborator Author

Looks like we have a possible solution: PR #2647

@octogonz
Copy link
Collaborator Author

octogonz commented May 4, 2021

This fix was released with Rush 5.46.0

@kaiyoma
Copy link

kaiyoma commented May 4, 2021

Tested and works great for us! I no longer have to specify --unsafe when installing Rush into a Docker image. npm install -g @microsoft/rush@5.46.0 works on its own now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as intended external issue The root cause is with an external component that needs a fix or workaround
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants