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 support for setting POSIX capabilities on the binary generated by Go #1098

Closed
wants to merge 8 commits into from

Conversation

inteon
Copy link

@inteon inteon commented Jul 17, 2023

To prevent an application from being swapped to disk, we use mlock (prevents writing private keys to disk).
However, this requires the POSIX CAP_IPC_LOCK capability to be set on the binary file.
This PR adds the option to specify what capabilities to set on the binary file.

NOTE for reviewer:
Would it be possible to get some feedback on whether this feature would be accepted in ko (regardless of the implementation)?

@inteon
Copy link
Author

inteon commented Aug 21, 2023

@cpanato Do you have an initial opinion on this PR?
& enable tests for this PR?

@imjasonh
Copy link
Member

This change looks good! Sorry it's taken so long to review it.

We'll want to regenerate the CLI docs to pass CI, and probably add docs to the website to describe how/why you'd use this. An e2e test that builds and runs ./test with some capability would also help guard against future regressions, but I'll defer to you about how best to test that.

Thanks for this contribution, and sorry again for taking so long to get to it.

@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2023

Codecov Report

Merging #1098 (207568f) into main (d4a1cc9) will decrease coverage by 1.65%.
The diff coverage is 5.10%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main    #1098      +/-   ##
==========================================
- Coverage   49.19%   47.54%   -1.65%     
==========================================
  Files          44       45       +1     
  Lines        3653     3786     +133     
==========================================
+ Hits         1797     1800       +3     
- Misses       1624     1752     +128     
- Partials      232      234       +2     
Files Changed Coverage Δ
pkg/build/options.go 61.11% <0.00%> (-3.60%) ⬇️
pkg/build/posixcapability.go 0.00% <0.00%> (ø)
pkg/commands/resolver.go 35.48% <0.00%> (-0.97%) ⬇️
pkg/build/gobuild.go 55.06% <12.19%> (-2.23%) ⬇️
pkg/commands/options/build.go 68.80% <100.00%> (+0.50%) ⬆️

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
@jonjohnsonjr
Copy link
Collaborator

NOTE for reviewer:
Would it be possible to get some feedback on whether this feature would be accepted in ko (regardless of the implementation)?

Definitely a feature I'm interested in. Is there any prior art here re: the UX of this flag?

@jonjohnsonjr
Copy link
Collaborator

Is there any prior art here re: the UX of this flag?

Answering my own question: I see that docker has a --cap-add and --cap-drop flag for docker run. Using that might be reasonable? I'm not sure if it's even possible to drop capabilities using xattrs, so maybe that doesn't make any sense.

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
pkg/build/gobuild.go Outdated Show resolved Hide resolved
@@ -177,3 +177,11 @@ func WithSBOMDir(dir string) Option {
return nil
}
}

// WithPOSIXCapabilities is a functional option for overriding the POSIX capabilities encoded in the binary file.
Copy link
Member

Choose a reason for hiding this comment

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

Small nit: How would you feel about WithPOSIXCapabilities(caps ...Cap) so that callers don't have to wrap it in a []Cap themselves?

Copy link
Author

Choose a reason for hiding this comment

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

Because WithPOSIXCapabilities overwrites the capabilities slice, I prefer the []Cap argument.
...Cap makes me think that the function will append the capabilities to the slice.
Maybe it's just me who thinks that. Please let me know if that is the case & I'll update the code.

type Cap int

// POSIX-draft defined capabilities.
const (
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd feel comfortable taking a dependency on https://pkg.go.dev/github.com/syndtr/gocapability instead of having to maintain our own copy here. Or if there's a better package to depend on, let me know.

Copy link
Author

@inteon inteon Aug 24, 2023

Choose a reason for hiding this comment

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

I think that library does not have a good FromString method that we can use.
I found that containerd defines its list of capabilities here: https://github.com/containerd/containerd/blob/v1.7.3/pkg/cap/cap_linux.go#L133-L187
& added some capabilities to the file to match their list.

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
@inteon
Copy link
Author

inteon commented Sep 29, 2023

@imjasonh @jonjohnsonjr WDYT about the latest changes? Is this ready to be merged?

Copy link

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

@mejedi
Copy link
Contributor

mejedi commented Oct 22, 2024

@inteon FYI similar feature landed in #1271

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.

5 participants