Propagate SECURITY LABEL ON ROLE stmt #7304

naisila · 2023-11-01T13:02:42Z

DESCRIPTION: Propagates SECURITY LABEL ON ROLE statement

See official docs for explanation on how this command works: https://www.postgresql.org/docs/current/sql-security-label.html This command stores the label in the pg_shseclabel catalog table.

We propagate SECURITY LABEL [for provider] ON ROLE rolename IS labelname to the worker nodes.
We also make sure to run the relevant SecLabelStmt commands on a newly added node by looking at roles found in pg_shseclabel.

This commit also fixes the regex string in check_gucs_are_alphabetically_sorted.sh script such that it escapes the dot. Previously it was looking for all strings starting with "citus" instead of "citus." as it should.

New Note to reviewer: To test this feature, I currently make use of a special GUC to control label provider registration in PG_init when creating the Citus extension.

Old Note to reviewer: in the current test, I did what I could to ensure that I am connecting to the worker node with the same connection. This is needed to ensure that when I create the label provider, it is visible by the next command. If there is a better way to ensure commands to be in the same connection, let me know. There is a small issue in citus_add_node() - I couldn't find a way to test the successful completion of that one. However, through logged remote commands, we can see it fails when trying to propagate the SecLabel statement.

Old TODOs:

fix citus_add_node() to update pg_seclabel on worker node
tests
comments

codecov · 2023-11-02T09:56:38Z

Codecov Report

Merging #7304 (c36a71a) into main (c6fbb72) will decrease coverage by 0.15%.
Report is 1 commits behind head on main.
The diff coverage is 89.41%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7304      +/-   ##
==========================================
- Coverage   89.58%   89.44%   -0.15%     
==========================================
  Files         275      277       +2     
  Lines       59610    59687      +77     
  Branches     7427     7437      +10     
==========================================
- Hits        53403    53388      -15     
- Misses       4076     4137      +61     
- Partials     2131     2162      +31

src/backend/distributed/commands/seclabel.c

src/backend/distributed/deparser/deparse_seclabel_stmts.c

src/backend/distributed/commands/seclabel.c

src/backend/distributed/commands/role.c

src/test/regress/expected/seclabel.out

src/test/regress/sql/seclabel.sql

src/backend/distributed/commands/role.c

src/backend/distributed/shared_library_init.c

src/test/regress/sql/seclabel.sql

gurkanindibay

After fixing other comments, it is OK for me to merge

ci/check_gucs_are_alphabetically_sorted.sh

ci/gucs.out

src/backend/distributed/commands/distribute_object_ops.c

src/test/regress/pg_regress_multi.pl

onurctirtir · 2023-11-14T13:37:15Z

src/test/regress/sql/multi_test_helpers.sql

@@ -550,3 +550,35 @@ BEGIN
    RETURN result;
 END;
 $func$ LANGUAGE plpgsql;
+
+-- Returns pg_seclabels entries from all nodes in the cluster
+-- for which the provider is citus_tests_label_provider and the object name is the input


nit: missing dot

Suggested change

-- for which the provider is citus_tests_label_provider and the object name is the input

-- for which the provider is citus_tests_label_provider and the object name is the input.

onurctirtir

Those are my final comments, LGTM.

onurctirtir · 2023-11-14T22:07:13Z

src/test/regress/expected/seclabel.out

+SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS NULL;
+NOTICE:  issuing SECURITY LABEL FOR citus_tests_label_provider ON ROLE user1 IS NULL
+DETAIL:  on server postgres@localhost:xxxxx connectionId: xxxxxxx
+SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS 'citus unclassified';


Should we also add a test where we don't provide the "provider" at all?

SECURITY LABEL ON ROLE brand_new_role IS 'citus unclassified';

From the docs:

The name of the provider with which this label is to be associated. The named provider must be loaded and must consent to the proposed labeling operation. If exactly one provider is loaded, the provider name may be omitted for brevity.

I am skipping this test since I am loading two providers to test the character escaping thing.

Can't we reduce the number of providers we have to one; such that we only have the one that requires proper quoting?

Hmm okay, then we will not be testing the general case with name that doesn't need quoting, therefore I didn't want to do that. But it's okay I guess.

src/backend/distributed/shared_library_init.c

src/backend/distributed/commands/role.c

src/backend/distributed/commands/seclabel.c

onurctirtir · 2023-11-15T14:40:57Z

src/backend/distributed/commands/seclabel.c

+		return;
+	}
+
+	ereport(ERROR,


let's add such a test for that to make codecov happy, if possible

Actually I have added this test, I don't know why codecov is not happy xD

onurctirtir · 2023-11-15T14:43:00Z

src/test/regress/expected/seclabel.out

+SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS NULL;
+NOTICE:  issuing SECURITY LABEL FOR citus_tests_label_provider ON ROLE user1 IS NULL
+DETAIL:  on server postgres@localhost:xxxxx connectionId: xxxxxxx
+SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS 'citus unclassified';


Can't we reduce the number of providers we have to one; such that we only have the one that requires proper quoting?

initial commit

naisila force-pushed the naisila/sec_label_role branch 3 times, most recently from aca5abf to 14394a9 Compare November 2, 2023 09:49

naisila force-pushed the naisila/sec_label_role branch from 14394a9 to b88b4e6 Compare November 6, 2023 12:51

naisila marked this pull request as ready for review November 6, 2023 12:53

naisila force-pushed the naisila/sec_label_role branch from 7b66b96 to d37fed2 Compare November 7, 2023 12:24

naisila requested review from onurctirtir and gurkanindibay November 7, 2023 13:00

naisila force-pushed the naisila/sec_label_role branch 2 times, most recently from d92e621 to d4b0b32 Compare November 8, 2023 12:17

onurctirtir reviewed Nov 8, 2023

View reviewed changes

src/test/regress/expected/seclabel.out Outdated Show resolved Hide resolved

src/test/regress/expected/seclabel.out Outdated Show resolved Hide resolved

src/test/regress/sql/seclabel.sql Outdated Show resolved Hide resolved

JelteF reviewed Nov 8, 2023

View reviewed changes

src/test/regress/sql/seclabel.sql Outdated Show resolved Hide resolved

naisila force-pushed the naisila/sec_label_role branch 4 times, most recently from 1251ab5 to 963e1b9 Compare November 9, 2023 11:28

gurkanindibay reviewed Nov 9, 2023

View reviewed changes

src/backend/distributed/commands/role.c Outdated Show resolved Hide resolved

src/backend/distributed/shared_library_init.c Show resolved Hide resolved

src/test/regress/sql/seclabel.sql Show resolved Hide resolved

src/test/regress/sql/seclabel.sql Show resolved Hide resolved

naisila force-pushed the naisila/sec_label_role branch from 6cbde49 to 9a8be06 Compare November 9, 2023 12:10

naisila requested review from onurctirtir and gurkanindibay November 9, 2023 12:24

gurkanindibay approved these changes Nov 9, 2023

View reviewed changes

onurctirtir reviewed Nov 10, 2023

View reviewed changes

ci/check_gucs_are_alphabetically_sorted.sh Show resolved Hide resolved

JelteF reviewed Nov 10, 2023

View reviewed changes

ci/gucs.out Outdated Show resolved Hide resolved

onurctirtir reviewed Nov 10, 2023

View reviewed changes

src/backend/distributed/commands/distribute_object_ops.c Show resolved Hide resolved

JelteF reviewed Nov 10, 2023

View reviewed changes

src/test/regress/pg_regress_multi.pl Outdated Show resolved Hide resolved

naisila force-pushed the naisila/sec_label_role branch 2 times, most recently from b4111ed to 5ebaa80 Compare November 14, 2023 08:23

naisila requested a review from onurctirtir November 14, 2023 08:23

onurctirtir reviewed Nov 14, 2023

View reviewed changes

naisila force-pushed the naisila/sec_label_role branch from a6eb324 to 58f55e7 Compare November 15, 2023 08:58

naisila requested a review from onurctirtir November 15, 2023 09:32

onurctirtir approved these changes Nov 15, 2023

View reviewed changes

naisila added 10 commits November 16, 2023 12:37

Propagate SECURITY LABEL ON ROLE stmt

cfd98d1

initial commit

Fix PG14 failure, register label provider in Citus for testing

4c50216

Create a dummy label provider and add test

f485fe6

Fix citus_add_node() to run SecLabel stmts, add tests&comments

53075d4

addressing reviews

91353d3

Escape the dot after citus in check_gucs_are_alphabetically_sorted

4f02800

Address reviews

4825d24

Fix vanilla test

8ea0432

Address reviews

cdc97f0

Address final reviews

c36a71a

naisila force-pushed the naisila/sec_label_role branch from 58f55e7 to c36a71a Compare November 16, 2023 09:37

naisila merged commit 0d1f188 into main Nov 16, 2023
157 checks passed

naisila deleted the naisila/sec_label_role branch November 16, 2023 10:12

onurctirtir mentioned this pull request Nov 29, 2023

DDL Support #4812

Open

57 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate SECURITY LABEL ON ROLE stmt #7304

Propagate SECURITY LABEL ON ROLE stmt #7304

naisila commented Nov 1, 2023 •

edited

Loading

codecov bot commented Nov 2, 2023 •

edited

Loading

gurkanindibay left a comment

onurctirtir Nov 14, 2023

onurctirtir left a comment

onurctirtir Nov 14, 2023

naisila Nov 15, 2023

onurctirtir Nov 15, 2023

naisila Nov 16, 2023

onurctirtir Nov 15, 2023

naisila Nov 16, 2023

onurctirtir Nov 15, 2023

	-- for which the provider is citus_tests_label_provider and the object name is the input
	-- for which the provider is citus_tests_label_provider and the object name is the input.

Propagate SECURITY LABEL ON ROLE stmt #7304

Propagate SECURITY LABEL ON ROLE stmt #7304

Conversation

naisila commented Nov 1, 2023 • edited Loading

codecov bot commented Nov 2, 2023 • edited Loading

Codecov Report

gurkanindibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onurctirtir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naisila commented Nov 1, 2023 •

edited

Loading

codecov bot commented Nov 2, 2023 •

edited

Loading