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.

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

validate capi-auth-token on dqlite/remove #56

6 changes: 6 additions & 0 deletions pkg/api/v2/consts.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package v2

const (
// CAPIAuthTokenHeader is the header used to pass the CAPI auth token.
CAPIAuthTokenHeader = "capi-auth-token"
)
4 changes: 3 additions & 1 deletion pkg/api/v2/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ func (a *API) RegisterServer(server *http.ServeMux, middleware func(f http.Handl
return
}

if rc, err := a.RemoveFromDqlite(r.Context(), req); err != nil {
token := r.Header.Get(CAPIAuthTokenHeader)

if rc, err := a.RemoveFromDqlite(r.Context(), req, token); err != nil {
httputil.Error(w, rc, fmt.Errorf("failed to remove from dqlite: %w", err))
return
}
Expand Down
13 changes: 11 additions & 2 deletions pkg/api/v2/remove.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,20 @@ import (
// RemoveFromDqliteRequest represents a request to remove a node from the dqlite cluster.
type RemoveFromDqliteRequest struct {
// RemoveEndpoint is the endpoint of the node to remove from the dqlite cluster.
RemoveEndpoint string `json:"removeEndpoint"`
RemoveEndpoint string `json:"remove_endpoint"`
}

// RemoveFromDqlite implements the "POST /v2/dqlite/remove" endpoint and removes a node from the dqlite cluster.
func (a *API) RemoveFromDqlite(ctx context.Context, req RemoveFromDqliteRequest) (int, error) {
func (a *API) RemoveFromDqlite(ctx context.Context, req RemoveFromDqliteRequest, token string) (int, error) {
isValid, err := a.Snap.IsCAPIAuthTokenValid(token)
Copy link
Contributor

Choose a reason for hiding this comment

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

For me, the validation of the token is a separate middleware/access handler that could potential be reused for other endpoints (similar to what we do in k8s-snap)

Disclaimer: I don't know much about the microk8s cluster agent. If this matches the other code structure I'm fine with that.

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 really like what you're saying, but IIUC the middlewares are not configurable per endpoint in cluster-agent: https://github.com/canonical/microk8s-cluster-agent/blob/main/pkg/server/server.go#L45-L46
All the v1 or v2 endpoints get registered with the same middleware.
We technically can have this capi-auth-token header check as a middleware, but since it's going to get applied for the other v2 endpoints as well (/image/import and /join) we need to make it optional, which is counter intuitive. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, thanks for pointing this out. Will do.

Copy link
Contributor Author

@HomayoonAlimohammadi HomayoonAlimohammadi Oct 11, 2024

Choose a reason for hiding this comment

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

@bschimke95 This is the best that I could come up with. TBH I feel like the previous implementation was more readable and understandable. Maybe I'm doing something wrong. WDYT?
2bcaeca

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'm reverting this. Not really happy with the result...

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, up to you. This was only a suggestion. If you think the current implementation is more straight-forward - go for it :)

if err != nil {
return http.StatusInternalServerError, fmt.Errorf("failed to validate CAPI auth token: %w", err)
}

if !isValid {
return http.StatusUnauthorized, fmt.Errorf("invalid CAPI auth token %q", token)
}

if err := snaputil.RemoveNodeFromDqlite(ctx, a.Snap, req.RemoveEndpoint); err != nil {
return http.StatusInternalServerError, fmt.Errorf("failed to remove node from dqlite: %w", err)
}
Expand Down
40 changes: 36 additions & 4 deletions pkg/api/v2/remove_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,55 @@ func TestRemove(t *testing.T) {
cmdErr := errors.New("failed to run command")
apiv2 := &v2.API{
Snap: &mock.Snap{
RunCommandErr: cmdErr,
RunCommandErr: cmdErr,
CAPIAuthTokenValid: true,
},
}

rc, err := apiv2.RemoveFromDqlite(context.Background(), v2.RemoveFromDqliteRequest{RemoveEndpoint: "1.1.1.1:1234"})
rc, err := apiv2.RemoveFromDqlite(context.Background(), v2.RemoveFromDqliteRequest{RemoveEndpoint: "1.1.1.1:1234"}, "token")

g := NewWithT(t)
g.Expect(err).To(MatchError(cmdErr))
g.Expect(rc).To(Equal(http.StatusInternalServerError))
})

t.Run("InvalidToken", func(t *testing.T) {
apiv2 := &v2.API{
Snap: &mock.Snap{
CAPIAuthTokenValid: false, // explicitly set to false
},
}

rc, err := apiv2.RemoveFromDqlite(context.Background(), v2.RemoveFromDqliteRequest{RemoveEndpoint: "1.1.1.1:1234"}, "token")

g := NewWithT(t)
g.Expect(err).To(HaveOccurred())
g.Expect(rc).To(Equal(http.StatusUnauthorized))
})

t.Run("TokenFileNotFound", func(t *testing.T) {
tokenErr := errors.New("token file not found")
apiv2 := &v2.API{
Snap: &mock.Snap{
CAPIAuthTokenError: tokenErr,
},
}

rc, err := apiv2.RemoveFromDqlite(context.Background(), v2.RemoveFromDqliteRequest{RemoveEndpoint: "1.1.1.1:1234"}, "token")

g := NewWithT(t)
g.Expect(err).To(MatchError(tokenErr))
g.Expect(rc).To(Equal(http.StatusInternalServerError))
})

t.Run("RemovesSuccessfully", func(t *testing.T) {
apiv2 := &v2.API{
Snap: &mock.Snap{},
Snap: &mock.Snap{
CAPIAuthTokenValid: true,
},
}

rc, err := apiv2.RemoveFromDqlite(context.Background(), v2.RemoveFromDqliteRequest{RemoveEndpoint: "1.1.1.1:1234"})
rc, err := apiv2.RemoveFromDqlite(context.Background(), v2.RemoveFromDqliteRequest{RemoveEndpoint: "1.1.1.1:1234"}, "token")

g := NewWithT(t)
g.Expect(err).ToNot(HaveOccurred())
Expand Down
5 changes: 5 additions & 0 deletions pkg/snap/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ type Snap interface {
GetSnapDataPath(parts ...string) string
// GetSnapCommonPath returns the path to a file or directory in the snap's common directory.
GetSnapCommonPath(parts ...string) string
// GetCAPIPath returns the path to a file or directory in the CAPI directory.
GetCAPIPath(parts ...string) string

// RunCommand runs a shell command.
RunCommand(ctx context.Context, commands ...string) error
Expand Down Expand Up @@ -98,6 +100,9 @@ type Snap interface {
// GetKnownToken returns the token for a known user from the known_users.csv file.
GetKnownToken(username string) (string, error)

// IsCAPIAuthTokenValid returns true if token is a valid CAPI auth token.
IsCAPIAuthTokenValid(token string) (bool, error)

// SignCertificate signs the certificate signing request, and returns the certificate in PEM format.
SignCertificate(ctx context.Context, csrPEM []byte) ([]byte, error)

Expand Down
14 changes: 14 additions & 0 deletions pkg/snap/mock/mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ type Snap struct {
SnapDir string
SnapDataDir string
SnapCommonDir string
CAPIDir string

RunCommandCalledWith []RunCommandCall
RunCommandErr error
Expand Down Expand Up @@ -85,6 +86,9 @@ type Snap struct {
KubeletTokens map[string]string // map hostname to token
KnownTokens map[string]string // map username to token

CAPIAuthTokenValid bool
CAPIAuthTokenError error

SignCertificateCalledWith []string // string(csrPEM)
SignedCertificate string

Expand Down Expand Up @@ -116,6 +120,11 @@ func (s *Snap) GetSnapCommonPath(parts ...string) string {
return filepath.Join(append([]string{s.SnapCommonDir}, parts...)...)
}

// GetCAPIPath is a mock implementation for the snap.Snap interface.
func (s *Snap) GetCAPIPath(parts ...string) string {
return filepath.Join(append([]string{s.CAPIDir}, parts...)...)
}

// RunCommand is a mock implementation for the snap.Snap interface.
func (s *Snap) RunCommand(_ context.Context, commands ...string) error {
s.RunCommandCalledWith = append(s.RunCommandCalledWith, RunCommandCall{Commands: commands})
Expand Down Expand Up @@ -320,6 +329,11 @@ func (s *Snap) GetKnownToken(username string) (string, error) {
return "", fmt.Errorf("no known token for user %s", username)
}

// IsCAPIAuthTokenValid is a mock implementation for the snap.Snap interface.
func (s *Snap) IsCAPIAuthTokenValid(token string) (bool, error) {
return s.CAPIAuthTokenValid, s.CAPIAuthTokenError
}

// RunUpgrade is a mock implementation for the snap.Snap interface.
func (s *Snap) RunUpgrade(ctx context.Context, upgrade string, phase string) error {
s.RunUpgradeCalledWith = append(s.RunUpgradeCalledWith, fmt.Sprintf("%s %s", upgrade, phase))
Expand Down
7 changes: 7 additions & 0 deletions pkg/snap/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,10 @@ func WithCommandRunner(f func(context.Context, ...string) error) func(s *snap) {
s.runCommand = f
}
}

// WithCAPIPath configures the path to the CAPI directory.
func WithCAPIPath(path string) func(s *snap) {
return func(s *snap) {
s.capiPath = path
}
}
18 changes: 18 additions & 0 deletions pkg/snap/snap.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ type snap struct {
snapDir string
snapDataDir string
snapCommonDir string
capiPath string
runCommand func(context.Context, ...string) error

clusterTokensMu sync.Mutex
Expand All @@ -34,13 +35,18 @@ type snap struct {
applyCNIBackoff time.Duration
}

const (
defaultCAPIPath = "/capi"
)

// NewSnap creates a new interface with the MicroK8s snap.
// NewSnap accepts the $SNAP, $SNAP_DATA and $SNAP_COMMON, directories, and a number of options.
func NewSnap(snapDir, snapDataDir, snapCommonDir string, options ...func(s *snap)) Snap {
s := &snap{
snapDir: snapDir,
snapDataDir: snapDataDir,
snapCommonDir: snapCommonDir,
capiPath: defaultCAPIPath,
runCommand: util.RunCommand,
}

Expand All @@ -65,6 +71,9 @@ func (s *snap) GetSnapDataPath(parts ...string) string {
func (s *snap) GetSnapCommonPath(parts ...string) string {
return filepath.Join(append([]string{s.snapCommonDir}, parts...)...)
}
func (s *snap) GetCAPIPath(parts ...string) string {
return filepath.Join(append([]string{s.capiPath}, parts...)...)
}

func (s *snap) GetGroupName() string {
if s.isStrict() {
Expand Down Expand Up @@ -331,6 +340,15 @@ func (s *snap) GetKnownToken(username string) (string, error) {
return "", fmt.Errorf("no known token found for user %s", username)
}

// IsCAPIAuthTokenValid checks if the given CAPI auth token is valid.
func (s *snap) IsCAPIAuthTokenValid(token string) (bool, error) {
contents, err := util.ReadFile(s.GetCAPIPath("etc", "token"))
if err != nil {
return false, fmt.Errorf("failed to read token file: %w", err)
}
return strings.TrimSpace(contents) == token, nil
}

func (s *snap) SignCertificate(ctx context.Context, csrPEM []byte) ([]byte, error) {
// TODO: consider using crypto/x509 for this instead of relying on openssl commands.
// NOTE(neoaggelos): x509.CreateCertificate() has some hardcoded fields that are incompatible with MicroK8s.
Expand Down
37 changes: 37 additions & 0 deletions pkg/snap/snap_capi_token_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package snap_test

import (
"os"
"path/filepath"
"testing"

. "github.com/onsi/gomega"

"github.com/canonical/microk8s-cluster-agent/pkg/snap"
)

func TestCAPIAuthToken(t *testing.T) {
capiTestPath := "./capi-test"
os.RemoveAll(capiTestPath)
s := snap.NewSnap("", "", "", snap.WithCAPIPath(capiTestPath))
token := "token123"

g := NewWithT(t)

isValid, err := s.IsCAPIAuthTokenValid(token)
g.Expect(err).To(MatchError(os.ErrNotExist))
g.Expect(isValid).To(BeFalse())

capiEtc := filepath.Join(capiTestPath, "etc")
defer os.RemoveAll(capiTestPath)
g.Expect(os.MkdirAll(capiEtc, 0755)).To(Succeed())
g.Expect(os.WriteFile("./capi-test/etc/token", []byte(token), 0600)).To(Succeed())

isValid, err = s.IsCAPIAuthTokenValid("random-token")
g.Expect(err).ToNot(HaveOccurred())
g.Expect(isValid).To(BeFalse())

isValid, err = s.IsCAPIAuthTokenValid(token)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(isValid).To(BeTrue())
}
Loading