Skip to content

*: refactor the RestrictedSQLExecutor interface (#22579)#22687

Merged
tiancaiamao merged 4 commits intopingcap:release-5.0-rcfrom
ti-srebot:release-5.0-rc-ea6ccf82e934
Feb 22, 2021
Merged

*: refactor the RestrictedSQLExecutor interface (#22579)#22687
tiancaiamao merged 4 commits intopingcap:release-5.0-rcfrom
ti-srebot:release-5.0-rc-ea6ccf82e934

Conversation

@ti-srebot
Copy link
Contributor

@ti-srebot ti-srebot commented Feb 3, 2021

cherry-pick #22579 to release-5.0-rc
You can switch your code base to this Pull Request by using git-extras:

# In tidb repo:
git pr https://github.com/pingcap/tidb/pull/22687

After apply modifications, you can push your change to this PR via:

git push git@github.com:ti-srebot/tidb.git pr/22687:release-5.0-rc-ea6ccf82e934

What problem does this PR solve?

Problem Summary:

Improve security for our code.

What is changed and how it works?

The initial definition of RestrictedSQLExecutor looks like this:

type RestrictedSQLExecutor interface {
	// ExecRestrictedSQL run sql statement in ctx with some restriction.
	ExecRestrictedSQL(sql string) ([]chunk.Row, []*ast.ResultField, error)
}

It's not good for security reasons, the usage pattern looks like this:

sql  = fmt.Sprintf('select xxx from ..%s ', varFromOutside)
stmt, err = ExecRestrictedSQL(sql) 
...

Later on, we add more methods to the interface which IMO, ugly:

	// ExecRestrictedSQLWithContext run sql statement in ctx with some restriction.
	ExecRestrictedSQLWithContext(ctx context.Context, sql string, opts ...OptionFuncAlias) ([]chunk.Row, []*ast.ResultField, error)
	// ExecRestrictedSQLWithSnapshot run sql statement in ctx with some restriction and with snapshot.
	// If current session sets the snapshot timestamp, then execute with this snapshot timestamp.
	// Otherwise, execute with the current transaction start timestamp if the transaction is valid.
	ExecRestrictedSQLWithSnapshot(sql string) ([]chunk.Row, []*ast.ResultField, error)

I propose to use this one as its new definition:

type RestrictedSQLExecutor interface {
	ParseWithParams(ctx context.Context, sql string, args ...interface{}) (ast.StmtNode, error)
	ExecRestrictedStmt(ctx context.Context, stmt ast.StmtNode, opts ...OptionFuncAlias) ([]chunk.Row, []*ast.ResultField, error)
}

This interface is secure, and easy to use:

stmt  = exec.ParseWithParams('select xxx from ..%? ', varFromOutside)
res, err = ExecRestrictedSQL(stmt) 
...

What's Changed:

ParseWithParams() now returns ast.StmtNode rather than []ast.StmtNode, multiple statements are not allowed here.

Update the "util", "store", and "domain" packages, ensure they use the new (secure) API.

How it Works:

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • No code

Side effects

  • Breaking backward compatibility
    API change

Release note

  • No release note

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot requested a review from a team as a code owner February 3, 2021 04:39
@ti-srebot ti-srebot requested review from qw4990 and removed request for a team February 3, 2021 04:39
@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@tiancaiamao you're already a collaborator in bot's repo.

@bb7133
Copy link
Member

bb7133 commented Feb 4, 2021

Please try to resolve the conflicts @tiancaiamao

Copy link
Contributor

@xhebox xhebox left a comment

Choose a reason for hiding this comment

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

Rest LGTM

if len(rs) > 0 {
defer terror.Call(rs[0].Close)
}
defer terror.Call(rs.Close)
Copy link
Contributor

@xhebox xhebox Feb 22, 2021

Choose a reason for hiding this comment

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

This defer should be moved after the err != nil check

Copy link
Contributor

@xhebox xhebox left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 22, 2021
Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Feb 22, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Feb 22, 2021
@bb7133
Copy link
Member

bb7133 commented Feb 22, 2021

/mergte

@bb7133
Copy link
Member

bb7133 commented Feb 22, 2021

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 22, 2021
@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@ti-srebot merge failed.

@tiancaiamao
Copy link
Contributor

/run-unit-test

@tiancaiamao tiancaiamao merged commit 2e791d4 into pingcap:release-5.0-rc Feb 22, 2021
@tiancaiamao tiancaiamao deleted the release-5.0-rc-ea6ccf82e934 branch February 22, 2021 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/session component/store component/util sig/execution SIG execution sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants