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

server: Add optional auth token #736

Closed
wants to merge 1 commit into from
Closed

server: Add optional auth token #736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions manifests/machineconfigserver/clusterrole.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,6 @@ rules:
- apiGroups: ["machineconfiguration.openshift.io"]
resources: ["machineconfigs", "machineconfigpools"]
verbs: ["*"]
- apiGroups: [""]
resources: ["secrets"]
verbs: ["get"]
3 changes: 3 additions & 0 deletions pkg/operator/assets/bindata.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 10 additions & 3 deletions pkg/server/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
78 changes: 54 additions & 24 deletions pkg/server/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,28 +11,37 @@ 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)

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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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 {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down
3 changes: 2 additions & 1 deletion pkg/server/bootstrap_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
41 changes: 39 additions & 2 deletions pkg/server/cluster_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package server

import (
"github.com/pkg/errors"
"fmt"
"io/ioutil"
"path/filepath"
Expand All @@ -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"
)
Expand All @@ -22,13 +25,18 @@ const (
inClusterConfig = ""

bootstrapTokenDir = "/etc/mcs/bootstrap-token"

componentNamespace = "openshift-machine-config-operator"
ignitionAuth = "ignition-auth"
)

// ensure clusterServer implements the
// Server interface.
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
Expand All @@ -48,17 +56,46 @@ 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
}

// 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{})
cgwalters marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

The ability to configure multiple secrets would make later rotation much easier.

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])
Copy link
Contributor

Choose a reason for hiding this comment

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

If an administrator forgets to configure an authsecret for a new machine pool, this will fail open.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's intentional for backcompat reasons.

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)
}
Expand Down
Loading