Skip to content

Commit

Permalink
Add checks to backend for terminal proxy
Browse files Browse the repository at this point in the history
Access to commandline terminals (cloud shells) has to be restricted to
avoid privilege escalation issues. This commit adds the following
checks:

- Ensure webhooks expected by che-workspace-operator are installed
- Ensures the user requesting a terminal is not a cluster admin (done by
  checking if they can read pods in a privileged namespace
  (openshift-operators).

Adds an endpoint: /api/terminal/available/ that can be used to check if
basic requirements for the proxy are met (workspace operator is running)

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
  • Loading branch information
amisevsk committed May 21, 2020
1 parent a8eeb7a commit ccdb51f
Show file tree
Hide file tree
Showing 6 changed files with 180 additions and 42 deletions.
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ require (
gopkg.in/square/go-jose.v2 v2.4.1 // indirect
gopkg.in/yaml.v2 v2.2.4
helm.sh/helm/v3 v3.0.1
k8s.io/api v0.0.0-20191016110408-35e52d86657a
k8s.io/apiextensions-apiserver v0.0.0-20191016113550-5357c4baaf65
k8s.io/apimachinery v0.0.0-20191004115801-a2eda9f80ab8
k8s.io/cli-runtime v0.0.0-20191016114015-74ad18325ed5
Expand Down
4 changes: 3 additions & 1 deletion pkg/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,9 @@ func (s *Server) HTTPHandler() http.Handler {
TLSClientConfig: s.K8sProxyConfig.TLSClientConfig,
ClusterEndpoint: s.K8sProxyConfig.Endpoint,
}
handle(terminal.Endpoint, authHandlerWithUser(terminalProxy.Handle))
handle(terminal.Endpoint, authHandlerWithUser(terminalProxy.HandleProxy))

handleFunc(terminal.AvailableEndpoint, terminalProxy.HandleProxyEnabled)

if s.prometheusProxyEnabled() {
// Only proxy requests to the Prometheus API, not the UI.
Expand Down
27 changes: 27 additions & 0 deletions pkg/terminal/auth.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package terminal

import (
authv1 "k8s.io/api/authorization/v1"
)

func (p *Proxy) checkUserPermissions(token string) (bool, error) {
client, err := p.createTypedClient(token)
if err != nil {
return false, err
}

sar := &authv1.SelfSubjectAccessReview{
Spec: authv1.SelfSubjectAccessReviewSpec{
ResourceAttributes: &authv1.ResourceAttributes{
Namespace: "openshift-operators",
Verb: "get",
Resource: "pods",
},
},
}
res, err := client.AuthorizationV1().SelfSubjectAccessReviews().Create(sar)
if err != nil || res == nil {
return false, err
}
return !res.Status.Allowed, nil
}
56 changes: 56 additions & 0 deletions pkg/terminal/client.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package terminal

import (
"k8s.io/client-go/dynamic"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/rest"
)

// createDynamicClient create dynamic client with the configured token to be used
func (p *Proxy) createDynamicClient(token string) (dynamic.Interface, error) {
var tlsClientConfig rest.TLSClientConfig
if p.TLSClientConfig.InsecureSkipVerify {
// off-cluster mode
tlsClientConfig.Insecure = true
} else {
inCluster, err := rest.InClusterConfig()
if err != nil {
return nil, err
}
tlsClientConfig = inCluster.TLSClientConfig
}

config := &rest.Config{
Host: p.ClusterEndpoint.Host,
TLSClientConfig: tlsClientConfig,
BearerToken: token,
}

client, err := dynamic.NewForConfig(dynamic.ConfigFor(config))
if err != nil {
return nil, err
}
return client, nil
}

func (p *Proxy) createTypedClient(token string) (*kubernetes.Clientset, error) {
var tlsClientConfig rest.TLSClientConfig
if p.TLSClientConfig.InsecureSkipVerify {
// off-cluster mode
tlsClientConfig.Insecure = true
} else {
inCluster, err := rest.InClusterConfig()
if err != nil {
return nil, err
}
tlsClientConfig = inCluster.TLSClientConfig
}

config := &rest.Config{
Host: p.ClusterEndpoint.Host,
TLSClientConfig: tlsClientConfig,
BearerToken: token,
}

return kubernetes.NewForConfig(config)
}
40 changes: 40 additions & 0 deletions pkg/terminal/operator.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package terminal

import (
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/rest"
)

const (
webhookName = "workspace.che.eclipse.org"
)

func workspaceOperatorIsRunning() (bool, error) {
config, err := rest.InClusterConfig()
if err != nil {
return false, err
}
client, err := kubernetes.NewForConfig(config)
if err != nil {
return false, err
}

_, err = client.AdmissionregistrationV1().MutatingWebhookConfigurations().Get(webhookName, metav1.GetOptions{})
if err != nil {
if errors.IsNotFound(err) {
return false, nil
}
return false, err
}

_, err = client.AdmissionregistrationV1().ValidatingWebhookConfigurations().Get(webhookName, metav1.GetOptions{})
if err != nil {
if errors.IsNotFound(err) {
return false, nil
}
return false, err
}
return true, nil
}
94 changes: 53 additions & 41 deletions pkg/terminal/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,17 @@ import (
"net/url"
"strings"

"github.com/openshift/console/pkg/auth"
"github.com/openshift/console/pkg/proxy"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/dynamic"
"k8s.io/client-go/rest"

"github.com/openshift/console/pkg/auth"
"github.com/openshift/console/pkg/proxy"
)

const (
//Endpoint that Proxy is supposed to handle
Endpoint = "/api/terminal/"
// Endpoint that Proxy is supposed to handle
Endpoint = "/api/terminal/"
AvailableEndpoint = "/api/terminal/available/"
)

// Proxy provides handlers to handle terminal related requests
Expand All @@ -42,21 +40,38 @@ var (
}
)

// Handle evaluates the namespace and workspace names from URL and after check that
// HandleProxy evaluates the namespace and workspace names from URL and after check that
// it's created by the current user - proxies the request there
func (p *Proxy) Handle(user *auth.User, w http.ResponseWriter, r *http.Request) {
func (p *Proxy) HandleProxy(user *auth.User, w http.ResponseWriter, r *http.Request) {
switch r.Method {
case
"GET",
"HEAD",
"OPTIONS",
"TRACE":
//These methods are considered as not vulnerable for CSRF attack, so the corresponding CSRF token check is skipped.
// These methods are considered as not vulnerable for CSRF attack, so the corresponding CSRF token check is skipped.
// But in terminal-proxy we must make sure that it's a request made by OpenShift Console
// before proxying request with passed token
w.WriteHeader(http.StatusForbidden)
return
}
operatorRunning, err := workspaceOperatorIsRunning()
if err != nil {
http.Error(w, "Failed to check workspace operator state. Cause: "+err.Error(), http.StatusInternalServerError)
}
if !operatorRunning {
http.Error(w, "Terminal endpoint is disabled: workspace operator is not deployed.", http.StatusForbidden)
return
}

enabledForUser, err := p.checkUserPermissions(user.Token)
if err != nil {
http.Error(w, "Failed to check workspace operator state. Cause: "+err.Error(), http.StatusInternalServerError)
}
if !enabledForUser {
http.Error(w, "Terminal is disabled for cluster-admin users.", http.StatusForbidden)
return
}

ok, namespace, workspaceName, path := stripTerminalAPIPrefix(r.URL.Path)
if !ok {
Expand All @@ -72,7 +87,7 @@ func (p *Proxy) Handle(user *auth.User, w http.ResponseWriter, r *http.Request)

userId := user.ID
if userId == "" {
//user id is missing, auth is used that does not support user info propagated, like OpenShift OAuth
// user id is missing, auth is used that does not support user info propagated, like OpenShift OAuth
userInfo, err := client.Resource(UserGroupVersionResource).Get("~", metav1.GetOptions{})
if err != nil {
http.Error(w, "Failed to retrieve the current user info. Cause: "+err.Error(), http.StatusInternalServerError)
Expand All @@ -81,7 +96,7 @@ func (p *Proxy) Handle(user *auth.User, w http.ResponseWriter, r *http.Request)

userId = string(userInfo.GetUID())
if userId == "" {
//uid is missing. it must be kube:admin
// uid is missing. it must be kube:admin
if "kube:admin" != userInfo.GetName() {
http.Error(w, "User must have UID to proceed authorization", http.StatusInternalServerError)
return
Expand Down Expand Up @@ -110,7 +125,30 @@ func (p *Proxy) Handle(user *auth.User, w http.ResponseWriter, r *http.Request)
p.proxyToWorkspace(terminalHost, path, user.Token, r, w)
}

//stripTerminalAPIPrefix strips path prefix that is expected for Terminal API request
func (p *Proxy) HandleProxyEnabled(w http.ResponseWriter, r *http.Request) {
if r.Method != "GET" {
w.Header().Set("Allow", "GET")
w.WriteHeader(http.StatusMethodNotAllowed)
return
}

enabled, err := workspaceOperatorIsRunning()
if err != nil {
plog.Info("[available] Got error when checking user: %s", err)
w.WriteHeader(http.StatusInternalServerError)
return
}
if !enabled {
plog.Info("[available] Operator is not running, endpoint is disabled.")
w.WriteHeader(http.StatusServiceUnavailable)
return
}
plog.Info("[available] Terminal is available.")
w.WriteHeader(http.StatusNoContent)
return
}

// stripTerminalAPIPrefix strips path prefix that is expected for Terminal API request
func stripTerminalAPIPrefix(requestPath string) (ok bool, namespace string, workspaceName string, path string) {
// URL is supposed to have the following format
// -> /api/terminal/{namespace}/{workspace-name}/{path} < optional
Expand All @@ -128,7 +166,7 @@ func stripTerminalAPIPrefix(requestPath string) (ok bool, namespace string, work
}
}

//getBaseTerminalHost evaluates ideUrl from the specified workspace and extract host from it
// getBaseTerminalHost evaluates ideUrl from the specified workspace and extract host from it
func (p *Proxy) getBaseTerminalHost(ws *unstructured.Unstructured) (*url.URL, error) {
ideUrl, success, err := unstructured.NestedString(ws.UnstructuredContent(), "status", "ideUrl")
if !success {
Expand Down Expand Up @@ -161,7 +199,7 @@ func (p *Proxy) proxyToWorkspace(workspaceIdeHost *url.URL, path string, token s

r2.URL.Path = path

//TODO a new proxy per request is created. Must be revised and probably changed
// TODO a new proxy per request is created. Must be revised and probably changed
terminalProxy := proxy.NewProxy(&proxy.Config{
Endpoint: workspaceIdeHost,
TLSClientConfig: p.TLSClientConfig,
Expand All @@ -170,29 +208,3 @@ func (p *Proxy) proxyToWorkspace(workspaceIdeHost *url.URL, path string, token s
terminalProxy.ServeHTTP(w, r2)
}

//createDynamicClient create dynamic client with the configured token to be used
func (p *Proxy) createDynamicClient(token string) (dynamic.Interface, error) {
var tlsClientConfig rest.TLSClientConfig
if p.TLSClientConfig.InsecureSkipVerify {
//off-cluster mode
tlsClientConfig.Insecure = true
} else {
inCluster, err := rest.InClusterConfig()
if err != nil {
return nil, err
}
tlsClientConfig = inCluster.TLSClientConfig
}

config := &rest.Config{
Host: p.ClusterEndpoint.Host,
TLSClientConfig: tlsClientConfig,
BearerToken: token,
}

client, err := dynamic.NewForConfig(dynamic.ConfigFor(config))
if err != nil {
return nil, err
}
return client, nil
}

0 comments on commit ccdb51f

Please sign in to comment.