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

HomayoonAlimohammadi · 2024-10-09T13:44:35Z

Summary

This PR adds the capi-auth-token header validation for the dqlite/remove endpoint.
The /capi/etc/token file is supposed to get created by the Microk8s bootstrap provider (in the cloud-init) and will get populated with a token string which is used to authenticate the CAPI calls (the dqlite/remove call specifically) to the cluster-agent.

Also the removeEndpoint request field is changed to remove_endpoint to comply with the recent MAAS API guidelines.

Alternative solution

We could have a "create capi token" endpoint that creates the token file and returns the /path/to/token as well as the token itself. Right now we introduced some coupling between Microk8s CAPI and cluster-agent since they both should be aware that the token path is /capi/etc/token. Implementing this solution and improving on the current implementation might be considered later.

PR series

Cluster agent token validation --> This one
Microk8s bootstrap provider add token file and secret --> Add capi-auth-token file to control plane machines cluster-api-bootstrap-provider-microk8s#115
Microk8s control plane provider use token for request --> Add capi-auth-token header to /dqlite/remove request cluster-api-control-plane-provider-microk8s#68

berkayoz

Great work, did a first pass

pkg/api/v2/remove.go

cmd/cluster-agent.go

bschimke95

Looking good, some first comments.

pkg/api/v2/remove.go

bschimke95 · 2024-10-10T15:39:57Z

pkg/api/v2/remove.go

 // 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)


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.

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?

You can simply set a new middleware on a per route basis:
https://github.com/canonical/microk8s-cluster-agent/blob/main/pkg/server/server.go#L19-L27

For this route you would add it here:
https://github.com/canonical/microk8s-cluster-agent/pull/56/files#diff-6ecd338993def4d5349db1e632bfa290c6aa5d6a7e04e6413f0f27e8983161f3R58

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

@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

I'm reverting this. Not really happy with the result...

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

This reverts commit 2496acf.

This reverts commit 2bcaeca.

bschimke95

LGTM

bschimke95 · 2024-10-11T10:10:21Z

pkg/api/v2/remove.go

 // 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)


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

berkayoz

LGTM. Although initially we had the os.GETENV("CAPI_PATH") that could override the default path. Did we forget to change the cluster agent command to use the WithCAPIPath option?

HomayoonAlimohammadi · 2024-10-11T10:19:16Z

@berkayoz I think the /capi is now the default path: https://github.com/canonical/microk8s-cluster-agent/blob/KU-1719/validate-capi-auth-token-on-remove/pkg/snap/snap.go#L49
We also have the WithCAPIPath option to tweak it in case we needed to:

Did I understand your point correctly?

HomayoonAlimohammadi · 2024-10-11T10:28:38Z

According to a conversation with @berkayoz, we decided to go with hard-coding that path for now and see if we need to make that configurable later on.

Note for future travelers

The path defined here should match the one in the "Microk8s CAPI bootstrap provider". That provider is responsible for populating this path with needed files and info (e.g. CAPI auth token).

* Add /v2/dqlite/remove endpoint (#55) * validate capi-auth-token on dqlite/remove (#56)

HomayoonAlimohammadi added 2 commits October 9, 2024 17:30

Validate capi-auth-token on dqlite/remove

55a739f

Fix lint

d41addb

HomayoonAlimohammadi requested a review from a team October 9, 2024 13:44

This was referenced Oct 9, 2024

Add capi-auth-token header to /dqlite/remove request canonical/cluster-api-control-plane-provider-microk8s#68

Merged

Add capi-auth-token file to control plane machines canonical/cluster-api-bootstrap-provider-microk8s#115

Merged

berkayoz reviewed Oct 10, 2024

View reviewed changes

pkg/api/v2/remove.go Outdated Show resolved Hide resolved

cmd/cluster-agent.go Outdated Show resolved Hide resolved

Address comments and issues

f5d5ad2

HomayoonAlimohammadi requested a review from berkayoz October 10, 2024 12:57

bschimke95 reviewed Oct 10, 2024

View reviewed changes

HomayoonAlimohammadi added 6 commits October 11, 2024 10:37

Address comments and issues

22a3bb7

Fix test

e711313

Add middleware for capi auth token

2bcaeca

Fix lint

2496acf

Revert "Fix lint"

6e986af

This reverts commit 2496acf.

Revert "Add middleware for capi auth token"

f4e5819

This reverts commit 2bcaeca.

HomayoonAlimohammadi requested a review from bschimke95 October 11, 2024 09:57

bschimke95 approved these changes Oct 11, 2024

View reviewed changes

berkayoz approved these changes Oct 11, 2024

View reviewed changes

HomayoonAlimohammadi merged commit 5e30719 into dqlite-remove-endpoint Oct 11, 2024
3 checks passed

HomayoonAlimohammadi deleted the KU-1719/validate-capi-auth-token-on-remove branch October 11, 2024 10:30

HomayoonAlimohammadi mentioned this pull request Oct 11, 2024

Add Dqlite remove endpoint (KU-1719) #57

Merged

HomayoonAlimohammadi added a commit that referenced this pull request Oct 14, 2024

Add Dqlite remove endpoint (KU-1719) (#57)

83f63f4

* Add /v2/dqlite/remove endpoint (#55) * validate capi-auth-token on dqlite/remove (#56)

HomayoonAlimohammadi added a commit that referenced this pull request Oct 16, 2024

Add Dqlite remove endpoint (KU-1719) (#57)

6421bc8

* Add /v2/dqlite/remove endpoint (#55) * validate capi-auth-token on dqlite/remove (#56)

HomayoonAlimohammadi added a commit that referenced this pull request Oct 16, 2024

Add Dqlite remove endpoint (KU-1719) (#57)

00bcdb1

* Add /v2/dqlite/remove endpoint (#55) * validate capi-auth-token on dqlite/remove (#56)

HomayoonAlimohammadi added a commit that referenced this pull request Oct 16, 2024

Add Dqlite remove endpoint (KU-1719) (#57)

1ce0520

* Add /v2/dqlite/remove endpoint (#55) * validate capi-auth-token on dqlite/remove (#56)

HomayoonAlimohammadi added a commit that referenced this pull request Oct 16, 2024

Add Dqlite remove endpoint (KU-1719) (#57)

66eec3a

* Add /v2/dqlite/remove endpoint (#55) * validate capi-auth-token on dqlite/remove (#56)

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

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

Uh oh!

Conversation

HomayoonAlimohammadi commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Alternative solution

PR series

Uh oh!

berkayoz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bschimke95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bschimke95 Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

HomayoonAlimohammadi Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

bschimke95 Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

HomayoonAlimohammadi Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

HomayoonAlimohammadi Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HomayoonAlimohammadi Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

bschimke95 Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

bschimke95 left a comment

Choose a reason for hiding this comment

Uh oh!

bschimke95 Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

berkayoz left a comment

Choose a reason for hiding this comment

Uh oh!

HomayoonAlimohammadi commented Oct 11, 2024

Uh oh!

HomayoonAlimohammadi commented Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Note for future travelers

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HomayoonAlimohammadi commented Oct 9, 2024 •

edited

Loading

HomayoonAlimohammadi Oct 11, 2024 •

edited

Loading

HomayoonAlimohammadi commented Oct 11, 2024 •

edited

Loading