fix(kuma-dp): tolerate endline in token file

[lahabana]

This is overly strict validation is causing more problems than it is solving. We now simply trim the token data

Fix #4567

Signed-off-by: Charly Molter [email protected]

Checklist prior to review

• Link to docs PR or issue --
• Link to UI issue or PR --
• Is the issue worked on linked? --
• The PR does not hardcode values that might break projects that depend on kuma (e.g. "kumahq" as a image registry) --
• The PR will work for both Linux and Windows, system specific functions like syscall.Mkfifo have equivalent implementation on the other OS --
• Unit Tests --
• E2E Tests --
• Manual Universal Tests --
• Manual Kubernetes Tests --
• Do you need to update UPGRADE.md? --
• Does it need to be backported according to the backporting policy? --
• Do you need to explicitly set a > Changelog: entry here or add a ci/ label to run fewer/more tests?

[jakubdyszkiewicz]

The problem was that Envoy is very strict about this token format and it's failing with "cryptic" message. That's why we were trying to be strict in kuma-dp to instruct user how to solve this.

Did you see a case where this change helps?

ok, not sure about Envoy behavior here

[lahabana]

Did you see a case where this change helps?
We now trim the token so what Envoy gets is the same. The reason why I'm changing this is folks complaining very often that they can't copy paste a token (likely their editor is configured with auto endline). This happened to @slonka on his on-boarding tasks and I've seen it a few times with customers too. @svenwal also mentioned it to me.

So to me it seems that the overly restrictive policy is causing more trouble than not having it and trimming seems to solve the most common problem

[jakubdyszkiewicz]

Let me try to rephrase my question.

Did you see a case where this validation was too restrictive? Meaning, if kuma-dp validation was gone, Envoy would have no problem with the token?
I just don't want to end up in the situation we were in before when Envoy was producing cryptic messages that the token is invalid without any hint of action. That was more troubling for users.

What I think may happened in the meantime before we created this validation and now is that by default we switched from Google gRPC to Envoy gRPC. I think that because we are embedding the token in metadata, all invalid characters are gone. I think it was Google gRPC that was the most restrictive part. However, we still let user use Google gRPC when kuma_dp_server_auth_use_token_path is true to support rotation (projected SATs)

[slonka]

I just don't want to end up in the situation we were in before when Envoy was producing cryptic messages that the token is invalid without any hint of action.

I totally agree with Jakub here.

Meaning, if kuma-dp validation was gone, Envoy would have no problem with the token?

Maybe we could have some test with Envoy and "mode=validate" to see if lifting this validation would cause problems with Envoy?

[slonka]

As said in the thread, I'm ok with approving this if there is a test case showing that lifting the newline check at the end does not cause any trouble.

[lahabana]

I'll check again but from my local tests the endline wasn't a problem for envoy

[lahabana]

I've checked removing the validation the error is only on the CP and shows:

 "error": "Envoy bootstrap config is not valid: invalid Bootstrap.DynamicResources: embedded message failed validation | caused by: invalid Bootstrap_DynamicResources.AdsConfig: embedded message failed validation | caused by: invalid ApiConfigSource.GrpcServices[0]: embedded message failed validation | caused by: invalid GrpcService.InitialMetadata[0]: embedded message failed validation | caused by: invalid HeaderValue.Value: value does not match regex pattern \"^[^\\x00\\n\\r]*$\"", "errorVerbose": "invalid Bootstrap.DynamicResources: embedded message failed validation | caused by: invalid Bootstrap_DynamicResources.AdsConfig: embedded message failed validation | caused by: invalid ApiConfigSource.GrpcServices[0]: embedded message failed validation | caused by: invalid GrpcService.InitialMetadata[0]: embedded message failed validation | caused by: invalid HeaderValue.Value: value does not match regex pattern \"^[^\\x00\\n\\r]*$\"\nEnvoy bootstrap config is not valid\ngithub.com/kumahq/kuma/pkg/xds/bootstrap.(*bootstrapGenerator).Generate\n\t/Users/[email protected]/code/kuma/pkg/xds/bootstrap/generator.go:170\ngithub.com/kumahq/kuma/pkg/xds/bootstrap.(*BootstrapHandler).Handle\n\t/Users/[email protected]/code/kuma/pkg/xds/bootstrap/handler.go:52\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2084\nnet/http.(*ServeMux).ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2462\ngithub.com/slok/go-http-metrics/middleware/std.Handler.func1.1\n\t/Users/[email protected]/go/pkg/mod/github.com/slok/[email protected]/middleware/std/std.go:27\ngithub.com/slok/go-http-metrics/middleware.Middleware.Measure\n\t/Users/[email protected]/go/pkg/mod/github.com/slok/[email protected]/middleware/middleware.go:117\ngithub.com/slok/go-http-metrics/middleware/std.Handler.func1\n\t/Users/[email protected]/go/pkg/mod/github.com/slok/[email protected]/middleware/std/std.go:26\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2084\ngithub.com/kumahq/kuma/pkg/dp-server/server.(*DpServer).handle\n\t/Users/[email protected]/code/kuma/pkg/dp-server/server/server.go:122\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2084\nnet/http.serverHandler.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2916\nnet/http.(*conn).serve\n\t/usr/local/go/src/net/http/server.go:1966\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_arm64.s:1270"

What's shown here is really that the token is validated as an header. So I've added the same regex and I also trim the token for validation and when we send it that way people can still have endlines in their token file.

[slonka]

I've checked removing the validation the error is only on the CP

If it's only in CP then 👍 from my side.