feat(kuma-cp) User Token for API Server authentication #2892

jakubdyszkiewicz · 2021-10-05T10:55:42Z

Summary

This PR introduces a new authentication mechanism in Kuma called User Token.
User Token is a JWT token that contains name and group.

Why?
Currently, our authentication mechanism is based on client certificates. We want to ditch it because

Managing client certs is painful. You need to pass them as a file to every instance of cp.
There is no notion of user. If you own client cert, you are admin
It works only with E2E TLS connections. Header-based auth is easier to pass through load balancers / reverse proxies.
User Token can be used for out of process integration with internal auth mechanism. For example, some system can authenticate user using whatever mechanism there is in place and then exchange this information for User Token with a short expiration date.

It is very similar to Dataplane Token that we use already, meaning there are similar endpoint and kumactl cli commands to generate a token.

Additionally, this token supports

Signing Key rotation
Token revocation list
Expiration

Those features will be carried to other tokens in a system for consistency.

In this PR, User Token auth is opt-in feature. I'll change it as a default and deprecate the current mechanism.

Implementation

It was implemented as a plugin. Once we try to carry rotation, revocation, and expiration to other tokens we can try to DRY the logic of Token Issuer.

TODO

In the next PR, I want to provide changes to kumactl to support configuring control plane with user token. Right now it's possible through --headers 'authorization=Bearer XYZ'

Documentation

I'd write docs after implementing bootstraping strategy of admin.

Testing

Unit tests
E2E tests
Manual testing on Universal
Manual testing on Kubernetes

Backwards compatibility

We don't want to backport it.

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>

lobkovilya · 2021-10-06T10:03:06Z

app/kumactl/cmd/install/testdata/install-metrics.defaults.golden.yaml

@@ -12269,4 +12269,4 @@ spec:
            name: prometheus-server
        - name: storage-volume
          persistentVolumeClaim:
-            claimName: prometheus-server
+            claimName: prometheus-server


hmm, why the last line disappeared from these 3 files?

I'm not sure. I regenerated everything using UPDATE_GOLDEN_FILES

I get the same if I update just on master:

app/kumactl/cmd/install/testdata/install-gateway-enterprise.defaults.golden.yaml | 2 +- app/kumactl/cmd/install/testdata/install-gateway-enterprise.overrides.golden.yaml | 2 +- app/kumactl/cmd/install/testdata/install-gateway.defaults.golden.yaml | 2 +- app/kumactl/cmd/install/testdata/install-gateway.overrides.golden.yaml | 2 +- app/kumactl/cmd/install/testdata/install-metrics.defaults.golden.yaml | 2 +- app/kumactl/cmd/install/testdata/install-metrics.no-grafana.golden.yaml | 2 +- app/kumactl/cmd/install/testdata/install-metrics.no-prometheus.golden.yaml | 2 +- app/kumactl/cmd/install/testdata/install-metrics.overrides.golden.yaml | 2 +-

bartsmykla

lgtm

pkg/plugins/authn/api-server/tokens/ws/client/client.go

jpeach

I ran out of time on this review, planning to finish up tomorrow.

pkg/plugins/authn/api-server/tokens/ws/client/client.go

jpeach · 2021-10-07T05:55:26Z

pkg/plugins/authn/api-server/tokens/issuer/signing_key.go

+	DefaultSerialNumber = 1
+)
+
+var SigningKeyNotFound = errors.New("there is no Signing Key in the Control Plane.")


Nit, no full-stop in the error string

app/kumactl/cmd/generate/generate_signing_key.go

app/kumactl/cmd/generate/context/context.go

app/kumactl/cmd/generate/generate_signing_key.go

pkg/plugins/authn/api-server/tokens/cli/generate_user_token.go

jpeach · 2021-10-07T06:06:02Z

pkg/plugins/authn/api-server/tokens/cli/generate_user_token.go

+	cmd.Flags().StringVar(&args.name, "name", "", "name of the user")
+	_ = cmd.MarkFlagRequired("name")
+	cmd.Flags().StringVar(&args.group, "group", "", "group of the user")
+	cmd.Flags().DurationVar(&args.validFor, "valid-for", 0, `how long the token will be valid (for example "24h"). If 0, then token has no expiration time`)


"never" seems like a really long default ... IMHO better to make a short default so people have to either rotate or make a decision to make it long

I think short implicit default is just asking for confusion. If we don't want to do infinity as default (which I get that it is not ideal), we should make this argument required and let the user decide always. WDYT?

I think short implicit default is just asking for confusion. If we don't want to do infinity as default (which I get that it is not ideal), we should make this argument required and let the user decide always. WDYT?

Yeh, I can live with making it required. Let's also take an issue to support "infinity" to me an forever, rather than overloading "0" for that.

pkg/plugins/authn/api-server/tokens/cli/generate_user_token.go

jpeach · 2021-10-07T06:11:21Z

Something's up with codecov here?

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>

codecov-commenter · 2021-10-07T11:30:23Z

Codecov Report

Merging #2892 (0cdf00b) into master (08f5c6e) will increase coverage by 0.10%.
The diff coverage is 58.95%.

@@            Coverage Diff             @@
##           master    #2892      +/-   ##
==========================================
+ Coverage   52.32%   52.43%   +0.10%     
==========================================
  Files         900      912      +12     
  Lines       52296    52688     +392     
==========================================
+ Hits        27362    27625     +263     
- Misses      22756    22857     +101     
- Partials     2178     2206      +28

Impacted Files	Coverage Δ
app/kumactl/pkg/cmd/root_context.go	`63.91% <ø> (+4.12%)`	⬆️
...g/plugins/authn/api-server/tokens/authenticator.go	`0.00% <0.00%> (ø)`
test/e2e/auth/auth_universal.go	`0.00% <0.00%> (ø)`
test/framework/interface.go	`0.00% <ø> (ø)`
test/framework/k8s_controlplane.go	`0.00% <0.00%> (ø)`
test/framework/kumactl.go	`0.00% <0.00%> (ø)`
test/framework/universal_cluster.go	`0.00% <0.00%> (ø)`
test/framework/universal_controlplane.go	`0.00% <0.00%> (ø)`
pkg/core/rbac/static_role.go	`87.50% <50.00%> (-12.50%)`	⬇️
...ns/authn/api-server/tokens/ws/server/webservice.go	`65.85% <65.85%> (ø)`
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08f5c6e...0cdf00b. Read the comment docs.

app/kumactl/cmd/generate/generate_signing_key.go

pkg/plugins/authn/api-server/tokens/cli/cli_suite_test.go

pkg/plugins/authn/api-server/tokens/cli/generate_user_token.go

pkg/plugins/authn/api-server/tokens/issuer/default_signing_key.go

pkg/plugins/authn/api-server/tokens/issuer/signing_key.go

pkg/tokens/builtin/issuer/signing_key.go

test/e2e/auth/auth_universal.go

test/framework/kumactl.go

test/framework/universal_cluster.go

jpeach · 2021-10-08T01:40:43Z

pkg/plugins/authn/api-server/tokens/issuer/issuer.go

+			ID:        core.NewUUID(),
+			IssuedAt:  jwt.NewNumericDate(now),
+			NotBefore: jwt.NewNumericDate(now.Add(time.Minute * -5)), // todo(jakubdyszkiewicz) parametrize via config and go through all clock skews in the project
+		},


Does it make sense to set the iss and aud claims here?

what would you put in both iss and aud in this case?

I was considering putting name into aud but then what about groups? should this be in payload as a separate field like we do right now?
What about iss? Should this be just Kuma User Token? Or should this contain username of user that issued the token?

The issuer would be the control plane that minted the token and the audience would be any control plane for that cluster. Larger question is whether those are actually concrete concepts that are useful here :)

jpeach · 2021-10-08T01:41:49Z

pkg/plugins/authn/api-server/tokens/issuer/issuer.go

+
+	if validFor != 0 {
+		c.RegisteredClaims.ExpiresAt = jwt.NewNumericDate(now.Add(validFor))
+	}


WDYT about a tracking issue for config to set default and maximum ticket lifetimes?

I changed this to be required always and 0 do not mean infinite

pkg/plugins/authn/api-server/tokens/issuer/issuer.go

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>

jakubdyszkiewicz added 8 commits September 30, 2021 16:24

feat(*) introduce auth on API using user token

a700c48

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>

e2e test

4d58e69

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>

make check

3d3c335

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>

client certs as defualt

678c471

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>

Merge remote-tracking branch 'origin/master' into user-token

420e8c5

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>

auth token

c3cec9d

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>

Merge remote-tracking branch 'origin/master' into user-token

56be51f

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>

conditional e2e test

6040b3c

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>

jakubdyszkiewicz requested a review from a team as a code owner October 5, 2021 10:55

lobkovilya approved these changes Oct 6, 2021

View reviewed changes

michaelbeaumont mentioned this pull request Oct 6, 2021

fix(tests) ensure newline at end of golden files #2907

Merged

bartsmykla approved these changes Oct 6, 2021

View reviewed changes

pkg/plugins/authn/api-server/tokens/ws/client/client.go Outdated Show resolved Hide resolved

jpeach suggested changes Oct 7, 2021

View reviewed changes

jakubdyszkiewicz mentioned this pull request Oct 7, 2021

Consolidate HTTP client usage #2919

Closed

jakubdyszkiewicz added 2 commits October 7, 2021 12:07

review remarks

caae183

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>

support multiple groups

0cdf00b

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>

jpeach approved these changes Oct 8, 2021

View reviewed changes

jpeach reviewed Oct 8, 2021

View reviewed changes

pkg/plugins/authn/api-server/tokens/issuer/issuer.go Outdated Show resolved Hide resolved

jpeach reviewed Oct 8, 2021

View reviewed changes

pkg/plugins/authn/api-server/tokens/issuer/issuer.go Show resolved Hide resolved

jpeach reviewed Oct 8, 2021

View reviewed changes

pkg/plugins/authn/api-server/tokens/issuer/issuer.go Outdated Show resolved Hide resolved

jakubdyszkiewicz added 2 commits October 8, 2021 13:32

review remarks

f3fb7ed

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>

explicit expiration date

430515a

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>

jakubdyszkiewicz force-pushed the user-token branch from 7b1aee2 to 430515a Compare October 8, 2021 12:36

fix test

9aaa89b

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>

jakubdyszkiewicz merged commit d373354 into master Oct 8, 2021

jakubdyszkiewicz deleted the user-token branch October 8, 2021 13:49

jpeach mentioned this pull request Oct 13, 2021

chore(ci) split e2e deployment options (backport #2950) #2952

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(kuma-cp) User Token for API Server authentication #2892

feat(kuma-cp) User Token for API Server authentication #2892

jakubdyszkiewicz commented Oct 5, 2021

lobkovilya Oct 6, 2021

jakubdyszkiewicz Oct 6, 2021

michaelbeaumont Oct 6, 2021

bartsmykla left a comment

jpeach left a comment

jpeach Oct 7, 2021

jpeach Oct 7, 2021

jakubdyszkiewicz Oct 7, 2021

jpeach Oct 7, 2021

jpeach commented Oct 7, 2021

codecov-commenter commented Oct 7, 2021

jpeach Oct 8, 2021

jakubdyszkiewicz Oct 8, 2021

jpeach Oct 8, 2021

jpeach Oct 8, 2021

jakubdyszkiewicz Oct 8, 2021

feat(kuma-cp) User Token for API Server authentication #2892

feat(kuma-cp) User Token for API Server authentication #2892

Conversation

jakubdyszkiewicz commented Oct 5, 2021

Summary

Implementation

TODO

Documentation

Testing

Backwards compatibility

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartsmykla left a comment

Choose a reason for hiding this comment

jpeach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpeach commented Oct 7, 2021

codecov-commenter commented Oct 7, 2021

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment