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

Support trust id as a scope in the OpenStack authentication logic #32111

Merged
merged 2 commits into from
Oct 14, 2016

Conversation

MatMaul
Copy link
Contributor

@MatMaul MatMaul commented Sep 6, 2016

This patch allows the use of Kubernetes with Keystone trust delegation to avoid passing the user credentials in clear inside the config file : a specific user with delegated rights can be created and used instead.


This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply "ok to test".

Regular contributors should join the org to skip this step.

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Sep 6, 2016
@MatMaul
Copy link
Contributor Author

MatMaul commented Sep 6, 2016

Please not merge for the moment : the CLA is with my old employer, I am working on signing a new one here. I would still appreciate comments however :)

@liggitt
Copy link
Member

liggitt commented Sep 6, 2016

can you split out the godep/vendor changes into their own commit (with a title of bump(github.com/rackspace/gophercloud): c90cb954266e1bdd6d1914678fd6909fc5fabbfa)

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added cla: no and removed cla: yes labels Sep 6, 2016
@MatMaul
Copy link
Contributor Author

MatMaul commented Sep 6, 2016

Thanks for the answer, done. And I managed to remove the previous CLA, let's wait for the new one.

@@ -106,6 +108,7 @@ type Config struct {
ApiKey string `gcfg:"api-key"`
TenantId string `gcfg:"tenant-id"`
TenantName string `gcfg:"tenant-name"`
TrustId string `gcfg:"trust-id"`
Copy link
Member

Choose a reason for hiding this comment

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

don't we also need to populate AuthOptions.TokenID if you want to use passwordless TrustId? Just looking through AuthenticateV3Trust/trustv3auth in the godeps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually using a token here would defeat the main use case of trusts : if we just want to delegate rights for a small time window passing a token would be enough. However in the OpenStack driver use case Kubernetes is a long living service and its lifetime is way higher than the token lifetime (usually a day). In this case the trust feature allows to delegate user rights to another user, that would have been created specifically for the Kubernetes cluster. Hence we do not leak the user credentials, only the creds of the user specifically created, which we don't care. And we can specify the lifetime we want for the trust when creating it (can be unlimited).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding the code, indeed it is using a first token to get a new token scoped with the trust delegation, but this first token is automatically created from the full creds if specified.

Copy link
Member

@liggitt liggitt Sep 7, 2016

Choose a reason for hiding this comment

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

this first token is automatically created from the full creds if specified

if the token is not specified and the username/password is? just making sure I'm understanding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exactly.

Copy link
Member

Choose a reason for hiding this comment

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

so how does this "avoid passing the user credentials in clear inside the config file"? it looks like we still have to have those

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you need to create a specific Kubernetes user, you will leak the specific user creds but we don't care since it was created for this on purpose, and you can revoke the trust delegation between the specific user and yourself whenever you want : in this case the specific user become powerless.
http://blogs.rdoproject.org/5858/role-delegation-in-keystone-trusts

@liggitt liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Sep 7, 2016
@liggitt liggitt added this to the v1.5 milestone Sep 7, 2016
@liggitt
Copy link
Member

liggitt commented Sep 7, 2016

LGTM

@liggitt
Copy link
Member

liggitt commented Sep 7, 2016

is this a dupe of #26037?

@anguslees
Copy link
Member

LGTM, thanks!

@anguslees
Copy link
Member

Fixes #25862, fwiw.

@strigazi
Copy link
Contributor

strigazi commented Sep 8, 2016

@liggitt Is it possible to add this in 1.4?

@liggitt
Copy link
Member

liggitt commented Sep 8, 2016

1.4 is limited to blocking bug fixes at this point, so it'll go in for 1.5 as soon as master opens back up

@googlebot
Copy link

CLAs look good, thanks!

@MatMaul
Copy link
Contributor Author

MatMaul commented Sep 16, 2016

Juste rebase on top of master, and the CLA is good now :)

@MatMaul
Copy link
Contributor Author

MatMaul commented Sep 20, 2016

I fixed the licenses however I don't understand the failure regarding the symbols (seems unrelated to my changes), let's see if the rebase helps.

@MatMaul
Copy link
Contributor Author

MatMaul commented Sep 20, 2016

ok so if I includes the test file I get a problem with verify-symbols, and if I don't I get a problem with verify-godeps, help appreciated here :)

@MatMaul
Copy link
Contributor Author

MatMaul commented Sep 20, 2016

I just understood why.
The file is named *_tests.go and not *_test.go...
So I'll send a PR to rackspace/gophercloud before getting back here.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit ce9be8f7eb5d6500133372c8611ebfd4737625a1. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit ce9be8f7eb5d6500133372c8611ebfd4737625a1. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit ce9be8f7eb5d6500133372c8611ebfd4737625a1. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit ce9be8f7eb5d6500133372c8611ebfd4737625a1. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE Node e2e failed for commit ce9be8f7eb5d6500133372c8611ebfd4737625a1. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@MatMaul MatMaul force-pushed the openstack-trustid branch 2 times, most recently from dda70a5 to 21d3422 Compare October 14, 2016 09:36
@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 21d3422d420e41efc6c54970ea83e7f0d4414c9a. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit a360af2b8db760dea98277f4b0b4a440fca8a620. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@MatMaul
Copy link
Contributor Author

MatMaul commented Oct 14, 2016

The fix has been committed upstream, tests are now green ! @liggitt if you can put your LGTM again. Thanks all !

@liggitt liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 14, 2016
@liggitt
Copy link
Member

liggitt commented Oct 14, 2016

LGTM
(you should probably go ahead and get the cncf cla bot happy to avoid future PR delays)

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 35abd47 into kubernetes:master Oct 14, 2016
@MatMaul MatMaul deleted the openstack-trustid branch October 17, 2016 08:19
dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018
Automatic merge from submit-queue

Support trust id as a scope in the OpenStack authentication logic

This patch allows the use of Kubernetes with Keystone trust delegation to avoid passing the user credentials in clear inside the config file : a specific user with delegated rights can be created and used instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants