API Entitlement by ashfall · Pull Request #61 · moby/libentitlement

ashfall · 2017-10-25T21:20:48Z

API entitlement allows a container management software to store at the entitlement-level the APIs and API subsets that the container is allowed [or not] to access.

The API entitlement's id api.access and takes a string argument of the following format: api-identifier:api-subset:access-rule (whole entitlement string should be api.access=id:subset:access;

api-identifier is the API identifier that the container management software wants to manage access to
api-subset allows the container management software to define if it wants to regulate access to the whole API (through the supported keyword all or only a subset, ex:engine:swarm or k8s:pod-create)
access-rule must be either deny or allow

We provide a default setup in Moby for Swarm, we should provide one for kub too.

Note:
The API entitlement is the first entitlement taking a string as a parameter so rework has been done on this end.

ashfall

@n4ss Pushed a WIP version -- had some questions below, which we couldn't discuss when pairing.

ashfall · 2017-10-25T21:21:36Z

secprofile/oci_profile.go

+
+const (
+	Allow  APIAccess = "allow"
+	Access APIAccess = "access"


Why do we need APIAccess="Access"?

hmm I only remember us deciding on:

Access APIAccess = "allow" Deny APIAccess = "deny"

Why adding Access APIAccess = "access" ?

Also for the type, it's better to have a different type when it is enumerable.

I believe this is a typo from our pairing session. I'll keep Allow and Deny, and remove the third line

n4ss@671f14b#diff-68edf18e069363d58649944c4d3ec6fbR17

Indeed, my bad!

LGTM for Allow/Deny! ^_^

ashfall · 2017-10-25T21:22:24Z

defaults/api_ents_test.go

+	apiEnt := DefaultEntitlements[entitlementID]
+
+	_, err := apiEnt.Enforce(ociProfile)
+	require.Equal(t, fmt.Errorf("OCI profile's APIAccess field nil"), err)


How do I set up the ociprofile so its APIAccess field is not nil?

We didn't finish this part last time, it was in the todo list iirc. NewOCIProfile() would need an additional parameter.

ashfall · 2017-10-25T21:25:15Z

secprofile/oci_profile.go

+		return &OCIProfile{
+			OCI:           ociSpec,
+			AppArmorSetup: nil,
+			APIAccess:     &APIAccessConfig{make(map[APIID]map[APISubsetId]APIAccess)},


I think the parsed APIAccess string needs to be passed to NewOCIProfile function

n4ss · 2017-10-26T14:16:55Z

defaults/api_ents_test.go

+	apiEnt := DefaultEntitlements[entitlementID]
+
+	_, err := apiEnt.Enforce(ociProfile)
+	require.Equal(t, fmt.Errorf("OCI profile's APIAccess field nil"), err)


We didn't finish this part last time, it was in the todo list iirc. NewOCIProfile() would need an additional parameter.

n4ss · 2017-10-26T14:18:16Z

defaults/api_ents.go

+	engineDomain = "engine"
+
+	// FIXME: we need to define the possible domain, identifier and value for the entitlement.
+	APIEntID = engineDomain + ".api"


Since api would be the entitlement's domain and engine the subdomain and swarm the ID, it'd make more sense to have api at the beginning I think and the following format:
docker run --entitlements="api:engine.swarm=all:deny"

WDYT?

Sounds good!

n4ss · 2017-10-26T14:29:57Z

defaults/api_ents.go

+
+	apiSubsetAndAccessFields := strings.Split(apiSubsetAndAccess, ":")
+	if len(apiSubsetAndAccessFields) != 3 {
+		return nil, fmt.Errorf("Wrong API subset and access format, should be \"api-id:subset:[allow|deny]\"")


Follow-up from previous comment
\"api-id:subset:[allow|deny]\" -> \"api-id=subset:[allow|deny]\"

n4ss · 2017-10-26T14:30:49Z

defaults/api_ents.go

+	}
+
+	apiSubsetAndAccessFields := strings.Split(apiSubsetAndAccess, ":")
+	if len(apiSubsetAndAccessFields) != 3 {


We should also split by = as : won't return every field.

n4ss · 2017-10-26T14:32:10Z

defaults/api_ents_test.go

+	_, err := apiEnt.Enforce(ociProfile)
+	require.Equal(t, fmt.Errorf("OCI profile's APIAccess field nil"), err)
+
+	//	require.NoError(t, err, "Failed to enforce while testing entitlement %s", entitlementID)


You can probably remove all these comments as we won't test the OCI specs content in this entitlement

n4ss · 2017-10-26T14:33:43Z

defaults/api_ents.go

+	// FIXME: we need to define the possible domain, identifier and value for the entitlement.
+	APIEntID = engineDomain + ".api"
+	// e.g. defining an ent like so: "engine.api=swarm:all:allow"
+	APIEntAllowID = APIEntID + "=swarm:all:allow"


APIEntAccessID ? (access can be either allow or deny, people might think this only whitelists)

So, at present, the value (allow/deny) is processed by NewStringEntitlement (through parser.ParseStringEntitlement). this means ha we'll need to have two different API IDs for alllow and deny, so the access specified is no lost in translation.

Since this is the first Entitlement to have a value, we might be able to account for the value processing elsewhere perhaps?

n4ss · 2017-10-26T14:34:40Z

defaults/defaults.go

 	HostProcessesNoneEntFullID:  entitlement.Entitlement(hostProcessesNoneEntitlement),
 	HostProcessesAdminEntFullID: entitlement.Entitlement(hostProcessesAdminEntitlement),
+
+	APIEntAllowID: entitlement.Entitlement(apiEntitlement),


Same comment as before on the APIEntAccessID

n4ss · 2017-10-26T14:35:28Z

secprofile/oci_profile.go

 }

 // NewOCIProfile instantiates an OCIProfile object with an OCI specification structure
 func NewOCIProfile(ociSpec *specs.Spec, apparmorProfileName string) *OCIProfile {


We should probably have one more parameter to specify APIAccess ^_^

Signed-off-by: Ashwini Oruganti <ashwini.oruganti@gmail.com> (github: ashfall)

codecov-io · 2017-11-01T21:52:34Z

Codecov Report

Merging #61 into master will increase coverage by 3.8%.
The diff coverage is 91.34%.

@@           Coverage Diff            @@
##           master     #61     +/-   ##
========================================
+ Coverage    64.7%   68.5%   +3.8%     
========================================
  Files          16      18      +2     
  Lines        1136     962    -174     
========================================
- Hits          735     659     -76     
+ Misses        344     247     -97     
+ Partials       57      56      -1

GordonTheTurtle · 2017-11-02T21:36:16Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "api-entitlement-2" git@github.com:ashfall/libentitlement.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354312520
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Signed-off-by: Ashwini Oruganti <ashwini.oruganti@gmail.com> (github: ashfall)

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Signed-off-by: Ashwini Oruganti <ashwini.oruganti@gmail.com> (github: ashfall)

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Signed-off-by: Ashwini Oruganti <ashwini.oruganti@gmail.com> (github: ashfall)

n4ss · 2017-11-10T00:37:19Z

defaults/api_ents.go

+const (
+	// APIEntFullID is the API entitlement identifier; the value format is: "api.access:api-id:subset:[allow|deny]"
+	// ex: "api.access:engine.swarm:all:allow"
+	APIEntFullID = "api.access"


We could only use 'api', this means that we have to update the DNS convention of accepting an empty domain for entitlements. Plenty of stuff to rewrite in that case.

We should open an issue for this but it's probably low priority.

n4ss · 2017-11-10T00:40:08Z

defaults/api_ents.go

+	}
+
+	if ociProfile.APIAccessConfig == nil {
+		return false, secprofile.Allow, fmt.Errorf("OCI profile's APIAccess field nil")


Comment for all errors, we should probably have all these in an error package so that we can reuse them easily in code + test. (less error-prone)

n4ss · 2017-11-10T02:40:03Z

defaults/api_ents.go

+
+	apiSubset, ok := ociProfile.APIAccessConfig.APIRights[swarmAPIFullID]
+	if !ok || apiSubset == nil {
+		return false, secprofile.Allow, nil


secprofile.Allow should probably be the result of a GetDefaultAccess() function accessible outside of this package.

n4ss · 2017-11-10T19:18:35Z

defaults/api_ents.go

+	access := secprofile.APIAccess(accessStr)
+
+	switch access {
+	case secprofile.Deny, secprofile.Allow:


nit: We can also provide an isValidAccessString(string) to do that if we come to find other spots where we need it. low priority

n4ss · 2017-11-10T19:20:25Z

can you take a look plz @riyazdf?

riyazdf · 2017-11-14T18:54:46Z

defaults/api_ents.go

+// Default known APIs and API subsets to control access of
+const (
+	// EngineAPI defines the Moby-Engine API
+	EngineAPI = "engine"


We need to enforce an API version, as both of these APIs may add new endpoints or introduce breaking changes

Good catch, let's create a set-able version variable so that Moby can tune it. Do you think we should accept different rules for different versions of the API?

is there a valid scenario for running multiple versions of the API simultaneously? I can't think of one, so I think it should just be a single version at a time.

riyazdf · 2017-11-14T18:56:23Z

defaults/api_ents.go

+
+/*IsSwarmAPIControlled checks if Moby Swarm API is controlled and whether it's allowed or not
+ * Return values are the following:
+ * isControlled - if no error is encountered, whether the Swarm API is currently controlled by the entitlements


should this just be captured in access? If isControlled is false, we can just return access == secprofile.Allow

Hmm I thought IsSwarmAPIControlled would make more sense as the default behavior should be controlled by the container management platform.

Follow-up note: we should have a set-able DefaultAccess that'd be returned in case of an error.

riyazdf · 2017-11-14T19:02:19Z

parser/parser.go


 import (
 	"fmt"
+	"github.com/Sirupsen/logrus"


non-blocking nit: let's use a separate import block for non-standard packages

riyazdf · 2017-11-14T19:03:33Z

parser/parser.go


 	idAndArgList := strings.Split(idAndArgString, "=")
-	if len(idAndArgList) != 2 {
+	id = idAndArgList[0]


non-blocking nit: this assignment should remain after the if len(idAndArgList) > 2 { check

/me shame
Good catch!

n4ss · 2017-11-15T19:52:12Z

defaults/api_ents_test.go

+	require.NoError(t, err)
+
+	_, err = apiEnt.Enforce(ociProfile)
+	require.Equal(t, err, fmt.Errorf("Wrong API subset and access format, should be \"api-id:subset:[allow|deny]\""))


If we plan to add more errors, it'd probably be better to make a test-slice/map for a cleaner function

riyazdf · 2017-11-15T23:14:23Z

defaults/api_ents.go

+	}
+
+	apiIDStr := apiToAccessFields[0]
+	apiSubsetStr := apiToAccessFields[1]


Is the subset string the raw API path or a different mapping? And if so to the latter, how is it specified?

And do we plan to limit certain arguments? For example, --privileged since it's settable with container/create?

riyazdf · 2017-11-15T23:15:50Z

defaults/api_ents.go

+// Default known APIs and API subsets to control access of
+const (
+	// EngineAPI defines the Moby-Engine API
+	EngineAPI = "engine"


Instead of engine and swarm, should this instead be image/container/volume/swarm/etc as specified in the Docker API docs? https://docs.docker.com/engine/api/v1.33 I'm not sure I understand the distinction here

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

justincormack · 2017-12-28T23:14:50Z

How is this expected to be implemented?

justincormack · 2018-01-02T14:17:14Z

Originally I was thinking that this would be just a control for bind mounting the socket, but obviously that can't do access control unless there is an authz plugin configured. But in that case the container needs a user identity, so we would then need container identity, and a user cert for this use case. But then it starts to look like generic API access control, not a specific entitlement, and is not much different from another remote API.

See also discussion in moby/moby#35711 about TLS for unix socket

ashfall commented Oct 25, 2017

View reviewed changes

ashfall requested a review from n4ss October 25, 2017 21:26

n4ss reviewed Oct 26, 2017

View reviewed changes

ashfall force-pushed the api-entitlement-2 branch from 34a268c to d275458 Compare October 30, 2017 20:04

ashfall added 2 commits October 30, 2017 13:06

Add APIEntitlements

345fd6f

Signed-off-by: Ashwini Oruganti <ashwini.oruganti@gmail.com> (github: ashfall)

Hold for move

4ea5b4f

Signed-off-by: Ashwini Oruganti <ashwini.oruganti@gmail.com> (github: ashfall)

ashfall force-pushed the api-entitlement-2 branch from d275458 to 4ea5b4f Compare October 31, 2017 22:14

Finish move, modify the entitlement ID a bit.

5051a30

GordonTheTurtle added the dco/no label Nov 2, 2017

n4ss and others added 17 commits November 3, 2017 16:11

Add a default entitlements getter and fix api entitlement + tests

9467b7f

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Adjust parser for optional string values

fcea91a

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Authorize optional string values

00fa9ca

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Fix api entitlement - temporary rename

f5b60c1

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Revert to use old entitlement format

46c83c6

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Fix the documentation with the new api entitlement id

5011dc8

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Add unit tests for GetSwarmAPIIdentifier and IsSwarmAPIControlled

ab7bbd4

Signed-off-by: Ashwini Oruganti <ashwini.oruganti@gmail.com> (github: ashfall)

Add an API access rule overriding test

93bf346

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Improve tests and fix ent ID in logs

bfbebb8

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Add test for bad apiToAccessFields format

e8c957c

Signed-off-by: Ashwini Oruganti <ashwini.oruganti@gmail.com> (github: ashfall)

Test for the case when the apiAccess string has invalid access controls

c2bf835

Signed-off-by: Ashwini Oruganti <ashwini.oruganti@gmail.com> (github: ashfall)

Add test cases for string ent SetValue

279de90

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Fix API entitlement full ID in enforcement callback

f32df62

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

DRY

1c9312e

Signed-off-by: Ashwini Oruganti <ashwini.oruganti@gmail.com> (github: ashfall)

Test apiEnt enforce with invalid profile type

25e8c8a

Signed-off-by: Ashwini Oruganti <ashwini.oruganti@gmail.com> (github: ashfall)

Test profile with nil APIAccessConfig

014d3e7

Signed-off-by: Ashwini Oruganti <ashwini.oruganti@gmail.com> (github: ashfall)

Set APIAccessConfig to nl in test

065833e

Signed-off-by: Ashwini Oruganti <ashwini.oruganti@gmail.com> (github: ashfall)

ashfall changed the title ~~WIP: API Entitlements~~ API Entitlements Nov 10, 2017

n4ss reviewed Nov 10, 2017

View reviewed changes

riyazdf reviewed Nov 14, 2017

View reviewed changes

n4ss reviewed Nov 15, 2017

View reviewed changes

riyazdf reviewed Nov 15, 2017

View reviewed changes

n4ss added 8 commits December 21, 2017 15:25

Update vendoring to oci specs 1.0 and sirupsen

21f41e5

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Add SYS_RESOURCE to security admin ent and update the syscall list

04e3f5e

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Add a version for engine api in moby defaults

3e1fbcb

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Finish updating vendored libs

b8b5b0c

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Update example on engine version format

d8161f1

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Update syscalls for security entitlement test

0a60e97

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Check id and arg list length before dereferencing

d60f217

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

Update test OCI struct to v1.0.0

874f280

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

GordonTheTurtle added the dco/no label Dec 21, 2017

n4ss mentioned this pull request Dec 21, 2017

For API entitlement: allow to define the default access behavior #67

Closed

n4ss merged commit 80ec3b1 into moby:master Dec 21, 2017

n4ss changed the title ~~API Entitlements~~ API Entitlement Dec 21, 2017

Comments

Conversation

ashfall commented Oct 25, 2017 • edited by n4ss Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashfall left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n4ss Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n4ss Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

GordonTheTurtle commented Nov 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n4ss commented Nov 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n4ss Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

ashfall commented Oct 25, 2017 •

edited by n4ss

Loading

ashfall left a comment •

edited

Loading

n4ss Oct 26, 2017 •

edited

Loading

n4ss Nov 1, 2017 •

edited

Loading

codecov-io commented Nov 1, 2017 •

edited

Loading

n4ss Nov 15, 2017 •

edited

Loading

justincormack commented Jan 2, 2018 •

edited

Loading