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.

Sign up for GitHub

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

Jump to bottom

return forbidden api status object #1452

Merged
merged 2 commits into from
Mar 25, 2015
Merged

return forbidden api status object #1452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
56f0a4c
tidy up MasterConfig getter location
deads2k Mar 25, 2015
c76aaba
return a forbidden api status object
deads2k Mar 25, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 25 additions & 73 deletions pkg/cmd/server/origin/master.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
package origin

import (
"bytes"
"crypto/tls"
"encoding/json"
"errors"
"fmt"
"net"
"net/http"
Expand All @@ -19,6 +22,7 @@ import (
"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api"
kapierror "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
klatest "github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest"
"github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver"
kclient "github.com/GoogleCloudPlatform/kubernetes/pkg/client"
kmaster "github.com/GoogleCloudPlatform/kubernetes/pkg/master"
Expand Down Expand Up @@ -114,78 +118,6 @@ func (fn APIInstallFunc) InstallAPI(container *restful.Container) []string {
return fn(container)
}

// KubeClient returns the kubernetes client object
func (c *MasterConfig) KubeClient() *kclient.Client {
return c.KubernetesClient
}

// PolicyClient returns the policy client object
// It must have the following capabilities:
// list, watch all policyBindings in all namespaces
// list, watch all policies in all namespaces
// create resourceAccessReviews in all namespaces
func (c *MasterConfig) PolicyClient() *osclient.Client {
return c.OSClient
}

// DeploymentClient returns the deployment client object
func (c *MasterConfig) DeploymentClient() *kclient.Client {
return c.KubernetesClient
}

// DNSServerClient returns the DNS server client object
// It must have the following capabilities:
// list, watch all services in all namespaces
func (c *MasterConfig) DNSServerClient() *kclient.Client {
return c.KubernetesClient
}

// BuildLogClient returns the build log client object
func (c *MasterConfig) BuildLogClient() *kclient.Client {
return c.KubernetesClient
}

// WebHookClient returns the webhook client object
func (c *MasterConfig) WebHookClient() *osclient.Client {
return c.OSClient
}

// BuildControllerClients returns the build controller client objects
func (c *MasterConfig) BuildControllerClients() (*osclient.Client, *kclient.Client) {
return c.OSClient, c.KubernetesClient
}

// ImageChangeControllerClient returns the openshift client object
func (c *MasterConfig) ImageChangeControllerClient() *osclient.Client {
return c.OSClient
}

// ImageImportControllerClient returns the deployment client object
func (c *MasterConfig) ImageImportControllerClient() *osclient.Client {
return c.OSClient
}

// DeploymentControllerClients returns the deployment controller client object
func (c *MasterConfig) DeploymentControllerClients() (*osclient.Client, *kclient.Client) {
return c.OSClient, c.KubernetesClient
}

// DeployerClientConfig returns the client configuration a Deployer instance launched in a pod
// should use when making API calls.
func (c *MasterConfig) DeployerClientConfig() *kclient.Config {
return &c.DeployerOSClientConfig
}

func (c *MasterConfig) DeploymentConfigControllerClients() (*osclient.Client, *kclient.Client) {
return c.OSClient, c.KubernetesClient
}
func (c *MasterConfig) DeploymentConfigChangeControllerClients() (*osclient.Client, *kclient.Client) {
return c.OSClient, c.KubernetesClient
}
func (c *MasterConfig) DeploymentImageChangeControllerClient() *osclient.Client {
return c.OSClient
}

func (c *MasterConfig) InstallProtectedAPI(container *restful.Container) []string {
defaultRegistry := env("OPENSHIFT_DEFAULT_REGISTRY", "${DOCKER_REGISTRY_SERVICE_HOST}:${DOCKER_REGISTRY_SERVICE_PORT}")
svcCache := service.NewServiceResolverCache(c.KubeClient().Services(api.NamespaceDefault).Get)
Expand Down Expand Up @@ -501,8 +433,28 @@ func (c *MasterConfig) authorizationFilter(handler http.Handler) http.Handler {

// forbidden renders a simple forbidden error
func forbidden(reason string, w http.ResponseWriter, req *http.Request) {
// Reason is an opaque string that describes why access is allowed or forbidden (forbidden by the time we reach here).
// We don't have direct access to kind or name (not that those apply either in the general case)
// We create a NewForbidden to stay close the API, but then we override the message to get a serialization
// that makes sense when a human reads it.
forbiddenError, _ := kapierror.NewForbidden("", "", errors.New("")).(*kapierror.StatusError)
forbiddenError.ErrStatus.Message = fmt.Sprintf("%q is forbidden because %s", req.RequestURI, reason)
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than jumping through hoops to use NewForbidden, can we just do

&kapierror.StatusError{
  kapi.Status{
    Status: kapi.StatusFailure,
    Code:   http.StatusForbidden,
    Reason: kapi.StatusReasonForbidden,
    Message: fmt.Sprintf("%q is forbidden because %s", req.RequestURI, reason),
  },
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'd rather stay with the library function and rely on it to set every field except for the message. That keeps us in sync if make changes. It will also alert a rebaser if we end up fixing it upstream.

Copy link
Contributor

Choose a reason for hiding this comment

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

is it more correct to have a nil Details field or a Details field with empty name and kind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd read it as, "a reason forbidden indicates that the StatusDetails MUST be present, and MAY have 'kind' set and MAY have 'id' set". The other reading wouldn't make much sense because it would say, "a reason forbidden indicates that the StatusDetails MUST be present, and MAY have a details stanza. If there is a details stanza, it MUST have both 'kind' and 'id'". That doesn't make much sense since id's aren't always available.

Clearly we should demand RFC style godoc for clarity!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, you could see an argument for including Verb and Namespace in the details. We want to set all those fields in a structured way someday.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok


// TODO: this serialization is broken in the general case. It chooses the latest, but
// it should choose the serialization based on the codec for the uri requested. What is coded here
// is better than not returning a status object at all, but needs to be fixed once authorization is
// tied into the APIInstaller itself. Then we'll only have the wrong codec for non-resource requests
formatted := &bytes.Buffer{}
output, err := klatest.Codec.Encode(&forbiddenError.ErrStatus)
if err != nil {
fmt.Fprintf(formatted, "%s", forbiddenError.Error())
} else {
_ = json.Indent(formatted, output, "", " ")
}

w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusForbidden)
fmt.Fprintf(w, "Forbidden: %q %s", req.RequestURI, reason)
w.Write(formatted.Bytes())
}

// RunProjectAuthorizationCache starts the project authorization cache
Expand Down
72 changes: 72 additions & 0 deletions pkg/cmd/server/origin/master_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,3 +223,75 @@ func getEtcdTokenAuthenticator(etcdHelper tools.EtcdHelper) authenticator.Token
oauthRegistry := oauthetcd.New(etcdHelper)
return authnregistry.NewTokenAuthenticator(oauthRegistry)
}

// KubeClient returns the kubernetes client object
func (c *MasterConfig) KubeClient() *kclient.Client {
return c.KubernetesClient
}

// PolicyClient returns the policy client object
// It must have the following capabilities:
// list, watch all policyBindings in all namespaces
// list, watch all policies in all namespaces
// create resourceAccessReviews in all namespaces
func (c *MasterConfig) PolicyClient() *osclient.Client {
return c.OSClient
}

// DeploymentClient returns the deployment client object
func (c *MasterConfig) DeploymentClient() *kclient.Client {
return c.KubernetesClient
}

// DNSServerClient returns the DNS server client object
// It must have the following capabilities:
// list, watch all services in all namespaces
func (c *MasterConfig) DNSServerClient() *kclient.Client {
return c.KubernetesClient
}

// BuildLogClient returns the build log client object
func (c *MasterConfig) BuildLogClient() *kclient.Client {
return c.KubernetesClient
}

// WebHookClient returns the webhook client object
func (c *MasterConfig) WebHookClient() *osclient.Client {
return c.OSClient
}

// BuildControllerClients returns the build controller client objects
func (c *MasterConfig) BuildControllerClients() (*osclient.Client, *kclient.Client) {
return c.OSClient, c.KubernetesClient
}

// ImageChangeControllerClient returns the openshift client object
func (c *MasterConfig) ImageChangeControllerClient() *osclient.Client {
return c.OSClient
}

// ImageImportControllerClient returns the deployment client object
func (c *MasterConfig) ImageImportControllerClient() *osclient.Client {
return c.OSClient
}

// DeploymentControllerClients returns the deployment controller client object
func (c *MasterConfig) DeploymentControllerClients() (*osclient.Client, *kclient.Client) {
return c.OSClient, c.KubernetesClient
}

// DeployerClientConfig returns the client configuration a Deployer instance launched in a pod
// should use when making API calls.
func (c *MasterConfig) DeployerClientConfig() *kclient.Config {
return &c.DeployerOSClientConfig
}

func (c *MasterConfig) DeploymentConfigControllerClients() (*osclient.Client, *kclient.Client) {
return c.OSClient, c.KubernetesClient
}
func (c *MasterConfig) DeploymentConfigChangeControllerClients() (*osclient.Client, *kclient.Client) {
return c.OSClient, c.KubernetesClient
}
func (c *MasterConfig) DeploymentImageChangeControllerClient() *osclient.Client {
return c.OSClient
}
12 changes: 5 additions & 7 deletions test/integration/authorization_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,8 @@ func TestRestrictedAccessForProjectAdmins(t *testing.T) {
t.Errorf("unexpected error: %v", err)
}

// TODO make kube and origin authorization failures cause a kapierror.Forbidden
_, err = markClient.Deployments("hammer-project").List(labels.Everything(), fields.Everything())
if (err == nil) || (!strings.Contains(err.Error(), "Forbidden")) {
if (err == nil) || !kapierror.IsForbidden(err) {
t.Errorf("expected forbidden error, but didn't get one")
}

Expand All @@ -61,9 +60,8 @@ func TestRestrictedAccessForProjectAdmins(t *testing.T) {
if err != nil {
t.Errorf("unexpected error: %v", err)
}
// TODO make kube and origin authorization failures cause a kapierror.Forbidden
_, err = markClient.Projects().Get("hammer-project")
if (err == nil) || (!strings.Contains(err.Error(), "Forbidden")) {
if (err == nil) || !kapierror.IsForbidden(err) {
t.Errorf("expected forbidden error, but didn't get one")
}

Expand Down Expand Up @@ -245,7 +243,7 @@ func TestResourceAccessReview(t *testing.T) {
test := resourceAccessReviewTest{
clientInterface: markClient.RootResourceAccessReviews(),
review: requestWhoCanViewDeployments,
err: "Forbidden",
err: "forbidden",
}
test.run(t)
}
Expand Down Expand Up @@ -372,7 +370,7 @@ func TestSubjectAccessReview(t *testing.T) {
subjectAccessReviewTest{
clientInterface: haroldClient.SubjectAccessReviews("mallet-project"),
review: askCanEdgarDeletePods,
err: "Forbidden",
err: "forbidden",
}.run(t)

askCanHaroldUpdateProject := &authorizationapi.SubjectAccessReview{User: "anypassword:harold", Verb: "update", Resource: "projects"}
Expand All @@ -399,7 +397,7 @@ func TestSubjectAccessReview(t *testing.T) {
subjectAccessReviewTest{
clientInterface: haroldClient.RootSubjectAccessReviews(),
review: askCanClusterAdminsCreateProject,
err: "Forbidden",
err: "forbidden",
}.run(t)

askCanICreatePods := &authorizationapi.SubjectAccessReview{Verb: "create", Resource: "projects"}
Expand Down
11 changes: 5 additions & 6 deletions test/integration/bootstrap_policy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
package integration

import (
"strings"
"testing"

kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api"
Expand Down Expand Up @@ -56,21 +55,21 @@ func TestAuthenticatedUsersAgainstOpenshiftNamespace(t *testing.T) {
if _, err := valerieOpenshiftClient.Templates(openshiftSharedResourcesNamespace).List(labels.Everything(), fields.Everything()); err != nil {
t.Errorf("unexpected error: %v", err)
}
if _, err := valerieOpenshiftClient.Templates(kapi.NamespaceDefault).List(labels.Everything(), fields.Everything()); err == nil || !strings.Contains(err.Error(), "Forbidden") {
if _, err := valerieOpenshiftClient.Templates(kapi.NamespaceDefault).List(labels.Everything(), fields.Everything()); err == nil || !kapierror.IsForbidden(err) {
t.Errorf("unexpected error: %v", err)
}

if _, err := valerieOpenshiftClient.ImageRepositories(openshiftSharedResourcesNamespace).List(labels.Everything(), fields.Everything()); err != nil {
t.Errorf("unexpected error: %v", err)
}
if _, err := valerieOpenshiftClient.ImageRepositories(kapi.NamespaceDefault).List(labels.Everything(), fields.Everything()); err == nil || !strings.Contains(err.Error(), "Forbidden") {
if _, err := valerieOpenshiftClient.ImageRepositories(kapi.NamespaceDefault).List(labels.Everything(), fields.Everything()); err == nil || !kapierror.IsForbidden(err) {
t.Errorf("unexpected error: %v", err)
}

if _, err := valerieOpenshiftClient.ImageRepositoryTags(openshiftSharedResourcesNamespace).Get("name", "tag"); !kapierror.IsNotFound(err) {
t.Errorf("unexpected error: %v", err)
}
if _, err := valerieOpenshiftClient.ImageRepositoryTags(kapi.NamespaceDefault).Get("name", "tag"); err == nil || !strings.Contains(err.Error(), "Forbidden") {
if _, err := valerieOpenshiftClient.ImageRepositoryTags(kapi.NamespaceDefault).Get("name", "tag"); err == nil || !kapierror.IsForbidden(err) {
t.Errorf("unexpected error: %v", err)
}
}
Expand All @@ -90,7 +89,7 @@ func TestOverwritePolicyCommand(t *testing.T) {
t.Errorf("unexpected error: %v", err)
}

if _, err := client.Policies(masterConfig.PolicyConfig.MasterAuthorizationNamespace).List(labels.Everything(), fields.Everything()); err == nil || !strings.Contains(err.Error(), "Forbidden") {
if _, err := client.Policies(masterConfig.PolicyConfig.MasterAuthorizationNamespace).List(labels.Everything(), fields.Everything()); err == nil || !kapierror.IsForbidden(err) {
t.Errorf("unexpected error: %v", err)
}

Expand Down Expand Up @@ -156,7 +155,7 @@ func TestSelfSubjectAccessReviews(t *testing.T) {
subjectAccessReviewTest{
clientInterface: valerieOpenshiftClient.SubjectAccessReviews("openshift"),
review: askCanClusterAdminsCreateProject,
err: "Forbidden",
err: "forbidden",
}.run(t)

}