Conditionally deny access to web console #4046

miminar · 2015-08-07T14:29:53Z

If the web console component is disabled through masterConfig.DisabledFeatures, deny all the access to:

$MASTER_URL/console
$MASTER_URL/console/*

And prevent http server for static assets from running.

Signed-off-by: Michal Minar miminar@redhat.com

liggitt · 2015-08-07T14:32:56Z

Auth is needed for OAuth and CLI login. Only disable the asset bits

liggitt · 2015-08-07T14:40:53Z

pkg/cmd/server/origin/asset.go

@@ -22,6 +22,10 @@ import (
 // then returns an array of strings indicating what endpoints were started
 // (these are format strings that will expect to be sent a single string value).
 func (c *AssetConfig) InstallAPI(container *restful.Container) []string {
+	if c.WebConsoleDisabled {


doesn't belong here... just skip building the AssetConfig entirely if web console is disabled

miminar · 2015-08-07T15:03:15Z

Thanks @liggitt, rework has just started.

miminar · 2015-08-07T16:36:14Z

Haven't touched the authentication part yet. Need to investigate a bit further. CLI login works with Web Console disabled (tested with default AllowAllPasswordIdentityProvider and HTPasswdPasswordIdentityProvider). @ashcrow any ideas?

TODO: write some tests.

Update: Access to /console gives me following with disabled Web Console:

{
    "kind": "Status",
    "apiVersion": "v1",
    "metadata": {},
    "status": "Failure",
    "message": "User \"system:anonymous\" cannot \"get\" on \"/console\"",
    "reason": "Forbidden",
    "details": {},
    "code": 403
}

Looks unfriendly to me.

liggitt · 2015-08-07T16:38:26Z

cli uses the challengers["login"] portion, but other, non-web-console clients could use the browser login

liggitt · 2015-08-07T16:38:48Z

e.g. https://master.example.com:8443/oauth/token/request, which is needed in order to get a token for the CLI if an identity provider that doesn't support challenge headers is configured

liggitt · 2015-08-07T16:46:35Z

pkg/cmd/server/origin/auth_config.go

@@ -72,6 +73,7 @@ func BuildAuthConfig(options configapi.MasterConfig) (*AuthConfig, error) {
 	ret := &AuthConfig{
 		Options: *options.OAuthConfig,

+		WebConsoleDisabled:   options.DisabledFeatures.Has(configapi.FeatureWebConsole),


remove this... the only difference the web console being disabled should make to auth is that assetPublicURLs be empty

If the web console component is disabled through `FeatureConfig`, deny all the access to it. Signed-off-by: Michal Minar <miminar@redhat.com>

miminar · 2015-08-10T14:52:01Z

Rebased, reverted auth part and added simple tests. @liggitt, is it any better now?

liggitt · 2015-08-10T19:34:20Z

hack/test-cmd.sh

@@ -253,6 +241,10 @@ oc get services
 mv ${HOME}/.kube/config ${HOME}/.kube/non-default-config
 echo "config files: ok"

+# Test access to /console/
+$(which curl) -sfL --max-time 5 "${API_SCHEME}://${API_HOST}:${API_PORT}/console/" | grep '<title>'


won't this fail with cert errors?

CURL_* variables are set above.

liggitt · 2015-08-10T19:48:32Z

looks a lot cleaner, a good integration test and we should be ok

liggitt · 2015-08-10T19:48:49Z

update the PR title and description with the updated content

miminar · 2015-08-11T13:31:08Z

@liggitt Added integration test. I'm concerned about /login endpoint though. I get standard login page in browser and after successful login, empty page is received.

liggitt · 2015-08-11T13:33:34Z

test/integration/console_access.go

+	}
+	if resp.Header.Get("Location") != masterConfig.AssetConfig.PublicURL {
+		t.Errorf("Expected %s, got %s", masterConfig.AssetConfig.PublicURL, resp.Header.Get("Location"))
+	}


should also GET the asset public URL to make sure the UI is served

liggitt · 2015-08-11T13:46:10Z

pkg/cmd/server/origin/master_config.go

@@ -428,6 +428,10 @@ func (c *MasterConfig) RouteAllocatorClients() (*osclient.Client, *kclient.Clien
 	return c.PrivilegedLoopbackOpenShiftClient, c.PrivilegedLoopbackKubernetesClient
 }

+func (c *MasterConfig) WebConsoleEnabled() bool {


godoc, since the two conditions aren't obvious from the name of the function

Signed-off-by: Michal Minar <miminar@redhat.com>

miminar · 2015-08-11T15:47:14Z

@liggitt could you take one more look?

Signed-off-by: Michal Minar <miminar@redhat.com>

liggitt · 2015-08-11T16:41:51Z

test/integration/web_console_access_test.go

+	if resp.StatusCode != expectedStatus {
+		t.Errorf("Expected status %d for %s, got %d", expectedStatus, url, resp.StatusCode)
+	} else {
+		if expectedRedirectLocation != "" {


this shouldn't be in an else

actually, the check shouldn't even be conditional... if we expect "", and get "foo", that's an error

Signed-off-by: Michal Minar <miminar@redhat.com>

liggitt · 2015-08-12T13:59:05Z

test/integration/web_console_access_test.go

+	}{
+		"":             {http.StatusFound, masterOptions.AssetConfig.PublicURL},
+		"healthz":      {http.StatusOK, ""},
+		"login":        {http.StatusOK, ""},


add "oauth/token/request" expecting a redirect to "oauth/authorize?..." to make sure OAuth is still hooked up

Added, good suggestions.

liggitt · 2015-08-12T13:59:29Z

a couple last checks for OAuth, then LGTM

Signed-off-by: Michal Minar <miminar@redhat.com>

miminar · 2015-08-12T14:45:42Z

@liggitt another ping :)

liggitt · 2015-08-12T14:46:24Z

LGTM, [merge]

openshift-bot · 2015-08-12T14:49:01Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/2940/) (Image: devenv-fedora_2147)

openshift-bot · 2015-08-12T14:49:01Z

Evaluated for origin merge up to 70af553

openshift-bot · 2015-08-12T14:49:02Z

[Test]ing while waiting on the merge queue

miminar · 2015-08-12T14:49:46Z

@liggitt Thanks!

openshift-bot · 2015-08-12T14:56:58Z

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

openshift-bot · 2015-08-12T14:56:59Z

Evaluated for origin test up to 70af553

Merged by openshift-bot

stevekuznetsov · 2015-08-19T18:13:26Z

@miminar @liggitt This test may not have ever been run, incorrect build tags: !no-etcd is etcd, no-etcd is !etcd.

miminar · 2015-08-20T09:46:01Z

@stevekuznetsov actually the tests run just fine. The problem is they can't be disabled :-). Adding no-etcd to tags doesn't help. There's another integration test file having this tag. ~~I'll remove it.~~

Update: Looks like you've already taken care of it :-)

stevekuznetsov · 2015-08-20T14:32:50Z

@miminar Yes, interestingly enough the logic for running tests did run yours. All is well.

miminar mentioned this pull request Aug 7, 2015

Deny access to Web Console miminar/origin#2

Closed

liggitt reviewed Aug 7, 2015
View reviewed changes

liggitt self-assigned this Aug 7, 2015

liggitt reviewed Aug 7, 2015
View reviewed changes

miminar force-pushed the disable-web-console branch from 37b39ab to 05936e2 Compare August 10, 2015 14:47

Deny access to Web Console

c8ca2fd

If the web console component is disabled through `FeatureConfig`, deny all the access to it. Signed-off-by: Michal Minar <miminar@redhat.com>

miminar force-pushed the disable-web-console branch from 05936e2 to c8ca2fd Compare August 10, 2015 14:50

liggitt reviewed Aug 10, 2015
View reviewed changes

miminar changed the title ~~Deny access to Web Console~~ Conditionally deny access to web console Aug 11, 2015

liggitt reviewed Aug 11, 2015
View reviewed changes

miminar force-pushed the disable-web-console branch from 09917a9 to 465193b Compare August 11, 2015 13:43

liggitt reviewed Aug 11, 2015
View reviewed changes

Michal Minar added 2 commits August 11, 2015 17:39

Test improvements

bbce458

Signed-off-by: Michal Minar <miminar@redhat.com>

Godoced exported method

82692ae

Signed-off-by: Michal Minar <miminar@redhat.com>

Inline test cases

00894c5

Signed-off-by: Michal Minar <miminar@redhat.com>

liggitt reviewed Aug 11, 2015
View reviewed changes

More polishment for integration test

86a2523

Signed-off-by: Michal Minar <miminar@redhat.com>

liggitt reviewed Aug 12, 2015
View reviewed changes

Extended tested endpoints

70af553

Signed-off-by: Michal Minar <miminar@redhat.com>

openshift-bot pushed a commit that referenced this pull request Aug 12, 2015

Merge pull request #4046 from miminar/disable-web-console

0b8fbcb

Merged by openshift-bot

openshift-bot merged commit 0b8fbcb into openshift:master Aug 12, 2015

miminar deleted the disable-web-console branch January 24, 2016 18:46

vanloswang mentioned this pull request Sep 28, 2016

disable feature does not work? #11133

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conditionally deny access to web console #4046

Conditionally deny access to web console #4046

miminar commented Aug 7, 2015

liggitt commented Aug 7, 2015

liggitt Aug 7, 2015

miminar commented Aug 7, 2015

miminar commented Aug 7, 2015

liggitt commented Aug 7, 2015

liggitt commented Aug 7, 2015

liggitt Aug 7, 2015

miminar commented Aug 10, 2015

liggitt Aug 10, 2015

miminar Aug 11, 2015

liggitt commented Aug 10, 2015

liggitt commented Aug 10, 2015

miminar commented Aug 11, 2015

liggitt Aug 11, 2015

liggitt Aug 11, 2015

miminar commented Aug 11, 2015

liggitt Aug 11, 2015

liggitt Aug 11, 2015

miminar Aug 12, 2015

liggitt Aug 12, 2015

miminar Aug 12, 2015

liggitt commented Aug 12, 2015

miminar commented Aug 12, 2015

liggitt commented Aug 12, 2015

openshift-bot commented Aug 12, 2015

openshift-bot commented Aug 12, 2015

openshift-bot commented Aug 12, 2015

miminar commented Aug 12, 2015

openshift-bot commented Aug 12, 2015

openshift-bot commented Aug 12, 2015

stevekuznetsov commented Aug 19, 2015

miminar commented Aug 20, 2015

stevekuznetsov commented Aug 20, 2015

Conditionally deny access to web console #4046

Conditionally deny access to web console #4046

Conversation

miminar commented Aug 7, 2015

liggitt commented Aug 7, 2015

Choose a reason for hiding this comment

miminar commented Aug 7, 2015

miminar commented Aug 7, 2015

liggitt commented Aug 7, 2015

liggitt commented Aug 7, 2015

Choose a reason for hiding this comment

miminar commented Aug 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Aug 10, 2015

liggitt commented Aug 10, 2015

miminar commented Aug 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miminar commented Aug 11, 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

liggitt commented Aug 12, 2015

miminar commented Aug 12, 2015

liggitt commented Aug 12, 2015

openshift-bot commented Aug 12, 2015

openshift-bot commented Aug 12, 2015

openshift-bot commented Aug 12, 2015

miminar commented Aug 12, 2015

openshift-bot commented Aug 12, 2015

openshift-bot commented Aug 12, 2015

stevekuznetsov commented Aug 19, 2015

miminar commented Aug 20, 2015

stevekuznetsov commented Aug 20, 2015