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

[PM-5420] feat: arm64 build for bw cli #7338

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mloiseleur
Copy link

@mloiseleur mloiseleur commented Dec 22, 2023

Type of change

- [ ] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [x] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Fixes #5262

Provide BitWarden CLI on arm64 env like Raspberry Pi or Servers.

Code changes

It's based on work of @noahjahn (#2976) focused only on Linux arm64. I needed it, so I tried to finish it and focus only on Linux arm64 build.

It also includes an update of argon2, needed for this target env. This update is based on work of @jrcichra (#6605).

It tries to be as non-intrusive as possible:

  1. A dedicated job for Cross Compilation
  2. A new pkg with arm64 target

Community post ref: https://community.bitwarden.com/t/arm-support-for-cli/42362

Like PR #2976, it involves @bitwarden/dept-devops to update workflow to pick this new artifact.

I tested it on my fork. The build-cli workflow runs successfully on my fork.

@mloiseleur mloiseleur requested a review from a team as a code owner December 22, 2023 09:31
@CLAassistant
Copy link

CLAassistant commented Dec 22, 2023

CLA assistant check
All committers have signed the CLA.

@bitwarden-bot
Copy link

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PM-5420

@bitwarden-bot bitwarden-bot changed the title feat: arm64 build for bw cli [PM-5420] feat: arm64 build for bw cli Dec 22, 2023
@1ace
Copy link

1ace commented Dec 22, 2023

I'm very interested in this, as my main work machine runs on aarch64.

Is it possible to get an example CI output (ie. the bw binary) so that I can confirm that it works (or help fix whatever would need fixing) before merging this PR?

Edit: in particular, something that people who are only/mostly familiar with x86 often overlook is that 4k is not the only page size that exists on arm architectures, and 16k apps work on 4k machines but not vice versa, so we need to make sure this isn't accidentally compiled for 4k aarch64 only. Testing on a Raspberry Pi for instance would not reveal this defect as those have 4k pages, but testing on a MacBook will as they have 16k pages.

@mloiseleur
Copy link
Author

The successful run is publicly available here:
https://github.com/mloiseleur/clients/actions/runs/7298251623

If you scroll down a bit, you can access to artifacts generated:
image

Direct link here.

@1ace
Copy link

1ace commented Dec 23, 2023

Ah I didn't think to look at your fork; thanks!

I'll run it when I have a minute between Christmas preparations 😅

@1ace
Copy link

1ace commented Dec 23, 2023

$ ./bw 
pkg/prelude/bootstrap.js:1872
      throw error;
      ^

Error: /tmp/pkg/4bd3f4b31ee48f604a21d5f03bada979b98c4ff4ee1f1798114d34ac3ac586cf/argon2/lib/binding/napi-v3/argon2.node: cannot open shared object file: No such file or directory
    at process.dlopen (pkg/prelude/bootstrap.js:2251:28)
    at Module._extensions..node (node:internal/modules/cjs/loader:1196:18)
    at Module.load (node:internal/modules/cjs/loader:988:32)
    at Module._load (node:internal/modules/cjs/loader:834:12)
    at Module.require (node:internal/modules/cjs/loader:1012:19)
    at Module.require (pkg/prelude/bootstrap.js:1851:31)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/snapshot/clients/node_modules/argon2/argon2.js:9:25)
    at Module._compile (pkg/prelude/bootstrap.js:1926:22)
    at Module._extensions..js (node:internal/modules/cjs/loader:1166:10) {
  code: 'ERR_DLOPEN_FAILED'
}

Node.js v18.5.0

I have no idea how to debug node apps, but I'm guessing this No such file or directory (which can mean either the lib is actually missing or simply unusable; in this case unusable) is because of this typical page size issue.

Since there's nothing in the CI or build script that mentions page sizes, there are two solutions here: either there's an env var or param that can be passed somewhere in the build system to set the page size, or the build sys doesn't support configuring this and is only able to use the page size of the build machine (sadly typical in build systems written by people from the x86 world), in which case the CI needs to be configured to run on a 16k/64k machine so that auto-detection does the right thing.

In case there is no such machine available physically, qemu can be used at a small performance cost, but for a build job it doesn't really matter even if the cost is 10% or 20%, it just means the build takes a few more seconds 😅

@mloiseleur
Copy link
Author

mloiseleur commented Dec 26, 2023

@1ace Digging deeper on your issue, I discovered there is a specific binary dependency on node-argon2. The last release of node-argon2 include a fix for macos and 0.31.1 for linux arm64. Inspired by PR #6605, I updated this dependency.

Feel free to test build on this run: https://github.com/mloiseleur/clients/actions/runs/7328518114

If it's not working, I'll need to get more info on your issue : the name of share object file it cannot open would help. You may catch it with strace or equivalent cli cmd.

@1ace
Copy link

1ace commented Dec 26, 2023

Thanks for working on this!

Unfortunately, the result is still the same:

$ ./bw 
pkg/prelude/bootstrap.js:1872
      throw error;
      ^

Error: /tmp/pkg/53ebb22c3ad69d48ae6551060e08df953fda12b022d1620201f969e145be4be2/argon2/lib/binding/napi-v3/argon2.node: cannot open shared object file: No such file or directory
    at process.dlopen (pkg/prelude/bootstrap.js:2251:28)
    at Module._extensions..node (node:internal/modules/cjs/loader:1196:18)
    at Module.load (node:internal/modules/cjs/loader:988:32)
    at Module._load (node:internal/modules/cjs/loader:834:12)
    at Module.require (node:internal/modules/cjs/loader:1012:19)
    at Module.require (pkg/prelude/bootstrap.js:1851:31)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/snapshot/clients/node_modules/argon2/argon2.js:9:25)
    at Module._compile (pkg/prelude/bootstrap.js:1926:22)
    at Module._extensions..js (node:internal/modules/cjs/loader:1166:10) {
  code: 'ERR_DLOPEN_FAILED'
}

Node.js v18.5.0

Here's the full strace: bw.strace.txt

If I'm reading this right, it checks that /tmp/pkg/53ebb22c3ad69d48ae6551060e08df953fda12b022d1620201f969e145be4be2/argon2/lib/binding/napi-v3/argon2.node exists, the check passes, it reads the file, sees that it's an ELF, then I think it opens it through dlopen() which uses openat() which fails with error 20 (ENOTDIR), which according to the man page means either of these:

ENOTDIR
       A component used as a directory in pathname is not, in fact, a directory, or O_DIRECTORY was specified and pathname was not a directory.

ENOTDIR
       (openat()) pathname is a relative pathname and dirfd is a file descriptor referring to a file other than a directory.

Neither of these seems to make sense given the successful faccessat() and pread() on the same path.

I'm guessing I'm reading something wrong, but I only had a few minutes tonight, so I hope you can make more sense of it :)

@mloiseleur
Copy link
Author

mloiseleur commented Dec 27, 2023

It seems to be a known bug with argon2 on cross build. It brings amd64 binary instead of arm64 binary. code-server encountered the same problem.

I tested the build on a arm64 pod, and file confirms it:

$ file /tmp/pkg/53ebb22c3ad69d48ae6551060e08df953fda12b022d1620201f969e145be4be2/argon2/lib/binding/napi-v3/argon2.node 
/tmp/pkg/53ebb22c3ad69d48ae6551060e08df953fda12b022d1620201f969e145be4be2/argon2/lib/binding/napi-v3/argon2.node: ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), dynamically linked, BuildID[sha1]=bf86948aeae90b278af6bbee2edd12f797e0e6c3, not stripped

Edit: node-argon2 maintainer made prebuilt cross arch binaries last week. It's using ESM and ... pkg does not support ESM.

@mloiseleur mloiseleur force-pushed the feat/arm64-cli branch 14 times, most recently from fa66bf0 to f49d61c Compare December 27, 2023 14:46
@mloiseleur
Copy link
Author

mloiseleur commented Dec 27, 2023

@1ace There are some real obstacles on the way:

  1. GitHub has not yet released pure arm runner.
  2. Running all the install and build workflow takes more 20 minutes on emulated arm64 action
  3. node-argon2 pre-built binaries are convenient only for same arch build.

At the end, I found an efficient way to get it working in an arm64 debian pod. It's quite simple: just replace the invalid binary with the correct one from the github release of node-argon2.

See this run if you want to test it: https://github.com/mloiseleur/clients/actions/runs/7339395597

@1ace
Copy link

1ace commented Dec 27, 2023

See this run if you want to test it: https://github.com/mloiseleur/clients/actions/runs/7339395597

It works! 🙌

I have verified that I can not only run the app, but login and access my credentials, so it all works!

Thank you so much for pushing through all these issues 💪

@ranisalt
Copy link

Edit: node-argon2 maintainer made prebuilt cross arch binaries last week. It's using ESM and ... pkg does not support ESM.

I could move back to commonjs if necessary. I just rewrote it with ESM due to a massive lack of feedback in the repo, which makes it hard to steer the project.

flabatut added a commit to flabatut/bitwarden-cli that referenced this pull request Jan 27, 2024
@jrcichra
Copy link

I've had success keeping my bitwarden cli pinned to @bitwarden/cli@2023.8.2 until a fix is merged.

@ranisalt
Copy link

node-argon2 has been updated to ship all available prebuilt binaries on install, once it's updated to v0.40.1 it will be easy :)

@mloiseleur
Copy link
Author

\o many thanks @ranisalt !

@MGibson1 @joseph-flinn Now that there is a simple working solution, any chance that this PR get reviewed and merged ?

@jomifepe
Copy link
Contributor

jomifepe commented Feb 29, 2024

Hey @mloiseleur, first of all, great work, I'm looking forward to this 🚀

Following the intent of #2976, I'm interested in also having the macos binary in arm64 🙂

Copy link
Contributor

@jomifepe jomifepe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the job for macos and it seems to work: https://github.com/jomifepe/clients/actions/runs/8118254797.

Hopefully we can get it done after this one is merged!

@jomifepe
Copy link
Contributor

Hey there, is there any news on this? Still no reviews from the Bitwarden team. I thought about opening a PR for the macOS part but seeing this doesn't get any attention, I don't know if I should bother.

curl -L -o argon2.tar.gz "https://github.com/ranisalt/node-argon2/releases/download/v${ARGON2_VERSION}/argon2-v${ARGON2_VERSION}-napi-v3-linux-arm64-glibc.tar.gz"
tar xvzf argon2.tar.gz
mv napi-v3/argon2.node node_modules/argon2/lib/binding/napi-v3/argon2.node
rm -rf argon2.tar.gz napi-v3

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This step can be skipped now, if you update argon2 in package.json

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried but it's failing:

./bw -version
pkg/prelude/bootstrap.js:1872
      throw error;
      ^

Error: Cannot find module '/snapshot/clients/apps/cli/node_modules/argon2/lib/binding/napi-v3/argon2.node'
Require stack:
- /snapshot/clients/apps/cli/node_modules/argon2/argon2.js
- /snapshot/clients/apps/cli/build/bw.js
1) If you want to compile the package/file into executable, please pay attention to compilation warnings and specify a literal in 'require' call. 2) If you don't want to compile the package/file into executable and want to 'require' it from filesystem (likely plugin), specify an absolute path in 'require' call using process.cwd() or process.execPath.
    at Module._resolveFilename (node:internal/modules/cjs/loader:946:15)
    at Function._resolveFilename (pkg/prelude/bootstrap.js:1951:46)
    at Module._load (node:internal/modules/cjs/loader:787:27)
    at Module.require (node:internal/modules/cjs/loader:1012:19)
    at Module.require (pkg/prelude/bootstrap.js:1851:31)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/snapshot/clients/apps/cli/node_modules/argon2/argon2.js:9:25)
    at Module._compile (pkg/prelude/bootstrap.js:1926:22)
    at Module._extensions..js (node:internal/modules/cjs/loader:1166:10)
    at Module.load (node:internal/modules/cjs/loader:988:32) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/snapshot/clients/apps/cli/node_modules/argon2/argon2.js',
    '/snapshot/clients/apps/cli/build/bw.js'
  ],
  pkg: true
}

Node.js v18.5.0

Did I miss something ? Or should I re-add the workaround ?

@mloiseleur mloiseleur force-pushed the feat/arm64-cli branch 2 times, most recently from 3a5e6e9 to 50da38d Compare April 16, 2024 10:50
@feyst
Copy link

feyst commented Sep 11, 2024

Hey, thanks for the great work. Does this also publish the ARM version to the snap and choco store?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to install bw-cli snap on Linux ARM
8 participants