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.

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

New params in TPT query to indicate #137

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hujambo-dunia
Copy link
Contributor

@hujambo-dunia hujambo-dunia commented Jul 15, 2023

Few questions for us to ponder:

  1. (high) What is else is necessary to prevent SQL injection?
  2. (moderate) More elegant way to pull in variables: arrSelect1 and arrWhere1 (JSON, multidimensional array, etc)?

@@ -4760,16 +4770,24 @@ query_tpt-tool-cpu() { ##? [--startyear=<YYYY>] [--endyear=<YYYY>] [--formula=av
sql_formula="SUM"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this variable not used?

@hexylena
Copy link
Member

SQL Injection

This is not a priority. This project will probably never add anything to prevent SQL injection outside of common sense checks on input. See e.g. q and aq in gxadmin query. gxadmin simply passes sql to the psql client, if you can run gxadmin, you can already also run arbitrary queries with psql. I do not think it is technically possible to provide any meaningful security layer between the user and psql.

Admins installing gxadmin should understand that they're granting read write database access to anyone with gxadmin access. You could setup a separate role that e.g. restricts table access. If SQL injection is a concern, then I strongly recommend doing this. A read-only DB account will prevent a lot of issues.

I think EU is currently the only place granting a select group of users the ability to run gxadmin just for statistics queries, but there I believe they use read-only access (right @mira-miracoli ?)

More elegant way to pull in variables: arrSelect1 and arrWhere1 (JSON, multidimensional array, etc)?

I'm not sure there is a more elegant way, but the current changes are hmm. It's not my query so I won't push back too hard, but, providing extra bits of sql in this way is not a very user friendly or ergonomic design.

You're also hitting the limits of our argument parsing library there. The ##? indicates that everything that follows should be parsed by our argument parser. If you do not explicitly name the additional arguments, they will not be set as parameters for the function: an error should be thrown.

We only have one other function that accepts arbitrary numbers of arguments but it's function signature is much simpler.

I'd honestly suggest maybe separate queries per metric, or a single query with an optional --destination filter for the stampede cluster, and another optional --metric that lets you override the default metric.

For design inspiration you could look at the jobs query which lets you filter by destination/user/a few other things. But they're all explicitly named arguments (which generally i think is easier for users because they'll know what's available, rather than having to dig into the DB to figure out which columns they can filter against).

@kysrpex
Copy link
Contributor

kysrpex commented Jul 19, 2023

@hexylena Yes, in EU a select group of users can log-in as a specific Linux user that has read-only access to the database, so their gxadmin calls cannot alter the database contents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants