-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
Docker authorization plug-in infrastructure #15365
Conversation
ea1523c
to
4544302
Compare
cc @icecrime @crosbymichael @NathanMcCauley @diogomonica perhaps you can have a look at this? General API description can be found in this document |
if err != nil { | ||
return err | ||
} | ||
a.plugins = append(a.plugins, p) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to parse and build this on every request? Could instead the Ctx
struct be built with a set of authZPlugins
of type []*plugins.Plugin
? Or better yet does it make sense to create a typed plugin that could be plugins.AuthZPlugin
?
Should there any limitations to what the Response rewriters can and cannot do? It looks like the proposal allows arbitrary modifications? I'm concerned that'll mean that we end up with accidental dependencies on the structure of the Docker Remote API request/response structure as 3rd party authz plugins now depend on being able to directly parse (and modify) the response bodies. |
How will this handle cases where data is streamed live back to the client, as in the case of the |
} | ||
|
||
// Request is the expect object that returns from the authZ plugin | ||
type Response struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #14674 @dimastopel mentions "Whether the next plug-in should be called or not for this specific notification (default is yes)." I don't see that represented in the API? Has that notion been left out of the proposal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have decided to go with a simpler approach as the first step and tune / optimize as we go. Current approach is to simply AND between all the plugins and only proceed if the access is approved by all. This can definitely be revisited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NathanMcCauley, thanks for you comments.
Re /logs and /attache, the authentication framework should only be called on the HTTP requests. Once the http connection is hijacked, no additional authorization should to be perform. This pattern enables full authorization capabilities to docker as described in the current requirements (for framework and specific RBAC plugin). The only change in current code is to replace the response recorder with concrete object that implements hijack.
Re response rewrites, since the api version is exposed to the plugin, I think it should be the plugin responsibility to determine whether it can or cannot modify the response or which parts can be changed. As far as I know, the docker daemon api is backward compatible, so plugins are essentially equivalent to regular client communicating with the api.
I totally agree with other comments, please let me know your thoughts on the above items, and I will modify the code appropriately.
c08f3e1
to
cd1c909
Compare
Added unit tests and fixed implementation according to comments. |
@icecrime @crosbymichael @NathanMcCauley @diogomonica |
How does this work w PAM, it would be nice if it was easy for people to understand and familiar the design that way |
Thanks for the review @diogomonica, @NathanMcCauley, @endophage, We've updated the design doc and answered all comments, looking forward to discuss further and agree on the right approach for plugin authorization.
I personally think both solution 1 and 2 are plausible, but the former (1) will require much more logic in the daemon core, and will have to be maintained anytime new commands are added (which can have some maintenance pain). What do you think? |
@jfrazelle PAM is one of the mechanisms which can be used for authentication by plugins which will be built on top of the authentication framework (#13697) that is currently under development. Authorization plugins will depend upon the authentication data to achieve access control with greater granularity than the current dichotomy of full access or no access. |
@liron-l Looking at the options you've laid out, I would tend to prefer the third from an architectural perspective, as it does not require additional requests to the public API. However, I can certainly understand how this might massively increase overhead, particularly in cases dealing with images. Option one would be a good alternative, assuming we have a reliable way of authenticating plugins (perhaps they could declare a public key in the plugin specification?). I really don't like the second option - it seems the sort of thing where it would be very easy for a user to shoot themselves in the foot if they had multiple authorization plugins. As a possible alternative, perhaps plugins could tell the daemon they required more information to make a decision, and the daemon could reissue the authorization request with additional information on the image or container in question. This would significantly increase the complexity of authorization responded and the complexity of the daemon code, but would remove the need for additional requests from the authorization plugin to the public API. |
ping @diogomonica, @NathanMcCauley, @jfrazelle PTAL (#15365 (comment)) |
I'd rather avoid premature optimizations. That will allow us to get this in sooner rather than later. If we don't need cross references with the daemon, let's not add them from now. From a design point of view, this change looks good to me, but I'll wait for other maintainers to confirm. https://github.com/docker/docker/blob/master/api/server/middleware.go |
Should be singular. |
@liron-l can you change the flag to |
Signed-off-by: Liron Levin <liron@twistlock.com>
Thanks @thaJeztah, we've replaced all occurrences of the authz argument. |
Thanks @liron-l, awesome!! |
Docker authorization plug-in infrastructure
Awesome. Great to finally see some form of ACL support in Docker. It's been a long time coming. 😸 |
Thanks @thaJeztah ... sorry to be MIA, I'll take a look at the pr and carry any changes that might be necessary. I'll ping @liron-l if that is actually necessary. |
just realized this also needs changes to the completion scripts. ping @albers @sdurrheimer if you have time to look at those? Otherwise I can see if there's a maintainer able to look at them ❤️ |
Sure, I'll take care of bash completion. Thanks for pinging me. |
@albers you da bomb! On Mon, Dec 14, 2015 at 5:19 AM, Harald Albers notifications@github.com
|
So do I with zsh completion. If I understand well, there is only a single new option |
@sdurrheimer correct! And to use @jfrazelle's words; you're also da bomb! thanks so much |
Been following this one. Awesome job. |
@doronp no, that still won't be possible with these changes. Container names need to be unique per daemon, however, an Authz plugin would probably be able to prevent a user from connecting to a network that is not owner by that user, or use an image that's not allowed to be used by that user. There's lots of things involved in a multi-tenant setup, and this can help, but won't solve all. |
It could also concatenate a unique ID such as user ID to the container name (modify request) and filter it out (modify response) in every response for example. |
@doronp yes, that would partly solve the problem, but may be quite tedious, and won't be a full solution; containers and images can be accessed by ID as well. Also think of (e.g.) volumes, and bind-mounted directories, people running docker-in-docker. This PR offers the option to develop plugins to limit access, but for multi-tenant situations there are still lots of things to tackle. Writing up a list of things that may be needed to cover that can be a start; if you have ideas, feel free to open an issue (there may already be issues in that area) |
REMARK: Code is intended to be complementary to the docker (authZ discussion) [https://github.com//issues/14674]. There are several aspects of the proposed code which require additional work and testing. Please use this commit as a low level design proposal.
Docker authorization plug-in infrastructure enables extending the functionality of the Docker daemon with respect to user authorization.
The infrastructure enables registering a set of external authorization
plug-in. Each plug-in receives information about the user and the
request and decides whether to allow or deny the request. Only in case
all plug-ins allow accessing the resource the access is granted.
Each plug-in operates as a separate service, and registers with Docker
through general (plug-ins API)
[https://blog.docker.com/2015/06/extending-docker-with-plugins/]. No
Docker daemon recompilation is required in order to add / remove an
authentication plug-in. Each plug-in is notified twice for each
operation: 1) before the operation is performed and, 2) before the
response is returned to the client. The plug-ins can modify the response
that is returned to the client.
The authorization depends on the authorization effort that takes place
in parallel [https://github.com//issues/13697].
This is the official issue of the authorization effort:
#14674
(Here)[https://github.com/rhatdan/docker-rbac] you can find an open
document that discusses a default RBAC plug-in for Docker.