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.

Sign up for GitHub

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

Jump to bottom

crypt: provide non-cgo implementation of "crypt()" to be more cross-build friendly #1114

Merged
merged 3 commits into from
Jan 13, 2025
Merged

crypt: provide non-cgo implementation of "crypt()" to be more cross-build friendly #1114

merged 3 commits into from
Jan 13, 2025

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Dec 20, 2024

This PR tweaks the images library to work without "cgo" so that we can cross build. Thanks to @supakeen for raising this!

With the PR we can now do:

$ GOARCH=arm64 go build -o ibc-aarch64 -tags "containers_image_openpgp exclude_graphdriver_btrfs exclude_graphdriver_devicemapper" ./cmd/image-builder

in the image-builder-cli source tree.

mvo5
mvo5 commented Dec 20, 2024
View reviewed changes
pkg/crypt/crypt_test.go Outdated Show resolved Hide resolved
@mvo5 mvo5 force-pushed the cross-build-friendly-crypt branch 2 times, most recently from 8299f82 to 6b24f2f Compare December 20, 2024 11:54
@ondrejbudai
Copy link
Member

Food for thought: what if we used openssl everywhere? I suppose that less cgo is generally better.

achilleas-k
achilleas-k requested changes Jan 9, 2025
View reviewed changes
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

LGTM, but unit tests in GH actions need openssl now.

Also a note about communicating external dependencies, which might be a topic for a follow-up PR.

pkg/crypt/crypt_impl_non_cgo.go Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
@mvo5 @thozza
crypt: provide a non-cgo fallback for crypt()
4b09614 
This commit add a trivial openssl based fallback version of crypt()
that works when the library is build without cgo (e.g. when doing
a cross build).
@thozza thozza force-pushed the cross-build-friendly-crypt branch from 6b24f2f to d627f30 Compare January 9, 2025 12:16
@thozza
Copy link
Member

thozza commented Jan 9, 2025

Rebased due to image build cache layout change (#1130)

@mvo5
workflow: run unit tests without cgo too
8bcad38 
Now that we support building images without cgo also test it.
@mvo5 mvo5 force-pushed the cross-build-friendly-crypt branch from d627f30 to 6b31cff Compare January 9, 2025 15:34
@mvo5 mvo5 changed the title [RFC] Be more cross-build friendly Be more cross-build friendly Jan 9, 2025
@mvo5 mvo5 changed the title Be more cross-build friendly crypt: provide non-cgo implementation of "crypt()" to be more cross-build friendly Jan 9, 2025
supakeen
supakeen previously approved these changes Jan 10, 2025
View reviewed changes
@mvo5 mvo5 requested a review from achilleas-k January 10, 2025 07:59
@mvo5
Copy link
Contributor Author

mvo5 commented Jan 10, 2025

Alternatively we could just switch to the pure go implementation in https://github.com/nanitor/pwhash - I have no strong opinions either way (I looked at the go implementation briefly and it looks fine and uses the official test vectors). I can do this as e.g. a followup.

@ondrejbudai
Copy link
Member

ondrejbudai commented Jan 10, 2025 •
edited
Loading

I actually have a quite strong opinion here: The more crypto we can offload to already existing implementation in operating systems, the better. I don't want our team to maintain needless crypto (and yes, since we are vendoring all the code, we are technically speaking maintaining it). Also, openssl and libxcrypt are widely used and recognized libraries.

I'm happy to use Go libraries for a almost everything, the Go ecosystem is wonderful. Just not crypto please.

thozza
thozza previously approved these changes Jan 13, 2025
View reviewed changes
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

LGTM.

The last commit has a typo in the commit message "kttp" vs. "http", but I can live with it 😅

@supakeen supakeen enabled auto-merge January 13, 2025 12:38
@mvo5
upload: disable koji when building with cgo
501d0f3 
koji requires the khttp kerberos module which requires cgo so when
build without cgo kerberos uploads are currently not supported.
@mvo5 mvo5 dismissed stale reviews from thozza and supakeen via 501d0f3 January 13, 2025 15:20
@mvo5 mvo5 force-pushed the cross-build-friendly-crypt branch from 6b31cff to 501d0f3 Compare January 13, 2025 15:20
@mvo5
Copy link
Contributor Author

mvo5 commented Jan 13, 2025

LGTM.

The last commit has a typo in the commit message "kttp" vs. "http", but I can live with it 😅

Nice catch! Indeed, the problematic import is "github.com/ubccr/kerby/khttp", I fixed kttp to khttp now.

Sadly will need a re-view :)

@mvo5 mvo5 requested a review from thozza January 13, 2025 15:20
@supakeen supakeen added this pull request to the merge queue Jan 13, 2025
Merged via the queue into osbuild:main with commit 5d0d854 Jan 13, 2025
18 checks passed
mvo5 added a commit to mvo5/image-builder-cli that referenced this pull request Jan 29, 2025
@mvo5
Containerfile: build without cgo to avoid arm64 crashing
058113c 
This commit changes the build to not build with CGO. There is really
no need to build with CGO, we only need it for the `crypt()` version
in the images library. However since with the merge of
osbuild/images#1114
that is no longer an issue.
@mvo5 mvo5 mentioned this pull request Jan 29, 2025
Merged
github-merge-queue bot pushed a commit to osbuild/image-builder-cli that referenced this pull request Jan 29, 2025
@mvo5 @supakeen
Containerfile: build without cgo to avoid arm64 crashing
c72ed92 
This commit changes the build to not build with CGO. There is really
no need to build with CGO, we only need it for the `crypt()` version
in the images library. However since with the merge of
osbuild/images#1114
that is no longer an issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@supakeen supakeen supakeen approved these changes

@thozza thozza thozza approved these changes

@achilleas-k achilleas-k achilleas-k approved these changes

@ondrejbudai ondrejbudai Awaiting requested review from ondrejbudai

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mvo5 @ondrejbudai @thozza @achilleas-k @supakeen