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 capability package #149

Merged
merged 52 commits into from
Sep 13, 2024
Merged

Add capability package #149

merged 52 commits into from
Sep 13, 2024

Conversation

kolyshkin
Copy link
Collaborator

@kolyshkin kolyshkin commented Sep 6, 2024

This integrates github.com/kolyshkin/capability (which itself is a fork
of github.com/syndtr/gocapability) to github.com/moby/sys/capability,
as suggested in opencontainers/runc#4358 (comment)

Some of github.com/syndtr/gocapability users are (in an alphabetical order):

The way it was done is:

# 1. Create a copy of a local repo: 
git clone --no-local capability capability-for-merge
 
# 2. Use git-filter-branch to:
#  - move contents to a subdirectory
#  - remove useless merge commits
#  - remote .github files
#  - add 'capability: ' prefix to commit messages
cd capability-for-merge
git filter-repo --prune-degenerate always --path-rename :capability/ \
    --path-match .github --invert-paths \
    --message-callback '
  prefix = b"capability: "
  if not message.startswith(prefix):
    message=prefix+message
  return message
'

# 3. Merge the above repo to moby/sys
cd ~/git/moby/sys
git checkout -b add-capability
git remote add cap-merge ../../capability-for-merge
git fetch cap-merge
git merge --allow-unrelated-histories --signoff -S cap-merge/main

syndtr and others added 30 commits February 7, 2013 18:00
…urrent task

In this case we can use /proc/self/, which is correct even if a task
live in another pid namespace.

Signed-off-by: Andrey Vagin <[email protected]>
New capabilities can be added, and we want to be sure
that a bounding set will be set correctly in this case.

Without this patch new capabilities are not dropped from a bounding set.

Signed-off-by: Andrey Vagin <[email protected]>
fmt.Fscanf reads byte by byte from this file, but
this doesn't work for sysctl-s.

29279 open("/proc/sys/kernel/cap_last_cap", O_RDONLY|O_CLOEXEC
29279 <... open resumed> ) = 3
29279 read(3, "3", 1) = 1
29279 read(3, "", 1) = 0

Reported-by: @syndtr
…t_cap file

Fix docker/libcontainer#452

Signed-off-by: Alexander Morozov <[email protected]>
Ambient capabilities were added in Linux 4.3 and provide a way
to pass on capabilities to unprivileged processes easily.

Signed-off-by: Justin Cormack <[email protected]>
After getting CapBnd, Loop break too early,
can't to get CapAmb value.

Signed-off-by: Ma Shimiao <[email protected]>
…by#14)

The old methods had an internal Load(), which is unnecessary for some
use cases.  For example, if you're going to drop all capabilities, you
don't need to load the current set first.  This commit deprecates the
old New* functions and adds New*2 functions which do not include the
internal Load.  Callers who do need the Load will need to call it
explicitly after initializing their Capabilities object.  Callers who
do not need the Load can just add the "2" to the function name and get
more efficient/robust behavior.

The "Deprecated:" paragraph syntax is recommended in [1]:

  To signal that an identifier should not be used, add a paragraph to
  its doc comment that begins with "Deprecated:" followed by some
  information about the deprecation.

[1]: https://blog.golang.org/godoc-documenting-go-code
* Fix capHeader.pid type

In C, int is 4 bytes in 32 and 64-bit systems. In Go, int is a
8 bytes in 64-bit systems. Before this fix, pid was being ignored
because the kernel will always read 0 due to padding added between
version and pid fields.

* Update capability_linux.go
CAP_PERFMON and CAP_BPF were introduced in kernel 5.8: https://kernelnewbies.org/Linux_5.8#Introduce_CAP_BPF_and_CAP_PERFMON_security_capabilities

CAP_CHECKPOINT_RESTORE was merged on the master recently and will be available in the next version of the kernel.
torvalds/linux@124ea65

The capability numbers are taken from https://github.com/torvalds/linux/blob/442489c219235991de86d0277b5d859ede6d8792/include/uapi/linux/capability.h#L373-L416

Signed-off-by: Akihiro Suda <[email protected]>
Signed-off-by: Kir Kolyshkin <[email protected]>
Go 1.17 introduced new style of adding build tags (//go:build), and some
tools no longer understand old-style (// +build) tags.

Add the new tag, drop the old one.

Signed-off-by: Kir Kolyshkin <[email protected]>
Move the code to the top-level directory.

Signed-off-by: Kir Kolyshkin <[email protected]>
In case kernel folks will ever release capability v4, the chances are
high v3 is still supported. Therefore, we should not error out upon
seeing an unknown version from the kernel, but assume we can go with v3.

While at it, treat the uninitialized capVers as an error. Before this
patch, it was still treated as an error, but "unknown capability version"
is not exactly what the error is, so let's be more specific.

Reported-by: Andrei Vagin <[email protected]>
Signed-off-by: Kir Kolyshkin <[email protected]>
Capabilities v3 API was added by the Linux kernel 2.6.26.

Since go 1.18 (no longer supported as of go 1.20 release), the minimum
Linux kernel requirement is 2.6.32 (see [1]). So, it does not make sense
to support capabilities v1 and v2 any more.

Drop the support, returning the appropriate error message.

[1] https://tip.golang.org/doc/go1.18#linux

Signed-off-by: Kir Kolyshkin <[email protected]>
Commit f3cb87f added support for ambient capabilities. Unfortunately,
the code added to Apply is incorrect because it uses a local variable
err which is never used or returned.

Found by a linter:

> capability_linux.go:480:5: ineffectual assignment to err (ineffassign)
> 				err = nil
> 				^

Fixes: f3cb87f
Signed-off-by: Kir Kolyshkin <[email protected]>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks! Forgot to submit some comments I had

capability/go.mod Outdated Show resolved Hide resolved
capability/go.mod Show resolved Hide resolved
Comment on lines +1 to +7
Copyright 2013 Suryandaru Triandana <[email protected]>
All rights reserved.

Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions are
met:
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious what the right approach is for this license file; currently this means that golang.org/moby/sys/capability will be licensed as MIT, but the LICENSE at the top of the repository is Apache2.

I know when vendoring other modules from this repository, go mod copies the LICENSE from the top-level, so I wonder what it does for this module (will it copy both? will it only use the MIT license?).

Reading these two StackOverflow answers;


  • At least the original license and copyright must be retained (see below)
  • Possibly as a secondary file (?)
  • Possibly included in a NOTICE file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am now testing this PR in opencontainers/runc#4358, and from what I see, go mod vendor uses top-level LICENSE file for e.g. moby/sys/mount and other packages, and it uses directory-level LICENSE for moby/sys/capability. In other words, it works as it should.

I don't see any problem with this layout -- a LICENSE file in a subdirectory overwrites the upper level LICENSE. If they are compatible (as in our case), all is fine and dandy.

Disclaimer: IANAL.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm somewhat looking from a "module" perspective, because it may become somewhat ambiguous; for the GitHub repo, the license on the repo-level is Apache 2, with a subdirectory having the MIT license.

But from a module perspective, normally it would copy that license; in this case it only has the MIT license. And perhaps that's OK (so, the capabilities module having a different license than the other modules), but was wondering if we wanted to re-license the new module to use Apache 2.

Copy link
Member

Choose a reason for hiding this comment

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

☝️ s/MIT/BSD 2-clause/ (I was confused)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

golang.org/moby/sys/capability will be licensed as MIT

For the sake of those reading this thread. This is actually a BSD 2-clause license, not the MIT license.
See https://opensource.org/license/bsd-2-clause

@kolyshkin kolyshkin force-pushed the add-capability branch 2 times, most recently from 5365f50 to 6a5bb11 Compare September 12, 2024 00:32
kolyshkin added a commit to kolyshkin/runc that referenced this pull request Sep 12, 2024
Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

BSD-2-clause is different from Apache 2, but strictly less restrictive, so is probably OK. Trying to relicense is going to be a huge pain. 👍

This seems fine to me (currently discussing some minor suggested changes in the Moby maintainers meeting, but I don't think there's any strong opposition overall).

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

@AkihiroSuda PTAL if this LGTY

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

Also /cc @dims (FYI - you probably also have an interest in this)

@dims
Copy link

dims commented Sep 13, 2024

of course! this is a welcome development. thanks for the poke @thaJeztah

This integrates github.com/kolyshkin/capability (which itself is a fork
of github.com/syndtr/gocapability) to github.com/moby/sys/capability.

Some of github.com/syndtr/gocapability users are (in an alphabetical
order):

 - https://github.com/canonical/lxd
 - https://github.com/containers/buildah
 - https://github.com/containers/podman
 - https://github.com/containers/skopeo
 - https://github.com/google/gvisor
 - https://github.com/hashicorp/nomad
 - https://github.com/linuxkit/linuxkit
 - https://github.com/opencontainers/runc
 - https://github.com/opencontainers/runtime-tools
 - https://github.com/slimtoolkit/slim

Signed-off-by: Kir Kolyshkin <[email protected]>
Rename a test file so the test it implements is only run on Linux.

Signed-off-by: Kir Kolyshkin <[email protected]>
1. Add "Copyright 2023 The Capability Authors" header to cover my work
   since 2023 as well as any future work in the forked package.

2. Remove (c) after the word "Copyright" (not required since 1989).

3. Remove the comma after a copyright year.

4. Minor formatting fix (move the line break earlier).

Signed-off-by: Kir Kolyshkin <[email protected]>
1. Clarify the fork source.

2. Fix links to 0.1.x PRs to point to the proper (old) repo.

Signed-off-by: Kir Kolyshkin <[email protected]>
It was mistakenly set to Go >= 1.20, while it fact this package requires
Go >= 1.21 (due to the use of sync.OnceValues).

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Collaborator Author

kolyshkin commented Sep 13, 2024

Just pushed a new version, mostly addressing @AkihiroSuda's comments, i.e.

  • fixed CHANGELOG.md to clarify the fork source and fix links to PRs.

But also:

All that (except for the missing commit addition) are added as the last 2 commits, so you are re-reviewing, you can only review these two.

@kolyshkin
Copy link
Collaborator Author

I think we can address any other concerns post-merge.

@kolyshkin kolyshkin merged commit 243daa2 into moby:main Sep 13, 2024
19 checks passed
@thaJeztah
Copy link
Member

Thanks 🙏

kolyshkin added a commit to kolyshkin/runc that referenced this pull request Sep 16, 2024
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.