sql: add syntax for supporting security definer

[annrpom]

This patch adds the syntax needed for supporting security
definer. Right now, if a routine is specified with
[EXTERNAL] SECURITY {INVOKER | DEFINER}, it is a no-op.
If the security mode is not specified, the default is
SECURITY INVOKER.

This also means that syntax for altering a function
supports the above options as well:

alter function f(int) external security definer;

Fixes: #128844
Epic: CRDB-37477

Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

It looks like your PR touches SQL parser code but doesn't add or edit parser tests. Please make sure you add or edit parser tests if you edit the parser.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[blathers-crl]

It looks like your PR touches SQL parser code but doesn't add or edit parser tests. Please make sure you add or edit parser tests if you edit the parser.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[rafiss]

nice work!

this change was small enough, so it doesn't matter too much, but for future reference: the pkg/sql/parser, pkg/sql/catalog, and pkg/sql/schemachanger changes, could be broken up into 3 separate commits. for other syntax/feature additions, that makes it easier to review, since we can merge the parser changes first, and get to the lower level changes which mirror the syntax separately.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom, @michae2, and @msbutler)

---

pkg/sql/catalog/catpb/function.proto line 81 at r1 (raw file):

  option (gogoproto.equal) = true;
  optional Function.Security security = 1 [(gogoproto.nullable) = false];
  optional bool external = 2 [(gogoproto.nullable) = false];

since the external keyword in the syntax actually does not have any functional effect, let's not include it in the proto

also, can we add a comment on security that says what the default behavior is if it's missing?

---

pkg/sql/opt/optbuilder/routine.go line 374 at r1 (raw file):

			panic(err)
		}
		// hi

hello!

---

pkg/sql/sem/tree/create_routine.go line 344 at r1 (raw file):

type RoutineSecurity struct {
	Security routineSecurityOptions
	External bool

since external doesn't do anything, we can just not include it in this struct, and always normalize when formatting the statement to not output it.

actually, if we don't have the external field, then we can avoid needing the struct at all. all we should need is the enum for invoker/definer.

---

pkg/ccl/logictestccl/testdata/logic_test/udf_params line 227 at r1 (raw file):

  CALLED ON NULL INPUT
  LANGUAGE plpgsql
  SECURITY INVOKER

i missed the logic that says that SECURITY INVOKER should be the default if it wasn't specified. could you point me to that, just wanna make sure i understand.

---

pkg/sql/parser/testdata/create_function line 555 at r1 (raw file):

parse
CREATE OR REPLACE FUNCTION f(a int = 7) RETURNS INT SECURITY INVOKER AS 'SELECT 1' LANGUAGE SQL

can you add some tests to make sure we can't specify security twice?

[mgartner]

Nice work!

Reviewed 65 of 65 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @annrpom, @michae2, and @msbutler)

---

pkg/sql/catalog/catpb/function.proto line 81 at r1 (raw file):

  option (gogoproto.equal) = true;
  optional Function.Security security = 1 [(gogoproto.nullable) = false];
  optional bool external = 2 [(gogoproto.nullable) = false];

If EXTERNAL has no effect, do we need to retain its value, or can we simply ignore it after parsing? If we ignore it, then SHOW CREATE FUNCTION ... won't be able to display EXTERNAL, but I don't think that's a problem, is it?

---

pkg/sql/opt/optbuilder/routine.go line 374 at r1 (raw file):

			panic(err)
		}
		// hi

hello :)

---

pkg/sql/schemachanger/screl/scalars.go line 130 at r1 (raw file):

	case *scpb.TypeComment, *scpb.DatabaseZoneConfig:
		return version.IsActive(clusterversion.V24_2)
	// TODO(before merge): gate v24_3

nit: Don't forget this :)

[annrpom]

this change was small enough, so it doesn't matter too much, but for future reference: the pkg/sql/parser, pkg/sql/catalog, and pkg/sql/schemachanger changes, could be broken up into 3 separate commits. for other syntax/feature additions, that makes it easier to review, since we can merge the parser changes first, and get to the lower level changes which mirror the syntax separately

good point -- ty for the knowledge

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner, @michae2, @msbutler, and @rafiss)

---

pkg/sql/catalog/catpb/function.proto line 81 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

since the external keyword in the syntax actually does not have any functional effect, let's not include it in the proto

also, can we add a comment on security that says what the default behavior is if it's missing?

done

---

pkg/sql/catalog/catpb/function.proto line 81 at r1 (raw file):

but I don't think that's a problem, is it?

tis not -- done

---

pkg/sql/opt/optbuilder/routine.go line 374 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

hello!

hi

---

pkg/sql/opt/optbuilder/routine.go line 374 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

hello :)

hi

---

pkg/sql/schemachanger/screl/scalars.go line 130 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: Don't forget this :)

done

---

pkg/sql/sem/tree/create_routine.go line 344 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

since external doesn't do anything, we can just not include it in this struct, and always normalize when formatting the statement to not output it.

actually, if we don't have the external field, then we can avoid needing the struct at all. all we should need is the enum for invoker/definer.

done

---

pkg/ccl/logictestccl/testdata/logic_test/udf_params line 227 at r1 (raw file):
i left the suggested comment in function.proto from:

also, can we add a comment on security that says what the default behavior is if it's missing?

i also added this in structured.proto & create_routine.go,

unless you mean general "logic" and not in the code -- which would be this line in the postgres docs:

SECURITY INVOKER indicates that the function is to be executed with the privileges of the user that calls it. That is the default.

let me know if this doesn't address your comment properly, though

---

pkg/sql/parser/testdata/create_function line 555 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

can you add some tests to make sure we can't specify security twice?

good point -- done! these are added in udf_options since we check this during buildCreateFunction

cockroach/pkg/sql/opt/optbuilder/create_function.go

Lines 78 to 80 in 16cd566

	if err := tree.ValidateRoutineOptions(cf.Options, cf.IsProcedure); err != nil {
	panic(err)
	}

[rafiss]

great work! lgtm!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @annrpom, @mgartner, @michae2, and @msbutler)

---

pkg/ccl/logictestccl/testdata/logic_test/udf_params line 227 at r1 (raw file):

Previously, annrpom (annie pompa) wrote…

i left the suggested comment in function.proto from:

also, can we add a comment on security that says what the default behavior is if it's missing?

i also added this in structured.proto & create_routine.go,

unless you mean general "logic" and not in the code -- which would be this line in the postgres docs:

SECURITY INVOKER indicates that the function is to be executed with the privileges of the user that calls it. That is the default.

let me know if this doesn't address your comment properly, though

thanks! i meant the code pointers; that was helpful.

[annrpom]

TFTRs! ('-')7

bors r=rafiss, mgartner

[craig]

This PR was included in a batch that was canceled, it will be automatically retried

[craig]

Build succeeded:

• windows_build
• examples_orms
• local_roachtest_fips
• macos_amd64_build
• acceptance
• linux_arm64_build
• lint
• local_roachtest
• check_generated_code
• macos_arm64_build
• linux_amd64_fips_build
• docker_image_amd64
• unit_tests
• linux_amd64_build