add context namespacing filter (Bug 1196022) #1140

deads2k · 2015-02-25T13:57:48Z

namespaces not set in context. Do not merge this without additional integration tests to avoid this problem in the future.

Integration tests will be much easier with #1128. I'm going to try that route first.

deads2k · 2015-02-26T16:52:31Z

@liggitt review? It came out a little bigger than I wanted, but I think it's still reviewable.

liggitt · 2015-02-26T16:56:21Z

test/integration/authorization_test.go

+		t.Errorf("unexpected error: %v", err)
+	}
+
+	removeInsecureOptions := &policy.RemoveGroupOptions{


add a TODO to remove once policy gets tightened

liggitt · 2015-02-26T17:26:56Z

test/integration/test_server.go

+	}()
+
+	// wait for the server to come up
+	if err := cmdutil.WaitForSuccessfulDial(true, "tcp", masterAddr, 100*time.Millisecond, 100*time.Millisecond, 100); err != nil {


is 20 seconds long enough to wait?

is 20 seconds long enough to wait?

Takes 4 on my machine.

liggitt · 2015-02-26T18:54:35Z

pkg/cmd/server/origin/master.go

+			// APIRequestInfoResolver without any modification or customization.
+			namespace := requestInfo.Namespace
+			if (requestInfo.Resource == "projects") && (len(requestInfo.Name) > 0) {
+				namespace = requestInfo.Name


if requestInfo has a namespace, and we get in here, is that an error?

Errors aren't caught on other weird conditions with non-namespaced resources having a namespace, so I'm going to with no.

yeah, that's concerning, actually... means you can do GET /namespaces/foo/oauthaccesstokens, get through authz if you have permission within namespace foo, then the rest layer ignores the namespace. not a problem to fix here, though

liggitt · 2015-02-26T19:30:45Z

test/integration/test_server.go

+
+// CreateNewProject creates a new project using the clusterAdminClient, then gets a token for the adminUser and returns
+// back a client for the admin user
+func CreateNewProject(clusterAdminClient *client.Client, clientConfig kclient.Config, projectName, adminUser string) (*client.Client, error) {


would it be worthwhile to return a kclient as well, so we can check kubernetes resources inside the project?

liggitt · 2015-02-26T19:33:47Z

LGTM

deads2k · 2015-02-26T19:43:44Z

[merge]

openshift-bot · 2015-02-26T20:01:56Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1052/) (Image: devenv-fedora_909)

openshift-bot · 2015-02-26T20:01:56Z

Evaluated for origin up to 6103934

Merged by openshift-bot

smarterclayton · 2015-03-14T19:15:15Z

Um... why are we doing this with namespacing filter? Namespace is set by API calls, not by arbitrary filters.

deads2k · 2015-03-16T11:55:26Z

Um... why are we doing this with namespacing filter? Namespace is set by API calls, not by arbitrary filters

After removing namespace from authorization.Attributes and instead using the value set on context, we needed to make sure the value was set on context. Since the API calls don't get invoked until after the call is authorized, we have to retrieve the namespace earlier than that. Previously, we were simply setting it on a private object, but after we started using the context for namespace values it seemed to make sense to set it in a filter placed before authorization.

smarterclayton · 2015-03-18T21:13:23Z

Can you open an issue upstream to track "check API login in API, and ensure non API calls go through the non-api authorization path"?

…service-catalog/' changes from 7e650e7e39..ef63307bdb ef63307bdb origin build: add origin tooling a876fe3 v0.0.17 (openshift#1178) c5237fe correct osbapi service definition (openshift#1177) 6036d4e Adding walkthrough instructions for 1.7 (openshift#1171) 5f111dd Specifying that you need Helm v2.5.0 for installation (openshift#1170) 08043bd Adding more small fixes to the walkthrough & install docs (openshift#1169) d65d4a1 rbac targets needed to be renamed as well (openshift#1161) 590f6f2 Write helm command to file for api aggregation (openshift#1141) 49ddcf6 clean before building a specific arch (openshift#1168) 43f7cfb Splitting up the Walkthrough for 1.6 and 1.7 instructions (openshift#1163) 02e0217 Updates to README (openshift#1166) 57f2aa5 Adding instructions for installing from Macs (openshift#1164) dfe620e fix rate-limiting for polling queue (openshift#1143) ca5f335 Use Generation instead of checksum for Broker (openshift#1145) 5364daa Merge branch 'pr/1158' f34c5db move Travis deployment script to directory in 'contrib/' 2a00d7f Update incorrect port (openshift#1156) b0ed60e improve the repository's layout (openshift#1154) f870baf Follow up file / renames from openshift#1142 (openshift#1152) 826b4f9 remove unnecessary json annotations (openshift#1153) 33cb345 Rename resources. closes openshift#1080 (openshift#1142) 70c2b9b Add ability to specify CA certs to use for TLS authentication. (openshift#1112) 2aa5039 v0.0.16 (openshift#1140) 65de49c Comments for unit test bullet proofing (openshift#1139) REVERT: 7e650e7e39 origin build: add origin tooling git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog git-subtree-split: ef63307bdbaa64efca204912f5361a4f3d3be2c8

Signed-off-by: Doug Davis <dug@us.ibm.com>

[3.9] UPSTREAM: 61284: Fix creation of subpath with SUID/SGID directories.

deads2k mentioned this pull request Feb 25, 2015

tighten authorization rules #1074

Merged

deads2k force-pushed the deads-fix-namespacing branch 2 times, most recently from 4285b37 to 5bd8b62 Compare February 26, 2015 16:49

deads2k changed the title ~~[WIP] add context namespacing filter (Bug 1196022)~~ add context namespacing filter (Bug 1196022) Feb 26, 2015

liggitt reviewed Feb 26, 2015
View reviewed changes

deads2k force-pushed the deads-fix-namespacing branch from 5bd8b62 to f140555 Compare February 26, 2015 16:59

liggitt reviewed Feb 26, 2015
View reviewed changes

deads2k force-pushed the deads-fix-namespacing branch from f4fd174 to 7af4266 Compare February 26, 2015 18:12

deads2k mentioned this pull request Feb 26, 2015

Allow project admins to use resourceaccessreview #1166

Merged

liggitt reviewed Feb 26, 2015
View reviewed changes

add context namespacing filter

6103934

liggitt reviewed Feb 26, 2015
View reviewed changes

deads2k force-pushed the deads-fix-namespacing branch from 7d2ad0c to 6103934 Compare February 26, 2015 19:34

openshift-bot pushed a commit that referenced this pull request Feb 26, 2015

Merge pull request #1140 from deads2k/deads-fix-namespacing

a518ff7

Merged by openshift-bot

openshift-bot merged commit a518ff7 into openshift:master Feb 26, 2015

deads2k deleted the deads-fix-namespacing branch February 26, 2015 20:25

jpeeler pushed a commit to jpeeler/origin that referenced this pull request Feb 1, 2018

v0.0.16 (openshift#1140)

2aa5039

Signed-off-by: Doug Davis <dug@us.ibm.com>

Miciah pushed a commit to Miciah/origin that referenced this pull request Jun 27, 2018

Merge pull request openshift#1140 from rootfs/pr-61284-3.8

d35a138

[3.9] UPSTREAM: 61284: Fix creation of subpath with SUID/SGID directories.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add context namespacing filter (Bug 1196022) #1140

add context namespacing filter (Bug 1196022) #1140

deads2k commented Feb 25, 2015

deads2k commented Feb 26, 2015

liggitt Feb 26, 2015

deads2k Feb 26, 2015

liggitt Feb 26, 2015

deads2k Feb 26, 2015

deads2k Feb 26, 2015

liggitt Feb 26, 2015

deads2k Feb 26, 2015

liggitt Feb 26, 2015

liggitt Feb 26, 2015

liggitt commented Feb 26, 2015

deads2k commented Feb 26, 2015

openshift-bot commented Feb 26, 2015

openshift-bot commented Feb 26, 2015

smarterclayton commented Mar 14, 2015

deads2k commented Mar 16, 2015

smarterclayton commented Mar 18, 2015

add context namespacing filter (Bug 1196022) #1140

add context namespacing filter (Bug 1196022) #1140

Conversation

deads2k commented Feb 25, 2015

deads2k commented Feb 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Feb 26, 2015

deads2k commented Feb 26, 2015

openshift-bot commented Feb 26, 2015

openshift-bot commented Feb 26, 2015

smarterclayton commented Mar 14, 2015

deads2k commented Mar 16, 2015

smarterclayton commented Mar 18, 2015