Skip to content
This repository has been archived by the owner on Feb 1, 2021. It is now read-only.

ACLs in Swarm see #1199 (ReOpen for #1365) #1366

Closed
wants to merge 1 commit into from

Conversation

doronp
Copy link
Contributor

@doronp doronp commented Nov 2, 2015

ACLs in Swarm see #1199 #1365

Authorization (ACLs) support in Swarm as a first step towards multi tenancy. #1199

This PR includes (organized by Commits for convenience)

1 - Validation API & Default implementation- Approve/Disapprove/condition of 'Who wants to to what'?
2 - Handler API & Default implementation - Use labels to enforce decision of 1
3 - Feature flag --multiTenant to get this feature going (regular Swarm otherwise)

(Re open PR for internal PM reasons)

@thaJeztah
Copy link
Member

@vieux @aluzzardi should this be looked at together with moby/moby#13697 and moby/moby#17412 (@NathanMcCauley is currently working with them on that proposal)

@doronp
Copy link
Contributor Author

doronp commented Nov 2, 2015

@abronan @chanwit Done with Most of the comments (except Shell tests should be converted to BATS I think)
I agree that moving to godep should happen at a later phase.
Kindly put the tags from the old PR to this one and lets continue our work here.

@abronan
Copy link
Contributor

abronan commented Nov 2, 2015

@doronp done! :) But it seems that some of your commits are still not signed though :( Also would be better if you can squash some commits (like all those Fix for golint mostly). Especially if you plan to add some more on top.

@thaJeztah Thanks this is helpful! We'll take a look at it as this seems to be closely related.

@tiborvass
Copy link
Contributor

FYI there's also moby/moby#17412 and moby/moby#17687 dealing with this.

@doronp
Copy link
Contributor Author

doronp commented Nov 8, 2015

@chanwit : Added BATS tests, however since the tests are run without the --multiTenant flag they fail and hence the build fails. Can you put the Flag on for those tests?
P.S. The tag milestone 1.1.0 did not move from the closed PR (#1365) if it matters.
P.P.S When squashing I let an older commit slip inside (not mine) - Should I override that or is that fine?
@tiborvass: Tnx.

@abronan
Copy link
Contributor

abronan commented Dec 12, 2015

@doronp Digging into this one. We are seeing the first bits being merged in docker with moby/moby#15365 (for authZ support with plugins). I wonder how we can think about an integration and use the support straight out of docker maybe? This would simplify the code tremendously on the Swarm side. WDYT?

@doronp
Copy link
Contributor Author

doronp commented Dec 15, 2015

@abronan.

Indeed it would be neat to harness the docker side authZ support.
I am not completely sure what you had in mind but I can assume at least two additional approaches:

1 - Swarm will pass the requests to the engines in its cluster and those will communicate with the AuthZ back-end via the plugin - Swarm will then aggregate and handle the responses.

I suspect there will be lots of inefficacy in this approach - Swarm will have to access the engines directly every time - it would not be able to use the internal cluster data as to complete operation as it does today*. This will probably have tremendous performance impact especially on bigger cluster (which is a reasonable assumption if someone uses ACL at all)
for example:
ps -a - list all container will access all over the cluster - then filter out to show only the users containers etc.

*We can keep the operations such as ps in Swarm and only make it do AuthZ via the engines but it is still inefficient.

2 - Take the AuthZ(+AuthN) plugin from docker and make swarm do that job itself - Swarm is just like an engine in that case

I am going on digging the possible implementation options.

let me know what you think.

@doronp
Copy link
Contributor Author

doronp commented Feb 16, 2016

@vieux @abronan @aluzzardi commit 497e328 is the plugin package import - It does not interfere with functionality and passes all tests. It will help a lot to review and merge it ASAP - better not mix it with the AuthZ plugin code (last commit is WIP) Thanks.

@abronan
Copy link
Contributor

abronan commented Feb 16, 2016

ping @docker/swarm-maintainers

@doronp doronp force-pushed the ACLs branch 8 times, most recently from c916115 to d09fab7 Compare March 29, 2016 08:17
@doronp
Copy link
Contributor Author

doronp commented Mar 29, 2016

@MHBauer There was a huge rebase maily #1880
Had lots of the same changes which were pre requisites for the AuthZ plugin usage..

That made separating Godeps and AuthZ plugin actual usage less relevant.

About the next step of actually importing and using AuthZ plugin from the Engine:
I did try to test the authZ usage itself and I get conflict version of the discovery version used in it and in swarm:

cannot save github.com/docker/docker/pkg/authorization at revision 6eb38359954269103f2a3094283c4cb0a1cb63f9: already have github.com/docker/docker/pkg/discovery at revision b65fd8e879545e8c9b859ea9b6b825ac50c79e46.

Does that mean that docker authZ and swarm should decide on one revision? Its a bit of a blocker...

About the failing tests - I am not sure this is related to my changes.

@@ -7,6 +7,8 @@ import (
"net/http/pprof"

log "github.com/Sirupsen/logrus"
// "github.com/docker/docker/pkg/authorization"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you remove it for now instead of commenting out ? Did you comment out the sections only due to the godep issue you mentioned above?

@doronp
Copy link
Contributor Author

doronp commented Mar 29, 2016

@ezrasilvera Good catch. The comment out is due to Godeps indeed. Without that code there is no commit really, with it the error you get is misleading since I could not update the Godeps yet. I will try to determine if I can persuade one side to use the others version to resolve that.

@doronp doronp force-pushed the ACLs branch 2 times, most recently from d1d732a to 419dc7f Compare March 29, 2016 13:18
@doronp
Copy link
Contributor Author

doronp commented Mar 29, 2016

So I changed the revision in the Swarm side it compiles now with the AuthZ import and passes basic manual tests.

@doronp
Copy link
Contributor Author

doronp commented Apr 4, 2016

@MHBauer Ping

@MHBauer
Copy link
Contributor

MHBauer commented Apr 12, 2016

Be forewarned, moby/moby#21897 is about to move the auth stuff around.

@duglin
Copy link
Contributor

duglin commented Apr 12, 2016

@vieux @abronan what do you think about the design of this?

@MHBauer
Copy link
Contributor

MHBauer commented Apr 12, 2016

I think the point of #1366 (comment) was if we could just init the Middleware directly without copying the init into swarm. newAuthorizationMiddleware is already out of date due to changes in docker-engine. It appears to be just a copy of the initial version. If that was not the intent of the comment, it is still what I myself think.

@MHBauer
Copy link
Contributor

MHBauer commented Apr 12, 2016

We'd have to reach directly into docker/docker/api/server/middleware to do that though. (Un)Fortunately that is moved by moby/moby#21897 into a pkg so it will be easy to grab in the future.

I can see that was considered, because it is commented out in primary.go. Some general cleanup to remove comments would be nice.

@doronp
Copy link
Contributor Author

doronp commented Apr 12, 2016

@MHBauer The reason newAuthorizationMiddleware is copied is that it is a bit different, Swarm handlers call chain does not support returning error whereas the dockers does.

In order to avoid that (almost a) copy there needs to be done another change of behaviour in Swarm which is that the handler being used should be able to return error and be like the one in Docker.

When that is done the behaviour could be the same as in Dockers middle wear. And then we can avoid that private function there and use Dockers.

I was planning to do that on another PR since it is not a small change.

Cleaned and re based.

… from docker engine

Signed-off-by: Doron Podoleanu <doronp@il.ibm.com>
@doronp
Copy link
Contributor Author

doronp commented Apr 19, 2016

@dongluochen - @abronan advised me you can help with this. What are the next steps on my side? 10x.

@ezrasilvera ezrasilvera deleted the ACLs branch June 8, 2016 15:30
@hsluoyz
Copy link

hsluoyz commented May 22, 2017

Hi, may I ask what status of the ACL integration for Swarm is right now?

I have developed a Docker auth plugin called casbin-authz-plugin, and I want it to work on Docker Swarm too. What is the suggested way to do this? Thanks!

@dongluochen
Copy link
Contributor

@hsluoyz Is your plugin for Docker Swarm-mode? You can find the difference of classic Swarm and Swarm-mode here. You may open issue at https://github.com/moby/moby for Swarm mode authentication.

@hsluoyz
Copy link

hsluoyz commented May 23, 2017

Hi @dongluochen ,

Is your plugin for Docker Swarm-mode?

No, my plugin is still a local Docker engine plugin base on: https://github.com/docker/go-plugins-helpers/tree/master/authorization

You can find the difference of classic Swarm and Swarm-mode here.

So I think I should develop the auth plugin based on the new Swarm-mode on the moby repo.

You may open issue at https://github.com/moby/moby for Swarm mode authentication.

I didn't find any information about how to develop an authorization plugin for Docker Swarm mode, I will open an issue in moby to ask about it as you suggested, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet