diff --git a/manifests/machineconfigserver/clusterrole.yaml b/manifests/machineconfigserver/clusterrole.yaml index a5bf7e869f..3dd6ffbb8a 100644 --- a/manifests/machineconfigserver/clusterrole.yaml +++ b/manifests/machineconfigserver/clusterrole.yaml @@ -7,3 +7,6 @@ rules: - apiGroups: ["machineconfiguration.openshift.io"] resources: ["machineconfigs", "machineconfigpools"] verbs: ["*"] +- apiGroups: [""] + resources: ["secrets"] + verbs: ["get"] diff --git a/pkg/operator/assets/bindata.go b/pkg/operator/assets/bindata.go index 7fd611fe43..ae9777f73d 100644 --- a/pkg/operator/assets/bindata.go +++ b/pkg/operator/assets/bindata.go @@ -796,6 +796,9 @@ rules: - apiGroups: ["machineconfiguration.openshift.io"] resources: ["machineconfigs", "machineconfigpools"] verbs: ["*"] +- apiGroups: [""] + resources: ["secrets"] + verbs: ["get"] `) func manifestsMachineconfigserverClusterroleYamlBytes() ([]byte, error) { diff --git a/pkg/server/api.go b/pkg/server/api.go index 081c5962a5..684ca07e48 100644 --- a/pkg/server/api.go +++ b/pkg/server/api.go @@ -90,15 +90,22 @@ func (sh *APIHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } + auth := r.URL.Query().Get("auth") + cr := poolRequest{ machineConfigPool: path.Base(r.URL.Path), } - conf, err := sh.server.GetConfig(cr) + conf, err := sh.server.GetConfig(cr, auth) if err != nil { w.Header().Set("Content-Length", "0") - w.WriteHeader(http.StatusInternalServerError) - glog.Errorf("couldn't get config for req: %v, error: %v", cr, err) + if IsForbidden(err) { + w.WriteHeader(http.StatusForbidden) + glog.Infof("Denying unauthorized request: %v", err) + } else { + w.WriteHeader(http.StatusInternalServerError) + glog.Errorf("couldn't get config for req: %v, error: %v", cr, err) + } return } if conf == nil && err == nil { diff --git a/pkg/server/api_test.go b/pkg/server/api_test.go index c4d179bdce..5de10937d1 100644 --- a/pkg/server/api_test.go +++ b/pkg/server/api_test.go @@ -11,11 +11,11 @@ import ( ) type mockServer struct { - GetConfigFn func(poolRequest) (*ignv2_2types.Config, error) + GetConfigFn func(cr poolRequest, auth string) (*ignv2_2types.Config, error) } -func (ms *mockServer) GetConfig(pr poolRequest) (*ignv2_2types.Config, error) { - return ms.GetConfigFn(pr) +func (ms *mockServer) GetConfig(pr poolRequest, auth string) (*ignv2_2types.Config, error) { + return ms.GetConfigFn(pr, auth) } type checkResponse func(t *testing.T, response *http.Response) @@ -23,16 +23,25 @@ type checkResponse func(t *testing.T, response *http.Response) type scenario struct { name string request *http.Request - serverFunc func(poolRequest) (*ignv2_2types.Config, error) + serverFunc func(pr poolRequest, auth string) (*ignv2_2types.Config, error) checkResponse checkResponse } func TestAPIHandler(t *testing.T) { + authServerFunc := func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { + if auth == "letmein" { + return new(ignv2_2types.Config), nil + } + return nil, &configError { + msg: "Not authorized", + forbidden: true, + } + } scenarios := []scenario{ { name: "get config path that does not exist", request: httptest.NewRequest(http.MethodGet, "http://testrequest/config/does-not-exist", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), fmt.Errorf("not acceptable") }, checkResponse: func(t *testing.T, response *http.Response) { @@ -44,7 +53,7 @@ func TestAPIHandler(t *testing.T) { { name: "get config path that exists", request: httptest.NewRequest(http.MethodGet, "http://testrequest/config/master", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), nil }, checkResponse: func(t *testing.T, response *http.Response) { @@ -57,7 +66,7 @@ func TestAPIHandler(t *testing.T) { { name: "head config path that exists", request: httptest.NewRequest(http.MethodHead, "http://testrequest/config/master", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), nil }, checkResponse: func(t *testing.T, response *http.Response) { @@ -70,7 +79,7 @@ func TestAPIHandler(t *testing.T) { { name: "post config path that exists", request: httptest.NewRequest(http.MethodPost, "http://testrequest/config/master", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), nil }, checkResponse: func(t *testing.T, response *http.Response) { @@ -79,6 +88,27 @@ func TestAPIHandler(t *testing.T) { checkBodyLength(t, response, 0) }, }, + { + name: "not auth", + request: httptest.NewRequest(http.MethodGet, "http://testrequest/config/master?auth=bad", nil), + serverFunc: authServerFunc, + checkResponse: func(t *testing.T, response *http.Response) { + checkStatus(t, response, http.StatusForbidden) + checkContentLength(t, response, 0) + checkBodyLength(t, response, 0) + }, + }, + { + name: "auth OK", + request: httptest.NewRequest(http.MethodGet, "http://testrequest/config/master?auth=letmein", nil), + serverFunc: authServerFunc, + checkResponse: func(t *testing.T, response *http.Response) { + checkStatus(t, response, http.StatusOK) + checkContentType(t, response, "application/json") + checkContentLength(t, response, 114) + checkBodyLength(t, response, 114) + }, + }, } for _, scenario := range scenarios { @@ -102,7 +132,7 @@ func TestHealthzHandler(t *testing.T) { { name: "get healthz", request: httptest.NewRequest(http.MethodGet, "http://testrequest/healthz", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), nil }, checkResponse: func(t *testing.T, response *http.Response) { @@ -114,7 +144,7 @@ func TestHealthzHandler(t *testing.T) { { name: "head healthz", request: httptest.NewRequest(http.MethodHead, "http://testrequest/healthz", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), nil }, checkResponse: func(t *testing.T, response *http.Response) { @@ -126,7 +156,7 @@ func TestHealthzHandler(t *testing.T) { { name: "post healthz", request: httptest.NewRequest(http.MethodPost, "http://testrequest/healthz", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), nil }, checkResponse: func(t *testing.T, response *http.Response) { @@ -154,7 +184,7 @@ func TestDefaultHandler(t *testing.T) { { name: "get root", request: httptest.NewRequest(http.MethodGet, "http://testrequest/", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), nil }, checkResponse: func(t *testing.T, response *http.Response) { @@ -166,7 +196,7 @@ func TestDefaultHandler(t *testing.T) { { name: "head root", request: httptest.NewRequest(http.MethodHead, "http://testrequest/", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), nil }, checkResponse: func(t *testing.T, response *http.Response) { @@ -178,7 +208,7 @@ func TestDefaultHandler(t *testing.T) { { name: "post root", request: httptest.NewRequest(http.MethodPost, "http://testrequest/", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), nil }, checkResponse: func(t *testing.T, response *http.Response) { @@ -206,7 +236,7 @@ func TestAPIServer(t *testing.T) { { name: "get config path that does not exist", request: httptest.NewRequest(http.MethodGet, "http://testrequest/config/does-not-exist", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), fmt.Errorf("not acceptable") }, checkResponse: func(t *testing.T, response *http.Response) { @@ -218,7 +248,7 @@ func TestAPIServer(t *testing.T) { { name: "get config path that exists", request: httptest.NewRequest(http.MethodGet, "http://testrequest/config/master", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), nil }, checkResponse: func(t *testing.T, response *http.Response) { @@ -231,7 +261,7 @@ func TestAPIServer(t *testing.T) { { name: "head config path that exists", request: httptest.NewRequest(http.MethodHead, "http://testrequest/config/master", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), nil }, checkResponse: func(t *testing.T, response *http.Response) { @@ -244,7 +274,7 @@ func TestAPIServer(t *testing.T) { { name: "post config path that exists", request: httptest.NewRequest(http.MethodPost, "http://testrequest/config/master", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), nil }, checkResponse: func(t *testing.T, response *http.Response) { @@ -256,7 +286,7 @@ func TestAPIServer(t *testing.T) { { name: "get healthz", request: httptest.NewRequest(http.MethodGet, "http://testrequest/healthz", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), nil }, checkResponse: func(t *testing.T, response *http.Response) { @@ -268,7 +298,7 @@ func TestAPIServer(t *testing.T) { { name: "head healthz", request: httptest.NewRequest(http.MethodHead, "http://testrequest/healthz", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), nil }, checkResponse: func(t *testing.T, response *http.Response) { @@ -280,7 +310,7 @@ func TestAPIServer(t *testing.T) { { name: "post healthz", request: httptest.NewRequest(http.MethodPost, "http://testrequest/healthz", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), nil }, checkResponse: func(t *testing.T, response *http.Response) { @@ -292,7 +322,7 @@ func TestAPIServer(t *testing.T) { { name: "get root", request: httptest.NewRequest(http.MethodGet, "http://testrequest/", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), nil }, checkResponse: func(t *testing.T, response *http.Response) { @@ -304,7 +334,7 @@ func TestAPIServer(t *testing.T) { { name: "head root", request: httptest.NewRequest(http.MethodHead, "http://testrequest/", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), nil }, checkResponse: func(t *testing.T, response *http.Response) { @@ -316,7 +346,7 @@ func TestAPIServer(t *testing.T) { { name: "post root", request: httptest.NewRequest(http.MethodPost, "http://testrequest/", nil), - serverFunc: func(poolRequest) (*ignv2_2types.Config, error) { + serverFunc: func(pr poolRequest, auth string) (*ignv2_2types.Config, error) { return new(ignv2_2types.Config), nil }, checkResponse: func(t *testing.T, response *http.Response) { diff --git a/pkg/server/bootstrap_server.go b/pkg/server/bootstrap_server.go index e394ea6e32..2a77421bd3 100644 --- a/pkg/server/bootstrap_server.go +++ b/pkg/server/bootstrap_server.go @@ -56,7 +56,8 @@ func NewBootstrapServer(dir, kubeconfig string) (Server, error) { // 3. Load the machine config. // 4. Append the machine annotations file. // 5. Append the KubeConfig file. -func (bsc *bootstrapServer) GetConfig(cr poolRequest) (*ignv2_2types.Config, error) { +func (bsc *bootstrapServer) GetConfig(cr poolRequest, auth string) (*ignv2_2types.Config, error) { + // Note we explicitly ignore auth for bootstrap. // 1. Read the Machine Config Pool object. fileName := path.Join(bsc.serverBaseDir, "machine-pools", cr.machineConfigPool+".yaml") diff --git a/pkg/server/cluster_server.go b/pkg/server/cluster_server.go index 9cd0db4002..f0b8cd0461 100644 --- a/pkg/server/cluster_server.go +++ b/pkg/server/cluster_server.go @@ -1,6 +1,7 @@ package server import ( + "github.com/pkg/errors" "fmt" "io/ioutil" "path/filepath" @@ -10,8 +11,10 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" rest "k8s.io/client-go/rest" + clientset "k8s.io/client-go/kubernetes" clientcmd "k8s.io/client-go/tools/clientcmd" clientcmdv1 "k8s.io/client-go/tools/clientcmd/api/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned/typed/machineconfiguration.openshift.io/v1" ) @@ -22,6 +25,9 @@ const ( inClusterConfig = "" bootstrapTokenDir = "/etc/mcs/bootstrap-token" + + componentNamespace = "openshift-machine-config-operator" + ignitionAuth = "ignition-auth" ) // ensure clusterServer implements the @@ -29,6 +35,8 @@ const ( var _ = Server(&clusterServer{}) type clusterServer struct { + // kubeClient is used for the Ignition secret currently + kubeClient clientset.Interface // machineClient is used to interact with the // machine config, pool objects. machineClient v1.MachineconfigurationV1Interface @@ -48,8 +56,13 @@ func NewClusterServer(kubeConfig, apiserverURL string) (Server, error) { return nil, fmt.Errorf("Failed to create Kubernetes rest client: %v", err) } + kc, err := clientset.NewForConfig(restConfig) + if err != nil { + return nil, err + } mc := v1.NewForConfigOrDie(restConfig) return &clusterServer{ + kubeClient: kc, machineClient: mc, kubeconfigFunc: func() ([]byte, []byte, error) { return kubeconfigFromSecret(bootstrapTokenDir, apiserverURL) }, }, nil @@ -57,8 +70,32 @@ func NewClusterServer(kubeConfig, apiserverURL string) (Server, error) { // GetConfig fetches the machine config(type - Ignition) from the cluster, // based on the pool request. -func (cs *clusterServer) GetConfig(cr poolRequest) (*ignv2_2types.Config, error) { - mp, err := cs.machineClient.MachineConfigPools().Get(cr.machineConfigPool, metav1.GetOptions{}) +func (cs *clusterServer) GetConfig(cr poolRequest, auth string) (*ignv2_2types.Config, error) { + authSecretObj, err := cs.kubeClient.CoreV1().Secrets(componentNamespace).Get(ignitionAuth, metav1.GetOptions{}) + authEnabled := true + if err != nil { + // For backwards compat, allow any request if the secret isn't found + if apierrors.IsNotFound(err) { + authEnabled = false + } else { + return nil, errors.Wrapf(err, "getting ignition-auth") + } + } + + name := cr.machineConfigPool + + // If there's a secret, require that it was passed as a query parameter. + if authEnabled { + authSecret := string(authSecretObj.Data[name]) + if auth != authSecret { + return nil, &configError { + msg: "Not authorized", + forbidden: true, + } + } + } + + mp, err := cs.machineClient.MachineConfigPools().Get(name, metav1.GetOptions{}) if err != nil { return nil, fmt.Errorf("could not fetch pool. err: %v", err) } diff --git a/pkg/server/server.go b/pkg/server/server.go index 776252a437..407e9ce844 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -31,10 +31,30 @@ type kubeconfigFunc func() (kubeconfigData []byte, rootCAData []byte, err error) // appenderFunc appends Config. type appenderFunc func(*ignv2_2types.Config) error +// configError is returned by the GetConfig API +type configError struct { + msg string + forbidden bool +} + +// configError returns the string +func (e *configError) Error() string { + return e.msg +} + +// IsForbidden says if err is an configError with forbidden set +func IsForbidden(err error) bool { + switch t := err.(type) { + case *configError: + return t.forbidden + } + return false +} + // Server defines the interface that is implemented by different // machine config server implementations. type Server interface { - GetConfig(poolRequest) (*ignv2_2types.Config, error) + GetConfig(cr poolRequest, auth string) (*ignv2_2types.Config, error) } func getAppenders(cr poolRequest, currMachineConfig string, f kubeconfigFunc, osimageurl string) []appenderFunc { diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index 89d7fb7adb..e2ffcdeb9f 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -12,6 +12,7 @@ import ( yaml "github.com/ghodss/yaml" "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" daemonconsts "github.com/openshift/machine-config-operator/pkg/daemon/constants" + k8sfake "k8s.io/client-go/kubernetes/fake" "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned/fake" ) @@ -99,7 +100,7 @@ func TestBootstrapServer(t *testing.T) { } res, err := bs.GetConfig(poolRequest{ machineConfigPool: testPool, - }) + }, "") if err != nil { t.Fatalf("expected err to be nil, received: %v", err) } @@ -151,6 +152,7 @@ func TestClusterServer(t *testing.T) { } csc := &clusterServer{ + kubeClient: k8sfake.NewSimpleClientset(), machineClient: cs.MachineconfigurationV1(), kubeconfigFunc: func() ([]byte, []byte, error) { return getKubeConfigContent(t) }, } @@ -174,7 +176,7 @@ func TestClusterServer(t *testing.T) { res, err := csc.GetConfig(poolRequest{ machineConfigPool: testPool, - }) + }, "") if err != nil { t.Fatalf("expected err to be nil, received: %v", err) }