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

feat: added new parameters for parameterized query #597

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

nekomeowww
Copy link
Contributor

@nekomeowww nekomeowww commented Nov 21, 2023

What type of PR is this?

/kind bug

What this PR does / why we need it

As

if utilfeature.DefaultMutableFeatureGate.Enabled(AllowRawSQLQuery) {
if len(opts.URLQuery[URLQueryWhereSQL]) > 0 {
// TODO: prevent SQL injection.
// If a string of numbers is passed in from SQL, the query will be taken as ID by default.
// If the SQL contains English letter, it will be passed in as column.
query = query.Where(opts.URLQuery[URLQueryWhereSQL][0])
}
}

has stated. There are potential SQL injection vulnerabilities of current implementation while users cannot easily defend it due to the lack of support to parameterized query.

This Pull Request

  1. supports parameterized query by using the native feature of Where(...) which GORM supports and will pass the required parameters from whereSQLParam and whereSQLJSONParams field when
    whereSQLStatement was used. Newly added fields:
  • whereSQLStatement, drop-in replacement of the existing whereSQL field while whereSQL will be deprecated in the future release.
  • whereSQLParam, for callers of Clusterpedia API to pass the parameters for the raw SQL. Users can send raw queries more safely by using ? as the placeholder of parameters (which is compatible for GORM) and then include all the corresponding parameters with either one of the newly added field whereSQLParam and whereSQLJSONParams.
  • whereSQLJSONParams, useful when users need more than just string type of values of parameters (e.g. IN and NOT IN).
  1. added a new AllowParameterizedSQLQuery feature gate to enable the support of whereSQLStatement, whereSQLParam, and whereSQLJSONParams when transitioning and migrating from whereSQL before its deprecation.

For more about the SQL injection of GORM, please read it more here: https://gorm.io/docs/security.html

Disclaimer: the potentials of SQL injection will NEVER be resolved if implementations of caller still use string concatenation to concat the raw SQL query with parameters directly without whereSQLParam or whereSQLJSONParams when using whereSQLStatement. End users are responsible for defensing and protecting the resources from malicious access at all the time. This Pull Request will only introduce new parameters to help users to defend with parameterized queries.

Besides parameterized query, even though the overall security will be improved with this patch, Clusterpedia cannot guarantee there will be no potential threats of SQL injections in the future and should never be when raw SQL involves.

End users should check, validate, monitor and restrict the scope of database access as much as possible to reduce the damage and limit the attacking surface even when it failed to defense.

Which issue(s) this PR fixes

None. This is a known issue.

Special notes for your reviewer

Does this PR introduce a user-facing change?

**Please update all the existing SQLs of raw queries to pass the parameters of raw SQL in `whereSQLParam` or `whereSQLJSONParams` field instead of string concatenation since it introduces the potential SQL injection when attackers obtained the access to Clusterpedia API.**

Test cases

Assumes we have the following resources:

❯ kubectl get all -A                                                                          
NAMESPACE              NAME                                                      READY   STATUS    RESTARTS   AGE
clusterpedia-test-ns   pod/hello-node-in-clusterpedia-test-ns-6fbb8854b5-tvncw   1/1     Running   0          26s
default                pod/hello-node-7579565d66-mvmt8                           1/1     Running   0          4h9m
kube-system            pod/coredns-5d78c9869d-d2tp8                              1/1     Running   0          4h10m
kube-system            pod/coredns-5d78c9869d-lkk4f                              1/1     Running   0          4h10m
kube-system            pod/etcd-kind-control-plane                               1/1     Running   0          4h10m
kube-system            pod/kindnet-lt8bn                                         1/1     Running   0          4h10m
kube-system            pod/kube-apiserver-kind-control-plane                     1/1     Running   0          4h10m
kube-system            pod/kube-controller-manager-kind-control-plane            1/1     Running   0          4h10m
kube-system            pod/kube-proxy-ssgg6                                      1/1     Running   0          4h10m
kube-system            pod/kube-scheduler-kind-control-plane                     1/1     Running   0          4h10m
local-path-storage     pod/local-path-provisioner-6bc4bddd6b-lssln               1/1     Running   0          4h10m

NAMESPACE              NAME                                         TYPE           CLUSTER-IP      EXTERNAL-IP   PORT(S)                  AGE
clusterpedia-test-ns   service/hello-node-in-clusterpedia-test-ns   LoadBalancer   10.96.252.86    <pending>     8080:30323/TCP           6s
default                service/hello-node                           LoadBalancer   10.96.134.228   <pending>     8080:32712/TCP           4h8m
default                service/kubernetes                           ClusterIP      10.96.0.1       <none>        443/TCP                  4h10m
kube-system            service/kube-dns                             ClusterIP      10.96.0.10      <none>        53/UDP,53/TCP,9153/TCP   4h10m

NAMESPACE     NAME                        DESIRED   CURRENT   READY   UP-TO-DATE   AVAILABLE   NODE SELECTOR            AGE
kube-system   daemonset.apps/kindnet      1         1         1       1            1           kubernetes.io/os=linux   4h10m
kube-system   daemonset.apps/kube-proxy   1         1         1       1            1           kubernetes.io/os=linux   4h10m

NAMESPACE              NAME                                                 READY   UP-TO-DATE   AVAILABLE   AGE
clusterpedia-test-ns   deployment.apps/hello-node-in-clusterpedia-test-ns   1/1     1            1           26s
default                deployment.apps/hello-node                           1/1     1            1           4h9m
kube-system            deployment.apps/coredns                              2/2     2            2           4h10m
local-path-storage     deployment.apps/local-path-provisioner               1/1     1            1           4h10m

NAMESPACE              NAME                                                            DESIRED   CURRENT   READY   AGE
clusterpedia-test-ns   replicaset.apps/hello-node-in-clusterpedia-test-ns-6fbb8854b5   1         1         1       26s
default                replicaset.apps/hello-node-7579565d66                           1         1         1       4h9m
kube-system            replicaset.apps/coredns-5d78c9869d                              2         2         2       4h10m
local-path-storage     replicaset.apps/local-path-provisioner-6bc4bddd6b               1         1         1       4h10m

GET with whereSQLStatement and single whereSQLParam with SQL injection statements

Case 1

Parameters with

field value
whereSQLStatement namespace IN (?)
whereSQLParam default) OR \( = \

you can test it with

curl --location 'https://localhost:8443/apis/clusterpedia.io/v1beta1/resources/apis/apps/v1/deployments?whereSQLStatement=namespace%20IN%20(%3F)&whereSQLParam=default)%20OR%20%5C(%20%3D%20%5C'

and it will eventually be sent to database and executed like this:

SELECT `object` FROM `resources` WHERE `group` = "apps" AND `resource` = "deployments" AND `version` = "v1" AND namespace IN ("default) OR \( = \")

you will get the following resources that returned by database:

name namespace

Case 2

Parameters with

field value
whereSQLStatement namespace IN (?)
whereSQLParam "any" OR 1 = 1) #

you can test it with

curl --location 'https://localhost:8443/apis/clusterpedia.io/v1beta1/resources/apis/apps/v1/deployments?whereSQLStatement=namespace%20IN%20(%3F)&whereSQLParam=%22any%22%20OR%201%20%3D%201)%20%23'

and it will eventually be sent to database and executed like this:

SELECT `object` FROM `resources` WHERE `group` = "apps" AND `resource` = "deployments" AND `version` = "v1" AND namespace IN ("\"any\" OR 1 = 1) #")

you will get the following resources that returned by database:

name namespace

GET with whereSQLStatement and single whereSQLParam

Parameters with

field value
whereSQLStatement (namespace IN (?))
whereSQLParam default

you can test it with

curl --location 'https://localhost:8443/apis/clusterpedia.io/v1beta1/resources/apis/apps/v1/deployments?whereSQLStatement=(namespace%20IN%20(%3F))&whereSQLParam=default'

and it will eventually be sent to database and executed like this:

SELECT `object` FROM `resources` WHERE `group` = "apps" AND `resource` = "deployments" AND `version` = "v1" AND (namespace IN ("default"))

you will get the following resources that returned by database:

name namespace
hello-node default

GET with whereSQLStatement and multiple whereSQLParam

Case 1

Parameters with

field value
whereSQLStatement (namespace IN (?, ?))
whereSQLParam default
whereSQLParam kube-system

Note

The reason why the query in whereSQLStatement consists two ? (question mark) is because whereSQLParam will be parsed as array of strings by default and use the spread syntax in Go to pass the values in to the actual query.
Therefore it will be equivalent to

db.Where("(namespace IN (?, ?))", "default", "kube-system")

instead of

db.Where("(namespace IN (?, ?))", []string{"default", "kube-system"})

that you may think of.

you can test it with

curl --location 'https://localhost:8443/apis/clusterpedia.io/v1beta1/resources/apis/apps/v1/deployments?whereSQLStatement=(namespace%20IN%20(%3F%2C%20%3F))&whereSQLParam=default&whereSQLParam=kube-system'

and it will eventually be sent to database and executed like this:

SELECT `object` FROM `resources` WHERE `group` = "apps" AND `resource` = "deployments" AND `version` = "v1" AND (namespace IN ("default", "kube-system"))
name namespace
hello-node default
coredns kube-system

Case 2

Parameters with

field value
whereSQLStatement (namespace = (?) OR namespace = (?))
whereSQLParam default
whereSQLParam kube-system

you can test it with

curl --location 'https://localhost:8443/apis/clusterpedia.io/v1beta1/resources/apis/apps/v1/deployments?whereSQLStatement=(namespace%20%3D%20(%3F)%20OR%20namespace%20%3D%20(%3F))&whereSQLParam=default&whereSQLParam=kube-system'

and it will eventually be sent to database and executed like this:

SELECT `object` FROM `resources` WHERE `group` = "apps" AND `resource` = "deployments" AND `version` = "v1" AND ((namespace = ("default") OR namespace = ("kube-system")))
name namespace
hello-node default
coredns kube-system

GET with whereSQLStatement and whereSQLJSONParams

As you may noticed, users must explicitly write down how many variables/parameters have with the same number of ? (question mark) when using whereSQLParam. This is not flexible enough for users who may want to treat any length of arrays as a single variable/parameter and represent it with a single ? (question mark). This is where whereSQLJSONParams might be helpful for such use case and scenario.

Parameters with

field value
whereSQLStatement (namespace IN (?))
whereSQLJSONParams W1siZGVmYXVsdCIsImt1YmUtc3lzdGVtIl1d (the base64 encoded of [ [ "default", "kube-system" ] ])

you can test it with

curl --location 'https://localhost:8443/apis/clusterpedia.io/v1beta1/resources/apis/apps/v1/deployments?whereSQLStatement=(namespace%20IN%20(%3F))&whereSQLJSONParams=W1siZGVmYXVsdCIsImt1YmUtc3lzdGVtIl1d'

and it will eventually be sent to database and executed like this:

SELECT `object` FROM `resources` WHERE `group` = "apps" AND `resource` = "deployments" AND `version` = "v1" AND (namespace IN ("default","kube-system"))

@clusterpedia-bot
Copy link

Hi @nekomeowww,
Thanks for your pull request!
If the PR is ready, use the /auto-cc command to assign Reviewer to Review.
We will review it shortly.

Details

Instructions for interacting with me using comments are available here.
If you have questions or suggestions related to my behavior, please file an issue against the gh-ci-bot repository.

@clusterpedia-bot clusterpedia-bot added the kind/bug Something isn't working label Nov 21, 2023
@nekomeowww
Copy link
Contributor Author

/auto-cc

Copy link
Member

@Iceber Iceber left a comment

Choose a reason for hiding this comment

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

We could probably add a new feature gate to differentiate from the AllowRawSQLQuery/whereSQL functionality, this pr would allow us to avoid sql injection if the user is using it properly

The whereSQL will be deprecated (tracked by other issues and prs) and removed for 1.0

pkg/storage/internalstorage/features.go Outdated Show resolved Hide resolved
pkg/storage/internalstorage/util.go Outdated Show resolved Hide resolved
pkg/storage/internalstorage/util.go Outdated Show resolved Hide resolved
@nekomeowww nekomeowww changed the title fix: the protential SQL injection issue when users toggled the AllowRawSQLQuery feature gate on feat: added new parameters for parameterized query Nov 22, 2023
@Iceber Iceber added the kind/feature New feature label Nov 23, 2023
Copy link
Member

@Iceber Iceber left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks for your contribution

1. added new `whereSQLStmt` for parameterized query statement
2. added new `whereSQLParams` for parameterized query parameters
3. added new `whereSQLJSONParams` for parameterized query parameters in JSON format with base64 encoding

Signed-off-by: Neko Ayaka <neko@ayaka.moe>
Signed-off-by: Neko Ayaka <neko@ayaka.moe>
@Iceber Iceber merged commit f3e50db into clusterpedia-io:main Nov 27, 2023
7 checks passed
@Iceber Iceber added the cherry-pick/0.7.x Change to be cherry picked to release/0.7 branch label Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick/0.7.x Change to be cherry picked to release/0.7 branch kind/bug Something isn't working kind/feature New feature size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants