Terser authentication logic in front-proxy

[ntnn]

Summary

That logic never sat right with me and it seems to cause a logic error.

The original error was fixed with #4125, which moved the front-proxy back to its old behaviour that relies on essentially a race for the test to function:

The root cause of the error was that an authenticated request came through but the authenticator wasn't yet ready, so the original code here:

kcp/pkg/proxy/filters/filters.go

Lines 49 to 59 in 5a9d2e2

	// If an authenticator for the workspace exists,
	// it offers an alternative way to authenticate.
	reqAuthenticator, ok := authentication.WorkspaceAuthenticatorFrom(req.Context())
	if ok {
	if auth != nil {
	authenticator = authenticatorunion.New(auth, reqAuthenticator)
	} else {
	authenticator = reqAuthenticator
	}
	additionalAuth = true
	}

Would return nil, false from here

kcp/pkg/authentication/filters.go

Lines 67 to 73 in 5a9d2e2

	func WorkspaceAuthenticatorFrom(ctx context.Context) (authenticator.Request, bool) {
	authenticator, ok := ctx.Value(authenticatorContextKey).(authenticator.Request)
	if !ok {
	return nil, false
	}
	return authenticator, true
	}

Because the handler that should add the workspace authenticator in the cotext silently fails here:

kcp/pkg/authentication/filters.go

Lines 41 to 44 in 5a9d2e2

	authn, ok := authIndex.Lookup(wsType)
	if !ok {
	handler.ServeHTTP(w, req)
	return

Because of the change that validated authenticators before returning them the authenticator was in the index later than the test assumed, turning this into a race.

Anyhow - this doesn't fix that issue. I just noticed this issue because of that.

This fixes another problem that currently isn't tested - if a request with a static token comes in for a workspace that has per-workspace authentication configured the request will get denied.

I skipped writing a test for this because this will be fixed as part of #4100

What Type of PR Is This?

/kind bug

Related Issue(s)

Fixes #

Release Notes

NONE

[kcp-ci-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign embik for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mjudeikis]

Will be suprised if this will not break anything.

[ntnn]

This whole thing is a mess.

Basically the front-proxy tries to authenticate a request but just lets it through if it doesn't succeed unless the workspace has per-workspace auth configured.
Then it actually fails.

This causes a problem in the tests because there the shards have fixed identities with tokens (from the csv file) that the front-proxy doesn't know about. This only works because the additionalAuth boolean is false for the front-proxy and is only set for per-workspace auth explicitly. And this in turn doesn't show up in the tests because the per-worspace auth tests only use per-workspace auth and the tests using static tokens don't.

This also hides a bug if a static token is used to access a worksapce with per-workspace auth - this will get declined on the front-proxy simply because per-workspace auth does not recognize the static token.