vtgate : Disable Automatically setting immediateCallerID to user from static authentication context

[Phanatic]

Description

Fixes #12970

This PR adds a new flag grpc-use-static-authentication-callerid to gate the behavior introduced in #12050

While I reviewed this PR, I didn't catch the issue where the username from static authentication context would completely override the immediate callerID for all Execute calls to vtgate.

PlanetScale's usage of vtgate static auth and ACLs system is a bit unique in that we don't handle authentication for user queries in vtgate. We do that in our own query front-end service.
VTGate static authentication is configured for service-to-service authentication between the query front-end and vtgate, ACL system is configured to pass through user roles from our credential store to vtgate.

We do this by setting the effectiveCallerID on requests made to the vtgate gRPC service and having the same names reflected in the acl config file for a given vttablet.

With the change in the referenced PR, all ACL checks for a database will fail since it will use the static authentication username, and not the effectiveCallerID from the ExecuteRequest call.

The precedence of assigning the immediate Caller ID is now :

The client cert common name (if using mTLS)
The effective caller id (if --grpc_use_effective_callerid=true)
The static auth username (if --grpc-use-static-authentication-callerid=true)

Checklist

• "Backport to:" labels have been added if this change should be back-ported
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on the CI
• Documentation was added or is not required

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.
• If a test is added or modified, there should be a documentation on top of the test to explain what the expected behavior is what the test does.

If a new flag is being introduced:

• Is it really necessary to add this flag?
• Flag names should be clear and intuitive (as far as possible)
• Help text should be descriptive.
• Flag names should use dashes (-) as word separators rather than underscores (_).

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow should be required, the maintainer team should be notified.

Bug fixes

• There should be at least one unit or end-to-end test.
• The Pull Request description should include a link to an issue that describes the bug.

Non-trivial changes

• There should be some code comments as to why things are implemented the way they are.

New/Existing features

• Should be documented, either by modifying the existing documentation or creating new documentation.
• New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• vtctl command output order should be stable and awk-able.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from VTop, if used there.

[deepthi]

Seems like a reasonable way to handle the issue.
@Phanatic plans to create an issue and link it (as required for all bug reports).

In addition to that, we need

• to have all the CI checks fixed
• a PR to update website docs at https://github.com/vitessio/website/blob/prod/content/en/docs/17.0/reference/programs/vtgate.md No longer needed since there is no new flag

@frouioui when this is backported to release-16.0 do we need to add a summary release note for 16.0.2?

[harshit-gangal]

Changes looks fine.

I need to understand a little bit more about how it is used.

1. My understanding is that the static auth user is always set. So, when the new flag is enabled how it will start working like what changes so that it does not fail?
2. My assumption is that immediate Caller ID is not set in the cert for PS. Is that correct?

[dbussink]

It seems like this check is in the wrong place? Isn't it more a responsibility of the caller of immediateCallerID?

We already have the useEffective so shouldn't we make sure that if that value is set, we don't let immediate override it?

[dbussink]

I think if we go with an approach like the following, we can achieve the same result without needing another flag?

diff --git i/go/vt/vtgate/grpcvtgateservice/server.go w/go/vt/vtgate/grpcvtgateservice/server.go
index d012786d6e..a819730cb1 100644
--- i/go/vt/vtgate/grpcvtgateservice/server.go
+++ w/go/vt/vtgate/grpcvtgateservice/server.go
@@ -104,12 +104,15 @@ func immediateCallerID(ctx context.Context) (string, []string) {
 // withCallerIDContext creates a context that extracts what we need
 // from the incoming call and can be forwarded for use when talking to vttablet.
 func withCallerIDContext(ctx context.Context, effectiveCallerID *vtrpcpb.CallerID) context.Context {
-       immediate, securityGroups := immediateCallerID(ctx)
-       if immediate == "" && useEffective && effectiveCallerID != nil {
+       var immediate string
+       var securityGroups []string
+       if useEffective && effectiveCallerID != nil {
                immediate = effectiveCallerID.Principal
                if useEffectiveGroups && len(effectiveCallerID.Groups) > 0 {
                        securityGroups = effectiveCallerID.Groups
                }
+       } else {
+               immediate, securityGroups = immediateCallerID(ctx)
        }
        if immediate == "" {
                immediate = unsecureClient

[Phanatic]

fixed now, I just reverted the changes from the PR that introduced this behavior

[brendar]

The changes @dbussink is suggesting above make sense to me. If the intention is for effective caller id to take precedence over other credentials, then that could be done explicitly. Just reverting the changes in #12050 would mean that a client connecting to vtgate using mTLS would still have their immediate caller id set from the client cert rather than the effective caller id.

[harshit-gangal]

@brendar in your use case do you use mTLS and not want to use the client name from the cert?

[brendar]

No, we use TLS (but not mTLS) + static auth to connect to vtgates. The precedence order doesn't matter to us since we're also not using grpc_use_effective_callerid, but we'd like to be able to use grpc static auth usernames in table ACLs. That allows us to provide clients with a set of username/password credentials, and they can use those to connect to vtgate via mysql protocol or grpc as they choose.

To clarify what I mean by precedence order, before #12050 the immediate caller id would have been set from the first non-empty value from:

• The client cert common name (if using mTLS)
• The effective caller id (if --grpc_use_effective_callerid=true)

After #12050 the order would have been:

• The static auth username (if using static auth)
• The client cert common name (if using mTLS)
• The effective caller id (if --grpc_use_effective_callerid=true)

If the effective caller id should take precedence over other credentials, then perhaps the order should be something like this?

• The effective caller id (if --grpc_use_effective_callerid=true)
• The static auth username (if using static auth)
• The client cert common name (if using mTLS)

Looking at the comment for grpc_use_effective_callerid though

"If set, and SSL is not used, will set the immediate caller id from the effective caller id's principal."

If we wanted to preserve that behavior, then perhaps the order should be:

• The client cert common name (if using mTLS)
• The effective caller id (if --grpc_use_effective_callerid=true)
• The static auth username (if using static auth)

[harshit-gangal]

@brendar We should also put static auth username behind a flag something like mysql_use_static_auth_username

[harshit-gangal]

Also, I think the last order makes sense as it is the least disruptive change in the way it works before the change in #12050

[harshit-gangal]

After reading the old PR #12050 and its description.
We can basically use the effective caller id and enable grpc_use_effective_callerid. The reason you were seeing unsecure_grpc_client was that the connection created between your service and vtgate is non-ssl connection.

This also means the previous change can be reverted.

[harshit-gangal]

The changes look good.
This PR would need an updated PR title and description.

[Phanatic]

website PR is at vitessio/website#1454

[brendar]

This solution looks good to me. Thanks for the opportunity to provide feedback, and apologies for the undesirable behavior.

[frouioui]

@frouioui when this is backported to release-16.0 do we need to add a summary release note for 16.0.2?

@deepthi @Phanatic, yes I think. It changes the behavior significantly enough that it should be mentioned in the 16.0.2 release notes, but I think also in the 17.0.0 release notes.