Skip to content

allow SA oauth clients to list projects#9977

Merged
openshift-bot merged 1 commit intoopenshift:masterfrom
deads2k:demo-hack-web
Aug 16, 2016
Merged

allow SA oauth clients to list projects#9977
openshift-bot merged 1 commit intoopenshift:masterfrom
deads2k:demo-hack-web

Conversation

@deads2k
Copy link
Copy Markdown
Contributor

@deads2k deads2k commented Jul 21, 2016

Allows an SA being used as an oauth client to list the available projects for this user. This also adds user:list-all-projects so that we have one scope for "list projects this token can see based on other permissions" and another for "list all projects this user can see regard of the other permissions on this token"

@liggitt @smarterclayton I'm unsure about this, but in my quest to run a personal webconsole I found it was the first request. Running a personal version of a normally cross namespace thing doesn't seem immediately unreasonable.

@deads2k
Copy link
Copy Markdown
Contributor Author

deads2k commented Jul 21, 2016

[test]

@liggitt
Copy link
Copy Markdown
Contributor

liggitt commented Jul 21, 2016

Not sure I see the point if the sa won't be able to request permissions or do things in any of those projects

@deads2k
Copy link
Copy Markdown
Contributor Author

deads2k commented Jul 21, 2016

Not sure I see the point if the sa won't be able to request permissions or do things in any of those projects

I think I want a "which projects can this token see". I also want a "show me all projects" endpoint.

@smarterclayton
Copy link
Copy Markdown
Contributor

Would the scope restrict you so it's possible not to let the SA
token see projects? Project names can arguably be a target of attack
(Davids-skynet-bootstrap-project might be someone you don't want us
knowing about).

@liggitt
Copy link
Copy Markdown
Contributor

liggitt commented Jul 22, 2016

That list of scopes is the most a SA could request, not the minimum.

@deads2k
Copy link
Copy Markdown
Contributor Author

deads2k commented Jul 22, 2016

Would the scope restrict you so it's possible not to let the SA
token see projects? Project names can arguably be a target of attack
(Davids-skynet-bootstrap-project might be someone you don't want us
knowing about).

Shhhh, no one else knows!

That's the current state. I think a "what scopes can this token see" endpoint (its not really /projects) for SA tokens might make integrations easier. Otherwise, you're left trying to describe it via downward API env vars.

@deads2k
Copy link
Copy Markdown
Contributor Author

deads2k commented Jul 26, 2016

The watch is the really nasty part of the filter. I think that a list on projects could be filtered by checking scopes like this:

  1. no scopes: return it all
  2. scopes unrestricted by namespace: if they have get namespace, return the whole list. If they don't, ignore them.
  3. scopes restricted by namespace: decimate to those namespaces, then re-check just those namespaces.

Given detailed knowledge of scopes and a watch on clusterpolicy and policy, it could be done. Is that a thing we want to do? It seems pretty likely that people will want to do this.

@smarterclayton
Copy link
Copy Markdown
Contributor

I don't care enough right now but we should put some weasel words in so
later we can take it away.

On Tue, Jul 26, 2016 at 8:58 AM, David Eads notifications@github.com
wrote:

The watch is the really nasty part of the filter. I think that a list on
projects could be filtered by checking scopes like this:

  1. no scopes: return it all
  2. scopes unrestricted by namespace: if they have get namespace,
    return the whole list. If they don't, ignore them.
  3. scopes restricted by namespace: decimate to those namespaces, then
    re-check just those namespaces.

Given detailed knowledge of scopes and a watch on clusterpolicy and
policy, it could be done. Is that a thing we want to do? It seems pretty
likely that people will want to do this.


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

@sosiouxme
Copy link
Copy Markdown
Member

Kibana needs to show you a list of projects you have access to see the logs for. Our Elasticsearch plugin also constructs its ACLs based on the list of projects (we could theoretically update the ACLs each time a new project is requested, but this would impact user experience as writing the ACL takes a notable amount of time).

With a SA client that gave out tokens with the ability to list projects, we could dispense with the whole deployer circus around creating a specialized oauthclient for this purpose.

@smarterclayton
Copy link
Copy Markdown
Contributor

If elastic search needs the list of projects you have access to, it should
explicitly request access to a scope that gives you that. Not having ES
request the scope is exactly the problem I'm worried about in the long
term. I don't think there's disagreement that it should be possible to
request that scope.

On Wed, Aug 3, 2016 at 10:27 AM, Luke Meyer notifications@github.com
wrote:

Kibana needs to show you a list of projects you have access to see the
logs for. Our Elasticsearch plugin also constructs its ACLs based on the
list of projects (we could theoretically update the ACLs each time a new
project is requested, but this would impact user experience as writing the
ACL takes a notable amount of time).

With a SA client that gave out tokens with the ability to list projects,
we could dispense with the whole deployer circus around creating a
specialized oauthclient for this purpose.


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

@deads2k
Copy link
Copy Markdown
Contributor Author

deads2k commented Aug 3, 2016

If elastic search needs the list of projects you have access to, it should
explicitly request access to a scope that gives you that. Not having ES
request the scope is exactly the problem I'm worried about in the long
term. I don't think there's disagreement that it should be possible to
request that scope.

It does have to explicitly request it, we're just denying them right now. This pull would allow @sosiouxme to request it. If you want it, just have to merge this guy.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2016
@smarterclayton
Copy link
Copy Markdown
Contributor

I'm ok with this - eventually this list will be expanded anyway.

// UserListProject gives explicit permission to see the projects a user can see. This is often used to prime secondary ACL systems
// unrelated to openshift and to display projects for selection in a secondary UI.
UserListProject = UserIndicator + "list-projects"
UserListAllProjects = UserIndicator + "list-all-projects"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@liggitt agree on the name

Copy link
Copy Markdown
Contributor

@liggitt liggitt Aug 12, 2016

Choose a reason for hiding this comment

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

I think most people would expect user:list-projects to let you list all the user's projects.

The "list projects my other scopes allow getting" scope is the harder one to understand (and probably less likely to be requested), so I think it has some explaining to do with its name. user:list-scoped-projects? user:list-gettable-projects?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The "list projects my other scopes allow getting" scope is the harder one to understand (and probably less likely to be requested), so I think it has some explaining to do with its name. user:list-scoped-projects? user:list-gettable-projects?

I'm fine with that. Fun as list-gettable would be, I'll probably go with list-scoped-projects.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think most people would expect user:list-projects to let you list all the user's projects.

I'll also point out that most people will probably be using for cases where they think their integration is "trusted", so getting back a project list where you don't have access to the projects sucks. Imagine the web-console as a for instance.

Still updating the names though.

@deads2k
Copy link
Copy Markdown
Contributor Author

deads2k commented Aug 12, 2016

@mfojtik can you take a look at the plumbing?

@deads2k
Copy link
Copy Markdown
Contributor Author

deads2k commented Aug 12, 2016

@ewolinetz You may need to update to request user:list-all-projects. See the comment here: #10252 (comment)

@deads2k
Copy link
Copy Markdown
Contributor Author

deads2k commented Aug 12, 2016

@stevekuznetsov unknown region: https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_conformance/5050/console

re[test]

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2016
@stevekuznetsov
Copy link
Copy Markdown
Contributor

@deads2k in case you're interested ... the flake that happened there is:

  • something caused us to grab an incorrect version of the Origin codebase, or your patch, or both
  • your patch didn't apply cleanly to master/HEAD so the script failed in the first steps
  • no AWS instance got launched
  • the cleanup step that assumes an instance is running failed because one wasn't

Unfortunately I don't think there's anything we can do about it.

@deads2k
Copy link
Copy Markdown
Contributor Author

deads2k commented Aug 12, 2016

something caused us to grab an incorrect version of the Origin codebase, or your patch, or both

Retry until you pull it properly?

@stevekuznetsov
Copy link
Copy Markdown
Contributor

@deads2k there's no real way of knowing that we have done it "properly" -- there are a number of scenarios where the "proper" code results in a merge conflict as well.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 15, 2016
UserAccessCheck = UserIndicator + "check-access"

// UserListProject gives explicit permission to see the projects that this token can see.
UserListProject = UserIndicator + "list-scoped-projects"
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.

make the const names match?

@openshift-bot
Copy link
Copy Markdown
Contributor

Evaluated for origin test up to 067fa42

@openshift-bot
Copy link
Copy Markdown
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7972/)

@deads2k
Copy link
Copy Markdown
Contributor Author

deads2k commented Aug 16, 2016

[merge]

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Aug 16, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7972/) (Image: devenv-rhel7_4841)

@openshift-bot
Copy link
Copy Markdown
Contributor

Evaluated for origin merge up to 067fa42

@openshift-bot openshift-bot merged commit 7596214 into openshift:master Aug 16, 2016
@deads2k deads2k deleted the demo-hack-web branch September 6, 2016 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants