sql: fix EXECUTE privileges for UDFs

EXECUTE privileges are now correctly checked for each user-defined
function referenced in a query.

Informs cockroachdb#103842

Release note (bug fix): A bug has been fixed that allowed users to
execute a user-defined function without having EXECUTE privileges on the
function. This bug has been present since version 22.2 when UDFs were
introduced.

docs/generated/settings/settings-for-tenants.txt

@@ -314,4 +314,4 @@ trace.opentelemetry.collector	string		address of an OpenTelemetry trace collecto
 trace.snapshot.rate	duration	0s	if non-zero, interval at which background trace snapshots are captured	tenant-rw
 trace.span_registry.enabled	boolean	true	if set, ongoing traces can be seen at https://<ui>/#/debug/tracez	tenant-rw
 trace.zipkin.collector	string		the address of a Zipkin instance to receive traces, as <host>:<port>. If no port is specified, 9411 will be used.	tenant-rw
-version	version	1000023.1-24	set the active cluster version in the format '<major>.<minor>'	tenant-rw
+version	version	1000023.1-26	set the active cluster version in the format '<major>.<minor>'	tenant-rw

docs/generated/settings/settings.html

@@ -267,6 +267,6 @@
 <tr><td><div id="setting-trace-span-registry-enabled" class="anchored"><code>trace.span_registry.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>if set, ongoing traces can be seen at https://&lt;ui&gt;/#/debug/tracez</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-trace-zipkin-collector" class="anchored"><code>trace.zipkin.collector</code></div></td><td>string</td><td><code></code></td><td>the address of a Zipkin instance to receive traces, as &lt;host&gt;:&lt;port&gt;. If no port is specified, 9411 will be used.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-ui-display-timezone" class="anchored"><code>ui.display_timezone</code></div></td><td>enumeration</td><td><code>etc/utc</code></td><td>the timezone used to format timestamps in the ui [etc/utc = 0, america/new_york = 1]</td><td>Dedicated/Self-Hosted</td></tr>
-<tr><td><div id="setting-version" class="anchored"><code>version</code></div></td><td>version</td><td><code>1000023.1-24</code></td><td>set the active cluster version in the format &#39;&lt;major&gt;.&lt;minor&gt;&#39;</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
+<tr><td><div id="setting-version" class="anchored"><code>version</code></div></td><td>version</td><td><code>1000023.1-26</code></td><td>set the active cluster version in the format &#39;&lt;major&gt;.&lt;minor&gt;&#39;</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 </tbody>
 </table>

pkg/clusterversion/cockroach_versions.go

@@ -570,6 +570,10 @@ const (
 	// options lagging_ranges_threshold and lagging_ranges_polling_interval.
 	V23_2_ChangefeedLaggingRangesOpts
 
+	// V23_2_GrantExecuteToPublic grants the EXECUTE privilege to the public
+	// role for all existing functions.
+	V23_2_GrantExecuteToPublic
+
 	// *************************************************
 	// Step (1) Add new versions here.
 	// Do not add new versions to a patch release.
@@ -995,6 +999,10 @@ var rawVersionsSingleton = keyedVersions{
 		Key:     V23_2_ChangefeedLaggingRangesOpts,
 		Version: roachpb.Version{Major: 23, Minor: 1, Internal: 24},
 	},
+	{
+		Key:     V23_2_GrantExecuteToPublic,
+		Version: roachpb.Version{Major: 23, Minor: 1, Internal: 26},
+	},
 
 	// *************************************************
 	// Step (2): Add new versions here.

pkg/sql/logictest/testdata/logic_test/mixed_version_udf_execute_privileges

@@ -0,0 +1,70 @@
+# LogicTest: cockroach-go-testserver-upgrade-to-master
+
+# Verify that all nodes are running previous version binaries.
+
+query T nodeidx=0
+SELECT crdb_internal.node_executable_version()
+----
+23.1
+
+query T nodeidx=1
+SELECT crdb_internal.node_executable_version()
+----
+23.1
+
+query T nodeidx=2
+SELECT crdb_internal.node_executable_version()
+----
+23.1
+
+# Create test user.
+
+statement ok
+CREATE USER testuser1
+
+# Create a user-defined function.
+
+statement ok
+CREATE FUNCTION f() RETURNS INT LANGUAGE SQL AS 'SELECT 1'
+
+user testuser1
+
+query I
+SELECT f()
+----
+1
+
+# Upgrade node 0 and verify that the user can use the function in mixed version
+# mode.
+
+upgrade 0
+
+user testuser1 nodeidx=0
+
+query I
+SELECT f()
+----
+1
+
+# Upgrade all nodes.
+
+upgrade 1
+
+upgrade 2
+
+# Verify that all nodes are now running 23.2 binaries.
+
+query B nodeidx=0
+SELECT crdb_internal.node_executable_version() SIMILAR TO '23.1-%'
+----
+true
+
+query B nodeidx=1
+SELECT crdb_internal.node_executable_version() SIMILAR TO '23.1-%'
+----
+true
+
+query B nodeidx=2
+SELECT crdb_internal.node_executable_version() SIMILAR TO '23.1-%'
+----
+true

pkg/sql/logictest/testdata/logic_test/udf_privileges

@@ -676,3 +676,57 @@ statement ok
 DROP USER u_test_owner;
 
 subtest end
+
+statement ok
+CREATE USER tester
+
+statement ok
+CREATE SCHEMA test;
+
+statement ok
+GRANT USAGE ON SCHEMA test TO tester;
+
+statement ok
+CREATE FUNCTION test.my_add(a INT, b INT) RETURNS INT IMMUTABLE LEAKPROOF LANGUAGE SQL AS 'SELECT a + b';
+
+statement ok
+SET ROLE tester
+
+# The tester role receives execute privileges to functions via the public role.
+statement ok
+SELECT test.my_add(1,2)
+
+statement ok
+SET ROLE root
+
+# Revoke execute privilege from the public role.
+statement ok
+REVOKE EXECUTE ON FUNCTION test.my_add FROM public
+
+# The root role can still execute the function.
+statement ok
+SELECT test.my_add(1,2)
+
+statement ok
+SET ROLE tester
+
+skipif config local-mixed-22.2-23.1
+statement error pgcode 42501 user tester does not have EXECUTE privilege on function my_add
+SELECT test.my_add(1,2)
+
+skipif config local-mixed-22.2-23.1
+statement error pgcode 42501 user tester does not have EXECUTE privilege on function my_add
+SELECT * FROM (VALUES (1), (2)) AS v(i) WHERE i = test.my_add(1,2)
+
+statement ok
+SET ROLE root
+
+# Re-grant execut privilege to the public role.
+statement ok
+GRANT EXECUTE ON FUNCTION test.my_add TO public
+
+statement ok
+SET ROLE tester
+
+statement ok
+SELECT test.my_add(1,2)

pkg/sql/logictest/tests/cockroach-go-testserver-upgrade-to-master/BUILD.bazel

@@ -17,7 +17,7 @@ go_test(
         "dockerNetwork": "standard",
         "Pool": "large",
     },
-    shard_count = 10,
+    shard_count = 11,
     tags = [
         "cpu:2",
     ],

pkg/sql/opt/cat/catalog.go

@@ -173,6 +173,11 @@ type Catalog interface {
 	// the given catalog object. If not, then CheckAnyPrivilege returns an error.
 	CheckAnyPrivilege(ctx context.Context, o Object) error
 
+	// CheckExecutionPrivilege verifies that the current user has execution
+	// privileges for the UDF with the given OID. If not, then CheckPrivilege
+	// returns an error.
+	CheckExecutionPrivilege(ctx context.Context, oid oid.Oid) error
+
 	// HasAdminRole checks that the current user has admin privileges. If yes,
 	// returns true. Returns an error if query on the `system.users` table failed
 	HasAdminRole(ctx context.Context) (bool, error)

pkg/sql/opt/memo/BUILD.bazel

@@ -102,6 +102,7 @@ go_test(
         "//pkg/sql/sem/eval",
         "//pkg/sql/sem/tree",
         "//pkg/sql/sem/tree/treewindow",
+        "//pkg/sql/sessiondata",
         "//pkg/sql/types",
         "//pkg/testutils",
         "//pkg/testutils/datapathutils",

pkg/sql/opt/memo/memo_test.go

@@ -25,9 +25,11 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/testutils/testcat"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/xform"
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
+	_ "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
 	"github.com/cockroachdb/cockroach/pkg/util/duration"
@@ -142,6 +144,10 @@ func TestMemoIsStale(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	_, err = catalog.ExecuteDDL("CREATE FUNCTION one() RETURNS INT LANGUAGE SQL AS $$ SELECT 1 $$")
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	// Revoke access to the underlying table. The user should retain indirect
 	// access via the view.
@@ -150,6 +156,7 @@ func TestMemoIsStale(t *testing.T) {
 	// Initialize context with starting values.
 	evalCtx := eval.MakeTestingEvalContext(cluster.MakeTestingClusterSettings())
 	evalCtx.SessionData().Database = "t"
+	evalCtx.SessionData().SearchPath = sessiondata.MakeSearchPath([]string{"public"})
 	// MakeTestingEvalContext created a fake planner that can only provide the
 	// memory monitor and will encounter a nil-pointer error when other methods
 	// are accessed. In this test, GetDatabaseSurvivalGoal method will be called
@@ -159,7 +166,7 @@ func TestMemoIsStale(t *testing.T) {
 	evalCtx.StreamManagerFactory = nil
 
 	var o xform.Optimizer
-	opttestutils.BuildQuery(t, &o, catalog, &evalCtx, "SELECT a, b+1 FROM abcview WHERE c='foo'")
+	opttestutils.BuildQuery(t, &o, catalog, &evalCtx, "SELECT a, b+one() FROM abcview WHERE c='foo'")
 	o.Memo().Metadata().AddSchema(catalog.Schema())
 
 	ctx := context.Background()
@@ -175,7 +182,7 @@ func TestMemoIsStale(t *testing.T) {
 		// tests as written still pass if the default value is 0. To detect this, we
 		// create a new memo with the changed setting and verify it's not stale.
 		var o2 xform.Optimizer
-		opttestutils.BuildQuery(t, &o2, catalog, &evalCtx, "SELECT a, b+1 FROM abcview WHERE c='foo'")
+		opttestutils.BuildQuery(t, &o2, catalog, &evalCtx, "SELECT a, b+one() FROM abcview WHERE c='foo'")
 
 		if isStale, err := o2.Memo().IsStale(ctx, &evalCtx, catalog); err != nil {
 			t.Fatal(err)
@@ -408,6 +415,15 @@ func TestMemoIsStale(t *testing.T) {
 	catalog.View(tree.NewTableNameWithSchema("t", catconstants.PublicSchemaName, "abcview")).Revoked = false
 	notStale()
 
+	// User no longer has execution privilege on a UDF.
+	catalog.RevokeExecution(catalog.Function("one").Oid)
+	_, err = o.Memo().IsStale(ctx, &evalCtx, catalog)
+	if exp := "user does not have privilege to execute function"; !testutils.IsError(err, exp) {
+		t.Fatalf("expected %q error, but got %+v", exp, err)
+	}
+	catalog.GrantExecution(catalog.Function("one").Oid)
+	notStale()
+
 	// Stale data sources and schema. Create new catalog so that data sources are
 	// recreated and can be modified independently.
 	catalog = testcat.New()
@@ -419,6 +435,10 @@ func TestMemoIsStale(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	_, err = catalog.ExecuteDDL("CREATE FUNCTION one() RETURNS INT LANGUAGE SQL AS $$ SELECT 1 $$")
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	// Table ID changes.
 	catalog.Table(tree.NewTableNameWithSchema("t", catconstants.PublicSchemaName, "abc")).TabID = 1
@@ -431,6 +451,12 @@ func TestMemoIsStale(t *testing.T) {
 	stale()
 	catalog.Table(tree.NewTableNameWithSchema("t", catconstants.PublicSchemaName, "abc")).TabVersion = 0
 	notStale()
+
+	// Function Version changes.
+	catalog.Function("one").Version = 1
+	stale()
+	catalog.Function("one").Version = 0
+	notStale()
 }
 
 // TestStatsAvailable tests that the statisticsBuilder correctly identifies

pkg/sql/opt/metadata.go

@@ -442,6 +442,14 @@ func (md *Metadata) CheckDependencies(
 		}
 	}
 
+	// Check that the role still has execution privilege on the user defined
+	// functions.
+	for _, overload := range md.udfDeps {
+		if err := optCatalog.CheckExecutionPrivilege(ctx, overload.Oid); err != nil {
+			return false, err
+		}
+	}
+
 	// Check that any references to builtin functions do not now resolve to a UDF
 	// with the same signature (e.g. after changes to the search path).
 	for name := range md.builtinRefsByName {

pkg/sql/opt/optbuilder/scalar.go

@@ -616,6 +616,9 @@ func (b *Builder) buildFunction(
 
 // buildUDF builds a set of memo groups that represents a user-defined function
 // invocation.
+//
+// TODO(mgartner): This function also builds built-in functions defined with a
+// SQL body. Consider renaming it to make it more clear.
 func (b *Builder) buildUDF(
 	f *tree.FuncExpr,
 	def *tree.ResolvedFunctionDefinition,
@@ -624,6 +627,15 @@ func (b *Builder) buildUDF(
 	colRefs *opt.ColSet,
 ) (out opt.ScalarExpr) {
 	o := f.ResolvedOverload()
+
+	// Check for execution privileges for user-defined overloads. Built-in
+	// overloads do not need to be checked.
+	if o.IsUDF {
+		if err := b.catalog.CheckExecutionPrivilege(b.ctx, o.Oid); err != nil {
+			panic(err)
+		}
+	}
+
 	b.factory.Metadata().AddUserDefinedFunction(o, f.Func.ReferenceByName)
 
 	if o.Type == tree.ProcedureRoutine {

pkg/sql/opt/testutils/build.go

@@ -31,19 +31,21 @@ func BuildQuery(
 ) {
 	stmt, err := parser.ParseOne(sql)
 	if err != nil {
-		t.Fatal(err)
+		t.Fatalf("%+v", err)
 	}
 
 	ctx := context.Background()
 	semaCtx := tree.MakeSemaContext()
+	semaCtx.FunctionResolver = catalog
+	semaCtx.SearchPath = &evalCtx.SessionData().SearchPath
 	if err := semaCtx.Placeholders.Init(stmt.NumPlaceholders, nil /* typeHints */); err != nil {
-		t.Fatal(err)
+		t.Fatalf("%+v", err)
 	}
 	semaCtx.Annotations = tree.MakeAnnotations(stmt.NumAnnotations)
 	o.Init(ctx, evalCtx, catalog)
 	err = optbuilder.New(ctx, &semaCtx, evalCtx, catalog, o.Factory(), stmt.AST).Build()
 	if err != nil {
-		t.Fatal(err)
+		t.Fatalf("%+v", err)
 	}
 }
 
@@ -53,7 +55,7 @@ func BuildScalar(
 ) opt.ScalarExpr {
 	expr, err := parser.ParseExpr(input)
 	if err != nil {
-		t.Fatal(err)
+		t.Fatalf("%+v", err)
 	}
 
 	b := optbuilder.NewScalar(context.Background(), semaCtx, evalCtx, f)