Skip to content

OpenStack Keystone Authorization#25624

Closed
kfox1111 wants to merge 2 commits intokubernetes:masterfrom
kfox1111:keystoneauthz
Closed

OpenStack Keystone Authorization#25624
kfox1111 wants to merge 2 commits intokubernetes:masterfrom
kfox1111:keystoneauthz

Conversation

@kfox1111
Copy link
Copy Markdown

@kfox1111 kfox1111 commented May 15, 2016

Extends experimental keystone auth support limiting access to a single keystone project.

OpenStack Keystone is mostly a system to provide a central place to manage Authorization for a Mutitenant Cloud. You typically use one Keystone to manage all of you Projects at your organization, of which usually all need to be isolated from each other. Very few clouds don't have a need for isolation.

This is an initial implementation that supports restricting Keystone access to k8s to a single OpenStack Project. This aligns with the OpenStack Magnum use case. Each project launches their own k8s cluster using Magnum, and then wants to login to it using their existing credentials, but block access to other projects. This PR enables that use case.

In future PR's, I hope to implement full Multitenancy so that additionally, one k8s cluster could be used for the entire OpenStack Cloud, and Projects are isolated from each other though some mechanism (perhaps mapping keystone project = namespaces?)

Keystone Authorization plugin was added to support restricting Keystone Access to a single Project.

This change is Reviewable

@k8s-bot
Copy link
Copy Markdown

k8s-bot commented May 15, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

2 similar comments
@k8s-bot
Copy link
Copy Markdown

k8s-bot commented May 15, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link
Copy Markdown

k8s-bot commented May 15, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-github-robot k8s-github-robot added kind/old-docs size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 15, 2016
@philips
Copy link
Copy Markdown
Contributor

philips commented May 15, 2016

Hello @kfox1111-

Perhaps we don't need keystone authentication at all since keystone now supports OpenID Connect and we are planning on getting OIDC integration into v1.3: #25270

For the authorization side of things you may want to see the work that Eric is doing to upstream the Authorization types and API originally in OpenShift: #24900

cc @ericchiang @bobbyrullo @erictune

@kfox1111
Copy link
Copy Markdown
Author

@philips Thanks for the links.

oauth in OpenStack is not well used and is being considered for deprecation:
http://www.gossamer-threads.com/lists/openstack/operators/53507

The authentication code is done in the same way its done in all other OpenStack services so it works well in an OpenStack environment.

I'll read through the authorization thread that you posted. its pretty long though, so might take me a while to get through. Thanks.

@nikhiljindal
Copy link
Copy Markdown
Contributor

@kfox1111 Can you please add some more details to the PR description.
Looks like you are adding a Keystone Authorization mode? Why?

Assigning to @erictune who should be a better reviewer for this.

@kfox1111
Copy link
Copy Markdown
Author

Can someone help me with formatting release notes? I totally don't understand how they work apparently. :/

@philips
Copy link
Copy Markdown
Contributor

philips commented May 16, 2016

@kfox1111 That is OAUTH 1.0 they are deprecating. My understanding is that Keystone also does OAUTH 2.0/OpenID Connect.

@kfox1111
Copy link
Copy Markdown
Author

If they support it, I have'nt found a way to use it. Do you have any documentation on it?

This authz and related authn patch makes Kubernetes authenticate just like any other Service of OpenStack. This uses code should be commonly available to all OpenStack clouds and commonly tested so is unlikely to have issues from code that isn't used much.

I'm worried that it will be quite difficult to use the oauth stuff for both the user and the operator since it is not what they are use to dealing with.

@kfox1111
Copy link
Copy Markdown
Author

I was able to find this:
https://blueprints.launchpad.net/keystone/+spec/openid-connect

But its about using an external idp to authenticate into Keystone, not supporting services where keystone is the idp. I think its going the wrong direction for this use case.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure what this extra check is buying us. I would just check the namespaced keystone projectID/roles data directly.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hmm... Your right. It was an attempt to let service users through but not users authenticated via the keystone plugin. But the authz stuff doesn't work that way... I'll remove it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Actually, I was looking and it does one thing. It logs if the authentication user isn't a keystone token to help you debug why authorization might not be working as you might expect. So I think its useful to leave it in?

Copy link
Copy Markdown
Member

@liggitt liggitt May 17, 2016

Choose a reason for hiding this comment

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

I'd rather make the authorizer check the actual info it cares about, and document that it should typically be used In conjunction with the keystone token authenticator

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok. I'll fix this in the next version.

@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 17, 2016
@erictune
Copy link
Copy Markdown
Contributor

@kfox1111

We are 4 days from a code freeze for the next release. I won't be able to review this before then. Will definitely consider for next release (1.4).

@kfox1111
Copy link
Copy Markdown
Author

@erictune bummer. but I understand. thanks for considering it early in 1.4. Its something the OpenStack folks really need.

@kfox1111
Copy link
Copy Markdown
Author

@erictune actually, an idea... This code is already marked as experimental. Just like the existing keystone password code. Since it is marked as such, can it still make it in? It would allow developers in the OpenStack community to continue to make progress with integrating with k8s in this experimental mode. This might flesh out issues, like those that were discovered with the keystone password module sooner, and possibly make for a better experience when the feature is made unexperimental, hopefully in 1.4? Its mostly just additive so shouldn't break anything outside of keystone token support?

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2016
Comment thread docs/admin/kube-apiserver.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

s/experimental/experimantal/

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

woops. good catch. Fixed in next version.

@erictune
Copy link
Copy Markdown
Contributor

erictune commented May 19, 2016

I'll take this for 1.3 under these condition:

  • you or someone you choose promptly updates documentation at http://kubernetes.io/docs/admin/authorization/ and http://kubernetes.io/docs/admin/authentication/ to descrbe the keystone options and flags. Does do not have the friday deadline, but should happen in next 3 weeks. You can at-mention me on the docs PR and I will review.
  • we both understand that the entire approach is experimental. It may be kept as is and promoted to stable. Or, with some reasonable amount of notice, there may be a request to implement it differently (e.g. using webhook to call to a keystone proxy, or something else entirely), and if no-one steps up to reimplement it, it may be dropped.
  • that the documentation you add states that the "Keystone support is experimental, and may be dropped or redesigned in a future release".

@kfox1111

@harlowja
Copy link
Copy Markdown

Is there a write-up here of what the short, medium term goals actually are for this work. It seems like those goals are being discovered as we go along and I'm thinking we could do better if the k8s community knows more about keystone and how it can be used; just getting a general gist for the thread it appears that information may actually be valuable (and IMHO keystone could actually provide a path to a solution that avoids k8s creating its own equivalent, which would be dumb/waste-of-time/resources).

@kfox1111
Copy link
Copy Markdown
Author

@harlowja Yeah. A lot of the discussion has happened in pr's/issues all over the place. I tried to distil a lot of it into one post here:
#25391 (comment)

Does k8s have a formalized spec kind of thing where we should place this and other relevant bits to the discussion so it can be more easily found?

@k8s-bot
Copy link
Copy Markdown

k8s-bot commented Jun 22, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@harlowja
Copy link
Copy Markdown

How do I complain to to ixdy; cause damn... should be a feature to shutup bots.

@davidopp
Copy link
Copy Markdown
Contributor

Please file an issue in the kubernetes/contrib repo if you have a suggestion for different behavior of the bot. I'd suggest @ mentioning at least one person to make sure somebody sees it.

@harlowja
Copy link
Copy Markdown

Will do, it'd be real nice if there was a way to put 'bot generated' messages in a different tab or something, cause PR comments are already a mess as is so more mess == bad IMHO.

@k8s-bot
Copy link
Copy Markdown

k8s-bot commented Jul 1, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

if err != nil {
return nil, err
if len(config.KeystoneConfig) > 0 {
if config.KeystoneAuthMode == "token" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it not possible to use both password and token mode at the same time in a single apiserver?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We tried it once and it failed. It would be nice if it could though.

@erictune
Copy link
Copy Markdown
Contributor

erictune commented Jul 6, 2016

@kfox1111
Your comment about using "advanced RBAC", are you talking about Kubernetes RBAC (new feature in 1.3) or http://docs.openstack.org/developer/horizon/topics/policy.html?

@kfox1111
Copy link
Copy Markdown
Author

kfox1111 commented Jul 6, 2016

@erictune: yeah, I was meaning hooking the Keystone Project/Role stuff enough into the Kubernetes RBAC system such that users could write RBAC rules that lets particular Keystone Roles on Keystone Projects do certain actions. For example: I may want to write a Kubernetes RBAC rule that lets a user with a "Monitoring" Keystone Role on Keystone Project "foo" access to list all pods on Namespace "foo", but do nothing else.

@erictune
Copy link
Copy Markdown
Contributor

erictune commented Jul 6, 2016

Does keystone assign any meaning to roles, outside of the user/groups/projects they are bound to? Or is it up to each service that uses role-based auth to define its own interpretation of those roles?

@kfox1111
Copy link
Copy Markdown
Author

kfox1111 commented Jul 6, 2016

Its up to each service to determine what can and can not be accessed based on the keystone roles on projects. Each OpenStack service usually uses a policy.json file to map keystone roles to their own api. For example, here's nova's (http://docs.openstack.org/juno/config-reference/content/section_nova-policy.json.html)

I think Kubernetes RBAC system is much more flexible then the policy.json system, but is the same kind of thing. So it would be great to be able to reuse it.

@kfox1111
Copy link
Copy Markdown
Author

kfox1111 commented Aug 5, 2016

This is still being worked on. I hope to get back to it this weekend if time permits.

@k8s-bot
Copy link
Copy Markdown

k8s-bot commented Aug 6, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

1 similar comment
@k8s-bot
Copy link
Copy Markdown

k8s-bot commented Aug 11, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link
Copy Markdown

k8s-bot commented Aug 30, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

@kfox1111
Copy link
Copy Markdown
Author

kfox1111 commented Sep 2, 2016

So, honestly I've kind of lost track of where we are with this. The webhook thing seems to have never materialized. Is this patch set the way we want to go? If so, I can work on finishing up the remaining concerns with it.

@harlowja
Copy link
Copy Markdown

harlowja commented Sep 3, 2016

I'm in favor of exploring with the k8s folks (sig-auth?) before we figure this one out, seem ok?

@liggitt
Copy link
Copy Markdown
Member

liggitt commented Sep 3, 2016

Agreed, we can talk during the sig meeting or schedule a dedicated meeting if we run out of time

@erictune
Copy link
Copy Markdown
Contributor

erictune commented Sep 6, 2016

Looks like this is on the agenda for tomorrow. Agenda is here:
https://docs.google.com/document/d/1woLGRoONE3EBVx-wTb4pvp4CI7tmLZ6lS26VTbosLKM/edit

On Fri, Sep 2, 2016 at 8:26 PM, Jordan Liggitt notifications@github.com
wrote:

Agreed, we can talk during the sig meeting or schedule a dedicated meeting
if we run out of time


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#25624 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHuudiHOYauFlPaUn-8Rr6qRDq2K1uoIks5qmOjfgaJpZM4IevdB
.

@@ -82,6 +82,10 @@ kube-apiserver
--etcd-servers=[]: List of etcd servers to connect with (http://ip:port), comma separated.
--etcd-servers-overrides=[]: Per-resource etcd servers overrides, comma separated. The individual override format: group/resource#servers, where servers are http://ip:port, semicolon separated.
--event-ttl=1h0m0s: Amount of time to retain events. Default 1 hour.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think, --experimental-authorization-keystone-role and --experimental-authorization-keystone-single-projectid, these two args can also be included in the Keystone config file. As these args are all passed to kube-apiserver when bootstrapping kubernetes env, all the configurations are fixed unless restarting kube-apiserver.

@idvoretskyi idvoretskyi added the area/provider/openstack Issues or PRs related to openstack provider label Nov 16, 2016
@k8s-github-robot
Copy link
Copy Markdown

This PR hasn't been active in 90 days. Closing this PR. Please reopen if you would like to work towards merging this change, if/when the PR is ready for the next round of review.

cc @erictune @kfox1111

You can add 'keep-open' label to prevent this from happening again, or add a comment to keep it open another 90 days

@attardi
Copy link
Copy Markdown

attardi commented Oct 15, 2017

@kfox1111 I would be interested in the Multitenant version, mapping project to namespaces. Have you worked on it?

@kfox1111
Copy link
Copy Markdown
Author

I have not. @dims is more active on the subject these days.

@dims
Copy link
Copy Markdown
Member

dims commented Oct 17, 2017

@attardi @kfox1111 - The last word is we should use a web hook, there's code in https://github.com/dims/k8s-keystone-auth we can collaborate there or move it to another common github org or gerrit in openstack

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/provider/openstack Issues or PRs related to openstack provider needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.