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

Build non-static binaries with PIE buildmode #102323

Merged
merged 1 commit into from
May 31, 2021

Conversation

saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented May 26, 2021

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

We now add the -buildmode pie flag when building non-static binaries, which enables the ASLR security mechanism.

Which issue(s) this PR fixes:

Fixes #90311

Special notes for your reviewer:

Supersedes #94448

For example, we now build the kubelet as LSB pie executable:

> make WHAT=cmd/kubelet
> file _output/bin/kubelet
_output/bin/kubelet: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /nix/store/vaf92wc23m067q9aig6zjkcnjvrg768n-glibc-2.32-46/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, Go BuildID=2XJTWqoOSzrJFsezWtNV/Ljlb-W11uIqFXSJ5PWOt/4aLSZPegz6V8zUDouB74/B8q7ucGO132SbpmNnZMA, stripped

We do not apply the change to statically linked binaries, because if we add the pie buildmode to them we would result in gaining a dynamically linked binary without choosing an external linker. Ref: golang/go#40719

Does this PR introduce a user-facing change?

Changed buildmode of non static Kubernetes binaries to produce position independent executables (PIE).

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

/area release-eng
/sig release
/priority important-soon
/cc @kubernetes/release-managers @abhay-krishna

@k8s-ci-robot
Copy link
Contributor

@saschagrunert: GitHub didn't allow me to request PR reviews from the following users: abhay-krishna.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

We now add the -buildmode pie flag when building non-static binaries, which enables the ASLR security mechanism.

Which issue(s) this PR fixes:

Fixes #90311

Special notes for your reviewer:

Supersedes #94448

For example, we now build the kubelet as LSB pie executable:

> make WHAT=cmd/kubelet
> file _output/bin/kubelet
_output/bin/kubelet: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /nix/store/vaf92wc23m067q9aig6zjkcnjvrg768n-glibc-2.32-46/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, Go BuildID=2XJTWqoOSzrJFsezWtNV/Ljlb-W11uIqFXSJ5PWOt/4aLSZPegz6V8zUDouB74/B8q7ucGO132SbpmNnZMA, stripped

We do not apply the change to statically linked binaries, because if we add the pie buildmode to them we would result in gaining a dynamically linked binary without choosing an external linker.

Does this PR introduce a user-facing change?

Build non static

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

/area release-eng
/sig release
/priority important-soon
/cc @kubernetes/release-managers @abhay-krishna

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 26, 2021
@k8s-ci-robot k8s-ci-robot requested a review from a team May 26, 2021 08:04
@k8s-ci-robot k8s-ci-robot added area/release-eng Issues or PRs related to the Release Engineering subproject size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. sig/release Categorizes an issue or PR as relevant to SIG Release. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 26, 2021
@k8s-ci-robot
Copy link
Contributor

@saschagrunert: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label May 26, 2021
Copy link
Member

@xmudrii xmudrii left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2021
@BenTheElder
Copy link
Member

BenTheElder commented May 26, 2021

Have we investigated fully e.g.:

  • binary size impact
  • Platform support? I recall this being restricted to certain platforms in go.
    /test pull-kubernetes-cross

I'm still not clear on if we think this will realistically defend against anything or check a compliance checkbox. If it's only the latter then I think the GOFLAGS support and custom builds should cover that angle.

Edit: unless it proves to have no negative impacts then 🤷‍♂️ but I haven't seen that addressed.

@saschagrunert
Copy link
Member Author

saschagrunert commented May 26, 2021

Binary size comparison

Only a few binaries are build with PIE if we run make:

> file _output/bin/* | rg pie
_output/bin/gendocs:                  ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /nix/store/vaf92wc23m067q9aig6zjkcnjvrg768n-glibc-2.32-46/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, Go BuildID=_d2KgGty1Y6_MSazK5Jk/3yF_B3YODNHxJQQcLO6A/QjAnNuKVWj4DboY948u2/wsZTn89yQHMAgYLlfRzL, stripped
_output/bin/genkubedocs:              ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /nix/store/vaf92wc23m067q9aig6zjkcnjvrg768n-glibc-2.32-46/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, Go BuildID=pUqkV9w3NiWUQeP5VG_T/sIolGFDiizX8-m5bRW4Q/nlEHNoxpLXMCRlOpc4tR/CczSf5vapfKrHp7SzQmj, stripped
_output/bin/genman:                   ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /nix/store/vaf92wc23m067q9aig6zjkcnjvrg768n-glibc-2.32-46/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, Go BuildID=xOy3GkYhsocVP1XlpjqM/V5QA7seFrY8jYQu_-2z5/cUhOL6ZFahsKsht6PWPj/hZHSFVhliVgSqi7FC3sl, stripped
_output/bin/genyaml:                  ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /nix/store/vaf92wc23m067q9aig6zjkcnjvrg768n-glibc-2.32-46/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, Go BuildID=7_02dm6WLh8fPyXGlX2E/P1WpfpSt7xBaUnhRM7vk/EGkqZ4LE0HlINKbKaRID/4o5VmlZrDHg70c1Uiccs, stripped
_output/bin/kubectl-convert:          ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /nix/store/vaf92wc23m067q9aig6zjkcnjvrg768n-glibc-2.32-46/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, Go BuildID=2rpVm1qyEjM93PNvoCuC/6eAjhy5q5ycwAmUnUrUU/aBpLHa02qan5X5T3IEul/BpxL3sumz7xd0aVczoEj, stripped
_output/bin/kubelet:                  ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /nix/store/vaf92wc23m067q9aig6zjkcnjvrg768n-glibc-2.32-46/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, Go BuildID=Hv1ij3pn7pWX7UAeGV92/Ljlb-W11uIqFXSJ5PWOt/4aLSZPegz6V8zUDouB74/w4DzDHeUwrplTcYeo3P-, stripped
_output/bin/kubemark:                 ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /nix/store/vaf92wc23m067q9aig6zjkcnjvrg768n-glibc-2.32-46/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, Go BuildID=f8w_af24P1DYtMUkIRVm/F9gwmSOF-a76VYIsHr84/KkyzMvyFIH9RKxoLDJuV/XZIL5rzhDzSWo0LnnofR, stripped

If we compare their binary sizes, then we indeed encounter a difference:

Binary without PIE (before) with PIE (after)
gendocs 55Mi 64Mi
genkubedocs 189Mi 226Mi
genman 197Mi 235Mi
genyaml 55Mi 64Mi
kubectl-convert 53Mi 62Mi
kubelet 113Mi 143Mi
kubemark 110Mi 139Mi

Platform support

We have PIE mode since:

  • go 1.6 for: android/386, android/amd64, android/arm, android/arm64, linux/386, linux/amd64, linux/arm, linux/arm64, and linux/ppc64le
  • go 1.10 for: darwin/amd64 (also forces the use of external linking on all systems)
  • go 1.13 for: aix/ppc64
  • go 1.15 for: windows (on per default), linux/amd64 and linux/arm64 defaults to internal linking mode for -buildmode=pie
  • go 1.16 for: darwin/arm64

Depending on how the compiler behaves and the output of pull-kubernetes-cross, we may have to exclude architectures like s390x.

@xmudrii
Copy link
Member

xmudrii commented May 26, 2021

I'm still not clear on if we think this will realistically defend against anything or check a compliance checkbox. If it's only the latter then I think the GOFLAGS support and custom builds should cover that angle.

The issue (#90311) has some context about this. This comment from @tabbysable and the following comments provide some insights why this would be useful: #90311 (comment)

From @saschagrunert's table above, the binary size increase is about 10-20% depending on the binary. This might sound like a lot, but most of the binaries are somewhat small. I don't have experience with this concrete topic, so I can't weigh is it worth or not, but in my opinion, I don't really see this as a problem.

@BenTheElder
Copy link
Member

I wouldn't call > 100mb small and I'm not sure many other people would either? Most of our binaries are sadly over 100mb easily. They're not all in the table above.

We can ignore the gen* as we don't distribute those.

Making kubelet ~27% larger is not great :(

@BenTheElder
Copy link
Member

This broke the cross build
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 26, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2021
@saschagrunert
Copy link
Member Author

/test pull-kubernetes-cross

@saschagrunert
Copy link
Member Author

/test pull-kubernetes-e2e-kind-ipv6

We now add the `-buildmode pie` flag when building non-static binaries,
which enables the ASLR security mechanism.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
@saschagrunert
Copy link
Member Author

/test pull-kubernetes-cross

@saschagrunert
Copy link
Member Author

The new kube-cross version makes pull-kubernetes-cross green. PTAL again @BenTheElder

@BenTheElder
Copy link
Member

Depending on how the compiler behaves and the output of pull-kubernetes-cross, we may have to exclude architectures like s390x.

It seems that wasn't the case. https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/102323/pull-kubernetes-cross/1397898981865951232

As an aside: I wonder how long the project should keep building for relatively obsolete platforms like i386 ...

Still disappointed to see kubelet get bigger again but oh well. https://groups.google.com/g/golang-nuts/c/cXhRsmNsMwo/m/VUZlLqo9AwAJ seems pretty clear that there's at least some benefit to programs using CGO (like kubelet to an extent, indirectly).

I also asked the GKE security team for more background on this since there's been pretty limited discussion on github and I've already poked sig-security, they had a similar response to the effect of roughly "it's probably relatively minor / theoretical but perhaps useful with CGO".

in either case, the build script changes look acceptable purely from a code POV without respect to "should we PIE or not" and PIE seems at least potentially useful for these non-static binaries.

/approve

and since cross is green again:
/hold-cancel

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 27, 2021
@saschagrunert
Copy link
Member Author

Referring to: #102323 (comment)
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 31, 2021
@saschagrunert
Copy link
Member Author

@kubernetes/release-engineering @kubernetes/sig-security-pr-reviews please take a look for lgtm

@k8s-ci-robot k8s-ci-robot added the sig/security Categorizes an issue or PR as relevant to SIG Security. label May 31, 2021
@justaugustus
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, justaugustus, saschagrunert, xmudrii

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@palnabarun
Copy link
Member

/test pull-kubernetes-e2e-kind-ipv6

@xmudrii
Copy link
Member

xmudrii commented May 31, 2021

/test pull-kubernetes-unit

@k8s-ci-robot k8s-ci-robot merged commit a6a6fb3 into kubernetes:master May 31, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone May 31, 2021
@saschagrunert saschagrunert deleted the pie branch May 31, 2021 11:52
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. release-note-none Denotes a PR that doesn't merit a release note. labels Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/security Categorizes an issue or PR as relevant to SIG Security. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build position independent executables
6 participants