From b10040780e0bb99bb5c8bd80b6122af5abdc1b6f Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Mon, 13 Sep 2021 19:12:35 +0200 Subject: [PATCH 1/5] Slight improvement of the ACL error message Signed-off-by: Florent Poinsard --- .../encrypted_transport_test.go | 12 ++++++------ .../endtoend/mysqlserver/mysql_server_test.go | 4 ++-- go/vt/vttablet/tabletserver/query_executor.go | 2 +- .../vttablet/tabletserver/query_executor_test.go | 16 ++++------------ 4 files changed, 13 insertions(+), 21 deletions(-) diff --git a/go/test/endtoend/encryption/encryptedtransport/encrypted_transport_test.go b/go/test/endtoend/encryption/encryptedtransport/encrypted_transport_test.go index c32d1605b43..f8155079a27 100644 --- a/go/test/endtoend/encryption/encryptedtransport/encrypted_transport_test.go +++ b/go/test/endtoend/encryption/encryptedtransport/encrypted_transport_test.go @@ -189,8 +189,8 @@ func TestSecureTransport(t *testing.T) { require.NoError(t, err) err = vterrors.FromVTRPC(qr.Error) require.Error(t, err) - assert.Contains(t, err.Error(), "table acl error") - assert.Contains(t, err.Error(), "cannot run Select on table") + assert.Contains(t, err.Error(), "authorization error:") + assert.Contains(t, err.Error(), "is not authorized to run Select on table") // now restart vtgate in the mode where we don't use SSL // for client connections, but we copy effective caller id @@ -214,8 +214,8 @@ func TestSecureTransport(t *testing.T) { require.NoError(t, err) err = vterrors.FromVTRPC(qr.Error) require.Error(t, err) - assert.Contains(t, err.Error(), "table acl error") - assert.Contains(t, err.Error(), "cannot run Select on table") + assert.Contains(t, err.Error(), "authorization error:") + assert.Contains(t, err.Error(), "is not authorized to run Select on table") // 'vtgate client 1' is authorized to access vt_insert_test callerID := &vtrpc.CallerID{ @@ -236,8 +236,8 @@ func TestSecureTransport(t *testing.T) { require.NoError(t, err) err = vterrors.FromVTRPC(qr.Error) require.Error(t, err) - assert.Contains(t, err.Error(), "table acl error") - assert.Contains(t, err.Error(), "cannot run Select on table") + assert.Contains(t, err.Error(), "authorization error:") + assert.Contains(t, err.Error(), "is not authorized to run Select on table") clusterInstance.Teardown() } diff --git a/go/test/endtoend/mysqlserver/mysql_server_test.go b/go/test/endtoend/mysqlserver/mysql_server_test.go index 9fb6344778b..2951a24b842 100644 --- a/go/test/endtoend/mysqlserver/mysql_server_test.go +++ b/go/test/endtoend/mysqlserver/mysql_server_test.go @@ -193,8 +193,8 @@ func TestSelectWithUnauthorizedUser(t *testing.T) { _, err = conn.ExecuteFetch("SELECT * from vt_insert_test limit 1", 1, false) require.NotNilf(t, err, "error expected, got nil") - assert.Contains(t, err.Error(), "table acl error") - assert.Contains(t, err.Error(), "cannot run Select on table") + assert.Contains(t, err.Error(), "authorization error:") + assert.Contains(t, err.Error(), "is not authorized to run Select on table") } func connectDB(t *testing.T, vtParams mysql.ConnParams, params ...string) *sql.DB { diff --git a/go/vt/vttablet/tabletserver/query_executor.go b/go/vt/vttablet/tabletserver/query_executor.go index d63e194fedf..50cc296da61 100644 --- a/go/vt/vttablet/tabletserver/query_executor.go +++ b/go/vt/vttablet/tabletserver/query_executor.go @@ -444,7 +444,7 @@ func (qre *QueryExecutor) checkAccess(authorized *tableacl.ACLResult, tableName } if qre.tsv.qe.strictTableACL { - errStr := fmt.Sprintf("table acl error: %q %v cannot run %v on table %q", callerID.Username, callerID.Groups, qre.plan.PlanID, tableName) + errStr := fmt.Sprintf("authorization error: user %q is not authorized to run %v on table %q", callerID.Username, qre.plan.PlanID, tableName) qre.tsv.Stats().TableaclDenied.Add(statsKey, 1) qre.tsv.qe.accessCheckerLogger.Infof("%s", errStr) return vterrors.Errorf(vtrpcpb.Code_PERMISSION_DENIED, "%s", errStr) diff --git a/go/vt/vttablet/tabletserver/query_executor_test.go b/go/vt/vttablet/tabletserver/query_executor_test.go index b6aef00d2ae..99f498fc73c 100644 --- a/go/vt/vttablet/tabletserver/query_executor_test.go +++ b/go/vt/vttablet/tabletserver/query_executor_test.go @@ -734,10 +734,7 @@ func TestQueryExecutorMessageStreamACL(t *testing.T) { return io.EOF }) - want := `table acl error: "u2" [] cannot run MessageStream on table "msg"` - if err == nil || err.Error() != want { - t.Errorf("qre.MessageStream(msg) error: %v, want %s", err, want) - } + assert.EqualError(t, err, `authorization error: user "u2" is not authorized to run MessageStream on table "msg"`) if code := vterrors.Code(err); code != vtrpcpb.Code_PERMISSION_DENIED { t.Fatalf("qre.Execute: %v, want %v", code, vtrpcpb.Code_PERMISSION_DENIED) } @@ -877,10 +874,8 @@ func TestQueryExecutorTableAclDualTableExempt(t *testing.T) { if code := vterrors.Code(err); code != vtrpcpb.Code_PERMISSION_DENIED { t.Fatalf("qre.Execute: %v, want %v", code, vtrpcpb.Code_PERMISSION_DENIED) } - wanterr := "table acl error" - if !strings.Contains(err.Error(), wanterr) { - t.Fatalf("qre.Execute: %v, want %s", err, wanterr) - } + + assert.EqualError(t, err, `authorization error: user "basic_username" is not authorized to run SelectImpossible on table "test_table"`) // table acl should be ignored when querying against dual table query = "select @@version_comment from dual limit 1" @@ -944,10 +939,7 @@ func TestQueryExecutorTableAclExemptACL(t *testing.T) { if code := vterrors.Code(err); code != vtrpcpb.Code_PERMISSION_DENIED { t.Fatalf("qre.Execute: %v, want %v", code, vtrpcpb.Code_PERMISSION_DENIED) } - wanterr := "table acl error" - if !strings.Contains(err.Error(), wanterr) { - t.Fatalf("qre.Execute: %v, want %s", err, wanterr) - } + assert.EqualError(t, err, `authorization error: user "u2" is not authorized to run Select on table "test_table"`) // table acl should be ignored since this is an exempt user. username = "exempt-acl" From 498f1dda0b1273867e29ae6dd2d7e229e6fb8375 Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Mon, 13 Sep 2021 20:50:15 +0200 Subject: [PATCH 2/5] Mimick MySQL ACL errors Signed-off-by: Florent Poinsard --- .../encryptedtransport/encrypted_transport_test.go | 12 ++++++------ go/test/endtoend/mysqlserver/mysql_server_test.go | 4 ++-- go/vt/vttablet/tabletserver/query_executor.go | 2 +- go/vt/vttablet/tabletserver/query_executor_test.go | 6 +++--- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/go/test/endtoend/encryption/encryptedtransport/encrypted_transport_test.go b/go/test/endtoend/encryption/encryptedtransport/encrypted_transport_test.go index f8155079a27..75b50d3e7cc 100644 --- a/go/test/endtoend/encryption/encryptedtransport/encrypted_transport_test.go +++ b/go/test/endtoend/encryption/encryptedtransport/encrypted_transport_test.go @@ -189,8 +189,8 @@ func TestSecureTransport(t *testing.T) { require.NoError(t, err) err = vterrors.FromVTRPC(qr.Error) require.Error(t, err) - assert.Contains(t, err.Error(), "authorization error:") - assert.Contains(t, err.Error(), "is not authorized to run Select on table") + assert.Contains(t, err.Error(), "Select command denied to user") + assert.Contains(t, err.Error(), "for table 'vt_insert_test' (ACL check error)") // now restart vtgate in the mode where we don't use SSL // for client connections, but we copy effective caller id @@ -214,8 +214,8 @@ func TestSecureTransport(t *testing.T) { require.NoError(t, err) err = vterrors.FromVTRPC(qr.Error) require.Error(t, err) - assert.Contains(t, err.Error(), "authorization error:") - assert.Contains(t, err.Error(), "is not authorized to run Select on table") + assert.Contains(t, err.Error(), "Select command denied to user") + assert.Contains(t, err.Error(), "for table 'vt_insert_test' (ACL check error)") // 'vtgate client 1' is authorized to access vt_insert_test callerID := &vtrpc.CallerID{ @@ -236,8 +236,8 @@ func TestSecureTransport(t *testing.T) { require.NoError(t, err) err = vterrors.FromVTRPC(qr.Error) require.Error(t, err) - assert.Contains(t, err.Error(), "authorization error:") - assert.Contains(t, err.Error(), "is not authorized to run Select on table") + assert.Contains(t, err.Error(), "Select command denied to user") + assert.Contains(t, err.Error(), "for table 'vt_insert_test' (ACL check error)") clusterInstance.Teardown() } diff --git a/go/test/endtoend/mysqlserver/mysql_server_test.go b/go/test/endtoend/mysqlserver/mysql_server_test.go index 2951a24b842..692b0e7da43 100644 --- a/go/test/endtoend/mysqlserver/mysql_server_test.go +++ b/go/test/endtoend/mysqlserver/mysql_server_test.go @@ -193,8 +193,8 @@ func TestSelectWithUnauthorizedUser(t *testing.T) { _, err = conn.ExecuteFetch("SELECT * from vt_insert_test limit 1", 1, false) require.NotNilf(t, err, "error expected, got nil") - assert.Contains(t, err.Error(), "authorization error:") - assert.Contains(t, err.Error(), "is not authorized to run Select on table") + assert.Contains(t, err.Error(), "Select command denied to user") + assert.Contains(t, err.Error(), "for table 'vt_insert_test' (ACL check error)") } func connectDB(t *testing.T, vtParams mysql.ConnParams, params ...string) *sql.DB { diff --git a/go/vt/vttablet/tabletserver/query_executor.go b/go/vt/vttablet/tabletserver/query_executor.go index 50cc296da61..ff3cd72a6f8 100644 --- a/go/vt/vttablet/tabletserver/query_executor.go +++ b/go/vt/vttablet/tabletserver/query_executor.go @@ -444,7 +444,7 @@ func (qre *QueryExecutor) checkAccess(authorized *tableacl.ACLResult, tableName } if qre.tsv.qe.strictTableACL { - errStr := fmt.Sprintf("authorization error: user %q is not authorized to run %v on table %q", callerID.Username, qre.plan.PlanID, tableName) + errStr := fmt.Sprintf("%v command denied to user '%s' for table '%s' (ACL check error)", qre.plan.PlanID.String(), callerID.Username, tableName) qre.tsv.Stats().TableaclDenied.Add(statsKey, 1) qre.tsv.qe.accessCheckerLogger.Infof("%s", errStr) return vterrors.Errorf(vtrpcpb.Code_PERMISSION_DENIED, "%s", errStr) diff --git a/go/vt/vttablet/tabletserver/query_executor_test.go b/go/vt/vttablet/tabletserver/query_executor_test.go index 99f498fc73c..356d625c41e 100644 --- a/go/vt/vttablet/tabletserver/query_executor_test.go +++ b/go/vt/vttablet/tabletserver/query_executor_test.go @@ -734,7 +734,7 @@ func TestQueryExecutorMessageStreamACL(t *testing.T) { return io.EOF }) - assert.EqualError(t, err, `authorization error: user "u2" is not authorized to run MessageStream on table "msg"`) + assert.EqualError(t, err, `MessageStream command denied to user 'u2' for table 'msg' (ACL check error)`) if code := vterrors.Code(err); code != vtrpcpb.Code_PERMISSION_DENIED { t.Fatalf("qre.Execute: %v, want %v", code, vtrpcpb.Code_PERMISSION_DENIED) } @@ -875,7 +875,7 @@ func TestQueryExecutorTableAclDualTableExempt(t *testing.T) { t.Fatalf("qre.Execute: %v, want %v", code, vtrpcpb.Code_PERMISSION_DENIED) } - assert.EqualError(t, err, `authorization error: user "basic_username" is not authorized to run SelectImpossible on table "test_table"`) + assert.EqualError(t, err, `SelectImpossible command denied to user 'basic_username' for table 'test_table' (ACL check error)`) // table acl should be ignored when querying against dual table query = "select @@version_comment from dual limit 1" @@ -939,7 +939,7 @@ func TestQueryExecutorTableAclExemptACL(t *testing.T) { if code := vterrors.Code(err); code != vtrpcpb.Code_PERMISSION_DENIED { t.Fatalf("qre.Execute: %v, want %v", code, vtrpcpb.Code_PERMISSION_DENIED) } - assert.EqualError(t, err, `authorization error: user "u2" is not authorized to run Select on table "test_table"`) + assert.EqualError(t, err, `Select command denied to user 'u2' for table 'test_table' (ACL check error)`) // table acl should be ignored since this is an exempt user. username = "exempt-acl" From 9a84551391950facec05cc007e97fafd688d4f7c Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Tue, 14 Sep 2021 12:57:58 +0200 Subject: [PATCH 3/5] Changed format flag for ACL check errors Signed-off-by: Florent Poinsard --- go/vt/vttablet/tabletserver/query_executor.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/vt/vttablet/tabletserver/query_executor.go b/go/vt/vttablet/tabletserver/query_executor.go index ff3cd72a6f8..98b913d1897 100644 --- a/go/vt/vttablet/tabletserver/query_executor.go +++ b/go/vt/vttablet/tabletserver/query_executor.go @@ -444,7 +444,7 @@ func (qre *QueryExecutor) checkAccess(authorized *tableacl.ACLResult, tableName } if qre.tsv.qe.strictTableACL { - errStr := fmt.Sprintf("%v command denied to user '%s' for table '%s' (ACL check error)", qre.plan.PlanID.String(), callerID.Username, tableName) + errStr := fmt.Sprintf("%s command denied to user '%s' for table '%s' (ACL check error)", qre.plan.PlanID.String(), callerID.Username, tableName) qre.tsv.Stats().TableaclDenied.Add(statsKey, 1) qre.tsv.qe.accessCheckerLogger.Infof("%s", errStr) return vterrors.Errorf(vtrpcpb.Code_PERMISSION_DENIED, "%s", errStr) From 84bea3c4dc9fc4829e864fbfa0a3eed18684e42e Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Mon, 20 Sep 2021 13:58:35 +0200 Subject: [PATCH 4/5] Updated TestTableACL E2E test expected error Signed-off-by: Florent Poinsard --- go/vt/vttablet/endtoend/acl_test.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/go/vt/vttablet/endtoend/acl_test.go b/go/vt/vttablet/endtoend/acl_test.go index f86229fea5f..0894c2838d0 100644 --- a/go/vt/vttablet/endtoend/acl_test.go +++ b/go/vt/vttablet/endtoend/acl_test.go @@ -19,9 +19,10 @@ package endtoend import ( "bytes" "encoding/json" - "strings" "testing" + "gotest.tools/assert" + "vitess.io/vitess/go/sqltypes" "vitess.io/vitess/go/vt/vttablet/endtoend/framework" "vitess.io/vitess/go/vt/vttablet/tabletserver/rules" @@ -32,7 +33,7 @@ import ( func TestTableACL(t *testing.T) { client := framework.NewClient() - aclErr := "table acl error" + aclErr := "command denied to user 'dev' for table" execCases := []struct { query string err string @@ -101,9 +102,7 @@ func TestTableACL(t *testing.T) { } continue } - if err == nil || !strings.HasPrefix(err.Error(), tcase.err) { - t.Errorf("Execute(%s): Error: %v, must start with %s", tcase.query, err, tcase.err) - } + assert.ErrorContains(t, err, tcase.err) } streamCases := []struct { @@ -131,9 +130,7 @@ func TestTableACL(t *testing.T) { } continue } - if err == nil || !strings.HasPrefix(err.Error(), tcase.err) { - t.Errorf("Error: %v, must start with %s", err, tcase.err) - } + assert.ErrorContains(t, err, tcase.err) } } From aa90f2598ae9b5bb1ef9676ecb68f951ada6599a Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Tue, 21 Sep 2021 07:52:05 +0200 Subject: [PATCH 5/5] Addition of user's groups in ACL Check error Signed-off-by: Florent Poinsard --- go/vt/vttablet/tabletserver/query_executor.go | 6 +++++- go/vt/vttablet/tabletserver/query_executor_test.go | 6 ++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/go/vt/vttablet/tabletserver/query_executor.go b/go/vt/vttablet/tabletserver/query_executor.go index 8994ab22901..38b2e30f312 100644 --- a/go/vt/vttablet/tabletserver/query_executor.go +++ b/go/vt/vttablet/tabletserver/query_executor.go @@ -444,7 +444,11 @@ func (qre *QueryExecutor) checkAccess(authorized *tableacl.ACLResult, tableName } if qre.tsv.qe.strictTableACL { - errStr := fmt.Sprintf("%s command denied to user '%s' for table '%s' (ACL check error)", qre.plan.PlanID.String(), callerID.Username, tableName) + groupStr := "" + if len(callerID.Groups) > 0 { + groupStr = fmt.Sprintf(", in groups [%s],", strings.Join(callerID.Groups, ", ")) + } + errStr := fmt.Sprintf("%s command denied to user '%s'%s for table '%s' (ACL check error)", qre.plan.PlanID.String(), callerID.Username, groupStr, tableName) qre.tsv.Stats().TableaclDenied.Add(statsKey, 1) qre.tsv.qe.accessCheckerLogger.Infof("%s", errStr) return vterrors.Errorf(vtrpcpb.Code_PERMISSION_DENIED, "%s", errStr) diff --git a/go/vt/vttablet/tabletserver/query_executor_test.go b/go/vt/vttablet/tabletserver/query_executor_test.go index 56c1c4b32ce..ce5a0b5c26d 100644 --- a/go/vt/vttablet/tabletserver/query_executor_test.go +++ b/go/vt/vttablet/tabletserver/query_executor_test.go @@ -822,6 +822,7 @@ func TestQueryExecutorMessageStreamACL(t *testing.T) { callerID = &querypb.VTGateCallerID{ Username: "u2", + Groups: []string{"non-admin"}, } qre.ctx = callerid.NewContext(context.Background(), nil, callerID) // Should fail because u2 does not have permission. @@ -829,7 +830,7 @@ func TestQueryExecutorMessageStreamACL(t *testing.T) { return io.EOF }) - assert.EqualError(t, err, `MessageStream command denied to user 'u2' for table 'msg' (ACL check error)`) + assert.EqualError(t, err, `MessageStream command denied to user 'u2', in groups [non-admin], for table 'msg' (ACL check error)`) if code := vterrors.Code(err); code != vtrpcpb.Code_PERMISSION_DENIED { t.Fatalf("qre.Execute: %v, want %v", code, vtrpcpb.Code_PERMISSION_DENIED) } @@ -1009,6 +1010,7 @@ func TestQueryExecutorTableAclExemptACL(t *testing.T) { username := "u2" callerID := &querypb.VTGateCallerID{ Username: username, + Groups: []string{"eng", "beta"}, } ctx := callerid.NewContext(context.Background(), nil, callerID) @@ -1034,7 +1036,7 @@ func TestQueryExecutorTableAclExemptACL(t *testing.T) { if code := vterrors.Code(err); code != vtrpcpb.Code_PERMISSION_DENIED { t.Fatalf("qre.Execute: %v, want %v", code, vtrpcpb.Code_PERMISSION_DENIED) } - assert.EqualError(t, err, `Select command denied to user 'u2' for table 'test_table' (ACL check error)`) + assert.EqualError(t, err, `Select command denied to user 'u2', in groups [eng, beta], for table 'test_table' (ACL check error)`) // table acl should be ignored since this is an exempt user. username = "exempt-acl"