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

Add boringcrypto image #71

Merged
merged 42 commits into from
Jul 18, 2023
Merged

Add boringcrypto image #71

merged 42 commits into from
Jul 18, 2023

Conversation

dimitarvdimitrov
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov commented Jul 17, 2023

This adds a boringcrypto image

  • builds and pushes amd64/arm64 boringcrypto image upon make publish-images
    • images are pushed to grafana/rollout-operator-boringcrypto
    • also build the binary in an alpine container as opposed to a Debian. This means that the binary is linked against musl instead of glibc and running alpine with glibc may come with problems
  • runs unit and integration tests with boringcrypto

@dimitarvdimitrov dimitarvdimitrov requested a review from a team as a code owner July 17, 2023 11:39
@dimitarvdimitrov
Copy link
Contributor Author

The CI is failing the same bug that @andyasp encountered here #70 (comment)

Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

See questions.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@aknuds1 aknuds1 requested a review from a team July 17, 2023 12:07
Makefile Show resolved Hide resolved
@aknuds1 aknuds1 requested a review from a team July 17, 2023 13:38
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

Please see comments

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@dimitarvdimitrov
Copy link
Contributor Author

thanks for the reviews @aknuds1, I think I addressed all of the issues. Sorry for the state of this PR; I should have reviewed it on my own a few times before opening.

Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM, modulo a couple of newline nits.

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
@dimitarvdimitrov
Copy link
Contributor Author

I pushed a commit to pin the go version so we can see a passing CI. After that I'll undo the pin and merge on failing CI

@dimitarvdimitrov
Copy link
Contributor Author

I had to make some adjustment to the PR. As of now this is the amd4 binary built from this PR using the commit where CI tests ran - 7a0770d

$ file /tmp/rollout-operator
/tmp/rollout-operator: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, Go BuildID=GpWoX651AkwtQPsrgVNx/nuEX87cG0CpHr_FAQAlZ/WGnquJfCp-utqUX_5FNc/eePK3vlRL3Hn65mybSiG, with debug_info, not stripped

and includes boringcrypto & FIPS symbols as opposed to standard library symbols

$ go tool nm /tmp/rollout-operator | grep FIPS
  461b80 t BORINGSSL_FIPS_abort
  45da80 t FIPS_mode_set
  45da90 t FIPS_read_counter
  401490 T _cgo_d8b1bdd8e714_Cfunc__goboringcrypto_FIPS_mode
  45da70 t _goboringcrypto_FIPS_mode
  65bd40 T crypto/internal/boring._Cfunc__goboringcrypto_FIPS_mode.abi0
 2a89888 D crypto/internal/boring._cgo_d8b1bdd8e714_Cfunc__goboringcrypto_FIPS_mode
  5eadc0 T crypto/internal/boring/sig.FIPSOnly.abi0
 2a8ae30 D crypto/tls.defaultCipherSuitesFIPS
 2a8aeb0 D crypto/tls.defaultFIPSCurvePreferences

The tests pass on 7a0770d with the go version pinned to 1.20.5. I reverted the pin in f4ef221 that's why the branch is now failing CI. The failing CI is due to a bugfix in docker which hasn't reached us yet (see #70 (comment))

Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

Please see questions, otherwise good to go I think?

Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM, although you should remove the gcompat package in Alpine.

Dockerfile Outdated Show resolved Hide resolved
@dimitarvdimitrov
Copy link
Contributor Author

commands ran on the image from 617719f

file /bin/rollout-operator
/bin/rollout-operator: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib/ld-musl-x86_64.so.1, Go BuildID=QhjtoKw-eV539V2keY8t/JMinCisYZsWEMlAzPP4y/VTEVI2hm4Ymq6TZTkxhX/fQgtBavrsbgLnofGYkki, with debug_info, not stripped
ldd /bin/rollout-operator
	/lib/ld-musl-x86_64.so.1 (0x7f1839c4f000)
	libc.musl-x86_64.so.1 => /lib/ld-musl-x86_64.so.1 (0x7f1839c4f000)
go tool nm /tmp/rollout-operator | grep -E 'sig.FIPSOnly|sig.BoringCrypto|sig.StandardCrypto'
  5ebda0 T crypto/internal/boring/sig.BoringCrypto.abi0
  5ebdc0 T crypto/internal/boring/sig.FIPSOnly.abi0

@aknuds1
Copy link
Contributor

aknuds1 commented Jul 18, 2023

For context, @dimitarvdimitrov and I decided to not statically link the boringcrypto based rollout-operator, since it leads to warnings during the link stage about implicit libc dependencies at runtime (also glibc static linking is discouraged). The dynamically linked image should work perfectly within the Docker image, as it just links to Alpine's musl (libc) library.

Copy link
Collaborator

@andyasp andyasp left a comment

Choose a reason for hiding this comment

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

Minor comments, nice job!

Dockerfile Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
dimitarvdimitrov and others added 2 commits July 18, 2023 15:50
Co-authored-by: Andy Asp <90626759+andyasp@users.noreply.github.com>
Makefile Outdated Show resolved Hide resolved
@dimitarvdimitrov dimitarvdimitrov enabled auto-merge (squash) July 18, 2023 14:18
@dimitarvdimitrov dimitarvdimitrov merged commit b9446f4 into main Jul 18, 2023
4 of 6 checks passed
@dimitarvdimitrov dimitarvdimitrov deleted the dimitar/boringcrypto-image branch July 18, 2023 14:21
dimitarvdimitrov added a commit that referenced this pull request Jul 22, 2023
---------

Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
Co-authored-by: Andy Asp <90626759+andyasp@users.noreply.github.com>
andyasp added a commit that referenced this pull request Jul 22, 2023
* Add boringcrypto image (#71)

---------

Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
Co-authored-by: Andy Asp <90626759+andyasp@users.noreply.github.com>

* Update CHANGELOG.md

* Pin go version in CI

---------

Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
Co-authored-by: Andy Asp <90626759+andyasp@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants