Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sql/tree: the schema names and role/user names are not properly anonymized #55385

Closed
knz opened this issue Oct 9, 2020 · 2 comments · Fixed by #55473
Closed

sql/tree: the schema names and role/user names are not properly anonymized #55385

knz opened this issue Oct 9, 2020 · 2 comments · Fixed by #55473
Assignees
@solongordon
Labels
A-security A-sql-syntax Issues strictly related to the SQL grammar, with no semantic aspect C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked.

Comments

@knz
Copy link
Contributor

knz commented Oct 9, 2020

The AST uses the Go type string to represent:

  • schema names in tree.CreateSchema
  • role names and role list names in various "Owner" fields for REASSIGN TO / AUTHORIZATION fields.

This is incorrect: only tree.Name is equipped to automatically and properly anonymize the identifiers when constructing telemetry data.

With the current code, the telemetry leaks (potentially sensitive) schema names and role names.

cc @solongordon @arulajmani
@knz knz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-syntax Issues strictly related to the SQL grammar, with no semantic aspect A-security release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. labels Oct 9, 2020
@knz
Copy link
Contributor Author

knz commented Oct 9, 2020

The code must be changed to use Name and NameList as appropriate instead.

Related (but not identical to) #54696

@knz knz mentioned this issue Oct 9, 2020
Merged
@solongordon solongordon self-assigned this Oct 12, 2020
solongordon added a commit to solongordon/cockroach that referenced this issue Oct 12, 2020
@solongordon
sql: anonymize schema and owner names
fc8b37a 
Schema and owner names were improperly being represented by strings
rather than `tree.Name`s in the AST. This meant they would not be
properly anonymized in telemetry.

Fixes cockroachdb#55385

Release note: None
@solongordon solongordon mentioned this issue Oct 12, 2020
Merged
@solongordon
Copy link
Contributor

This applies to ENUM values as well. I'll resolve this in the same PR.

solongordon added a commit to solongordon/cockroach that referenced this issue Oct 13, 2020
@solongordon
sql: anonymize schema and owner names
4095146 
Schema and owner names were improperly being represented by strings
rather than `tree.Name`s in the AST. This meant they would not be
properly anonymized in telemetry.

Fixes cockroachdb#55385

Release note: None
solongordon added a commit to solongordon/cockroach that referenced this issue Oct 13, 2020
@solongordon
sql: anonymize enum values
5d9fcac 
ENUM values were improperly being represented as strings in the AST,
which meant that they were not anonymized in telemetry. I created a new
AST node type for them which respects anonymization.

Fixes cockroachdb#55385

Release note: None
solongordon added a commit to solongordon/cockroach that referenced this issue Oct 13, 2020
@solongordon
sql: anonymize schema and owner names
5b579d7 
Schema and owner names were improperly being represented by strings
rather than `tree.Name`s in the AST. This meant they would not be
properly anonymized in telemetry.

Fixes cockroachdb#55385

Release note: None
solongordon added a commit to solongordon/cockroach that referenced this issue Oct 13, 2020
@solongordon
sql: anonymize enum values
4a1a6f7 
ENUM values were improperly being represented as strings in the AST,
which meant that they were not anonymized in telemetry. I created a new
AST node type for them which respects anonymization.

Fixes cockroachdb#55385

Release note: None
solongordon added a commit to solongordon/cockroach that referenced this issue Oct 13, 2020
@solongordon
sql: anonymize schema and owner names
50aaaa5 
Schema and owner names were improperly being represented by strings
rather than `tree.Name`s in the AST. This meant they would not be
properly anonymized in telemetry.

Fixes cockroachdb#55385

Release note: None
solongordon added a commit to solongordon/cockroach that referenced this issue Oct 13, 2020
@solongordon
sql: anonymize enum values
fbce16e 
ENUM values were improperly being represented as strings in the AST,
which meant that they were not anonymized in telemetry. I created a new
AST node type for them which respects anonymization.

Fixes cockroachdb#55385

Release note: None
@solongordon solongordon mentioned this issue Oct 13, 2020
Merged
solongordon added a commit to solongordon/cockroach that referenced this issue Oct 13, 2020
@solongordon
sql: anonymize enum values
bad7df0 
ENUM values were improperly being represented as strings in the AST,
which meant that they were not anonymized in telemetry. I created a new
AST node type for them which respects anonymization.

Fixes cockroachdb#55385

Release note: None
craig bot pushed a commit that referenced this issue Oct 13, 2020
@solongordon
Merge #55473
a842328 
55473: sql: anonymize schema names, owner names, and enum values r=solongordon a=solongordon

Schema and owner names were improperly being represented by strings
rather than `tree.Name`s in the AST. This meant they would not be
properly anonymized in telemetry.

ENUM values had the same problem, but we cannot use `tree.Name` in
this case because we need the values to be quoted. I created a custom AST
node for this case.

Fixes #55385

Release note: None

Co-authored-by: Solon Gordon <[email protected]>
@craig craig bot closed this as completed in 12ad408 Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@solongordon solongordon

Labels
A-security A-sql-syntax Issues strictly related to the SQL grammar, with no semantic aspect C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql: anonymize schema names, owner names, and enum values solongordon/cockroach
3 participants
@knz @solongordon @thoszhang