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

*: hide secure information in SHOW PROCESSLIST #4451

Merged
merged 9 commits into from
Sep 12, 2017
2 changes: 2 additions & 0 deletions ast/ast.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ type Node interface {
Text() string
// SetText sets original text to the Node.
SetText(text string)
// SecureText is different from Text that it hide password information.
SecureText() string
Copy link
Member

Choose a reason for hiding this comment

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

I think this method should not be put in Node interface.
How about defining a CrusialStmt interface which only has a SecureText method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we implement SecureText() for so many

*ast.CreateUserStmt, *ast.DropUserStmt, *ast.AlterUserStmt, *ast.SetPwdStmt, *ast.GrantStmt,
*ast.RevokeStmt, *ast.AlterTableStmt, *ast.CreateDatabaseStmt, *ast.CreateIndexStmt, *ast.CreateTableStmt,
*ast.DropDatabaseStmt, *ast.DropIndexStmt, *ast.DropTableStmt, *ast.RenameTableStmt, *ast.TruncateTableStmt:

and most of those implemention is just ... ?

func (x *XXX) SecureText() string {
    return x.Text()
}

Copy link
Member

Choose a reason for hiding this comment

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

Well, not all crucial statement need to have secure text.
Then how about define a SensitiveStatement which has SecureText?

Copy link
Member

@coocood coocood Sep 6, 2017

Choose a reason for hiding this comment

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

if ss, ok := node.(SensitiveStmt); ok {
    text = ss.SecureText()
} else {
    text = node.Text()
}

Copy link
Member

Choose a reason for hiding this comment

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

Could we add SecureText to StmtNode interface?

Copy link
Member

Choose a reason for hiding this comment

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

@shenli
SecureText only needed by 4 statements, I think adding it to all StmtNode is not worthy.

Copy link
Member

Choose a reason for hiding this comment

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

ok

}

// Flags indicates whether an expression contains certain types of expression.
Expand Down
5 changes: 5 additions & 0 deletions ast/base.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ func (n *node) Text() string {
return n.text
}

// SecureText implements Node interface.
func (n *node) SecureText() string {
return n.text
}

// stmtNode implements StmtNode interface.
// Statement implementations should embed it in.
type stmtNode struct {
Expand Down
40 changes: 40 additions & 0 deletions ast/misc.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@
package ast

import (
"bytes"
"fmt"
"strings"

"github.com/pingcap/tidb/context"
"github.com/pingcap/tidb/model"
Expand Down Expand Up @@ -406,6 +408,11 @@ type SetPwdStmt struct {
Password string
}

// SecureString overloads Node interface method.
func (n *SetPwdStmt) SecureString() string {
Copy link
Member

Choose a reason for hiding this comment

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

SecureText or SecureString?

return fmt.Sprintf("set password for user %s", n.User)
}

// Accept implements Node Accept interface.
func (n *SetPwdStmt) Accept(v Visitor) (Node, bool) {
newNode, skipChildren := v.Enter(n)
Expand Down Expand Up @@ -455,6 +462,17 @@ func (n *CreateUserStmt) Accept(v Visitor) (Node, bool) {
return v.Leave(n)
}

// SecureString overloads Node interface method.
func (n *CreateUserStmt) SecureString() string {
var buf bytes.Buffer
buf.WriteString("create user")
for _, user := range n.Specs {
buf.WriteString(" ")
buf.WriteString(user.SecurityString())
}
return buf.String()
}

// AlterUserStmt modifies user account.
// See https://dev.mysql.com/doc/refman/5.7/en/alter-user.html
type AlterUserStmt struct {
Expand All @@ -465,6 +483,17 @@ type AlterUserStmt struct {
Specs []*UserSpec
}

// SecureString overloads Node interface method.
func (n *AlterUserStmt) SecureString() string {
var buf bytes.Buffer
buf.WriteString("alter user")
for _, user := range n.Specs {
buf.WriteString(" ")
buf.WriteString(user.SecurityString())
}
return buf.String()
}

// Accept implements Node Accept interface.
func (n *AlterUserStmt) Accept(v Visitor) (Node, bool) {
newNode, skipChildren := v.Enter(n)
Expand Down Expand Up @@ -649,6 +678,17 @@ type GrantStmt struct {
WithGrant bool
}

// SecureString overloads Node interface method.
func (n *GrantStmt) SecureString() string {
text := n.text
// Filter "identified by xxx" because it would expose password information.
idx := strings.Index(strings.ToLower(text), "identified")
if idx > 0 {
text = text[:idx]
}
return text
}

// Accept implements Node Accept interface.
func (n *GrantStmt) Accept(v Visitor) (Node, bool) {
newNode, skipChildren := v.Enter(n)
Expand Down
7 changes: 6 additions & 1 deletion executor/adapter.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,13 @@ func (a *statement) Exec(ctx context.Context) (ast.RecordSet, error) {
var pi processinfoSetter
if raw, ok := ctx.(processinfoSetter); ok {
pi = raw
sql := a.OriginText()
if simple, ok := a.plan.(*plan.Simple); ok && simple.Statement != nil {
// Use SecureString to avoid leak password information.
sql = simple.Statement.SecureText()
}
// Update processinfo, ShowProcess() will use it.
pi.SetProcessInfo(a.OriginText())
pi.SetProcessInfo(sql)
}
// Fields or Schema are only used for statements that return result set.
if e.Schema().Len() == 0 {
Expand Down
27 changes: 3 additions & 24 deletions session.go
Original file line number Diff line number Diff line change
Expand Up @@ -1247,30 +1247,9 @@ func (s *session) ShowProcess() util.ProcessInfo {
// logCrucialStmt logs some crucial SQL including: CREATE USER/GRANT PRIVILEGE/CHANGE PASSWORD/DDL etc.
func logCrucialStmt(node ast.StmtNode) {
switch stmt := node.(type) {
case *ast.CreateUserStmt:
for _, user := range stmt.Specs {
log.Infof("[CRUCIAL OPERATION] create user %s.", user.SecurityString())
}
case *ast.DropUserStmt:
log.Infof("[CRUCIAL OPERATION] drop user %v.", stmt.UserList)
case *ast.AlterUserStmt:
for _, user := range stmt.Specs {
log.Infof("[CRUCIAL OPERATION] alter user %s.", user.SecurityString())
}
case *ast.SetPwdStmt:
log.Infof("[CRUCIAL OPERATION] set password for user %s.", stmt.User)
case *ast.GrantStmt:
text := stmt.Text()
// Filter "identified by xxx" because it would expose password information.
idx := strings.Index(strings.ToLower(text), "identified")
if idx > 0 {
text = text[:idx]
}
log.Infof("[CRUCIAL OPERATION] %s.", text)
case *ast.RevokeStmt:
log.Infof("[CRUCIAL OPERATION] %s.", stmt.Text())
case *ast.AlterTableStmt, *ast.CreateDatabaseStmt, *ast.CreateIndexStmt, *ast.CreateTableStmt,
case *ast.CreateUserStmt, *ast.DropUserStmt, *ast.AlterUserStmt, *ast.SetPwdStmt, *ast.GrantStmt,
*ast.RevokeStmt, *ast.AlterTableStmt, *ast.CreateDatabaseStmt, *ast.CreateIndexStmt, *ast.CreateTableStmt,
*ast.DropDatabaseStmt, *ast.DropIndexStmt, *ast.DropTableStmt, *ast.RenameTableStmt, *ast.TruncateTableStmt:
log.Infof("[CRUCIAL OPERATION] %s.", stmt.Text())
log.Infof("[CRUCIAL OPERATION] %s.", stmt.SecureText())
}
}