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

*: make bcrypt-cost configurable #9633

Closed
wants to merge 25 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
334263c
Make bcrypt-cost configurable
Apr 26, 2018
ea3b601
*: Update based on CR comments
Apr 26, 2018
d18422e
*: add missing line in config.go for bcrypt-cost
Apr 26, 2018
9005333
Remove comments to trigger rebuild
Apr 26, 2018
d70369e
Move testcases and address comments
Apr 30, 2018
e159331
Merge branch 'master' into bcrypt-cost-config
Apr 30, 2018
94dd681
*: remove duplicates in Documentation/op-guide/configuration.md
Apr 30, 2018
3f23c99
merge
Apr 30, 2018
596e9c0
*: move bcryptCost into authStore
May 1, 2018
5b51eb9
Fix help message and trigger rebuild
May 1, 2018
21fdde5
CHANGELOG-3.5: deprecate "--log-package-levels" in v3.5
gyuho May 1, 2018
9e7a3e5
Documentation/upgrades: highlight "--log-package-levels"
gyuho May 1, 2018
d80ef1f
etcd.conf.yml.sample: remove "--log-package-levels"
gyuho May 1, 2018
2afd827
Documentation: binding listeners must be IP.
hexfusion Apr 26, 2018
5e06955
*: update bcrypt cost for integration tests
May 2, 2018
9e1b58f
Merge branch 'master' into bcrypt-cost-config
May 2, 2018
53f51b2
Merge pull request #9676 from gyuho/logger
gyuho May 2, 2018
859172e
Merge pull request #9638 from hexfusion/df
gyuho May 2, 2018
8893c66
*: make bcrypt-cost configurable (squashed)
May 2, 2018
ad1be4e
Merge branch 'bcrypt-cost-config' of github.com:jxuan/etcd into bcryp…
May 2, 2018
e1d05d0
Fix format
May 2, 2018
d923b1d
*: make bcrypt-cost configurable (squashed)
May 2, 2018
c16a8d9
Merge branch 'bcrypt-cost-config' of github.com:jxuan/etcd into bcryp…
May 2, 2018
afcf163
*: make bcrypt-cost configurable (squashed)
May 2, 2018
7328865
*: use proper constant for help message
May 2, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Documentation/op-guide/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,10 @@ Follow the instructions when using these flags.
+ Example option of JWT: '--auth-token jwt,pub-key=app.rsa.pub,priv-key=app.rsa,sign-method=RS512,ttl=10m'
+ default: "simple"

### --bcrypt-cost
+ Specify the cost / strength of the bcrypt algorithm for hashing auth passwords. Valid values are between 4 and 31.
+ default: 10

## Experimental flags

### --experimental-corrupt-check-time
Expand Down
8 changes: 7 additions & 1 deletion auth/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -1048,12 +1048,18 @@ func decomposeOpts(optstr string) (string, map[string]string, error) {

}

func NewTokenProvider(lg *zap.Logger, tokenOpts string, indexWaiter func(uint64) <-chan struct{}) (TokenProvider, error) {
func NewTokenProvider(lg *zap.Logger, tokenOpts string, bcryptCost int, indexWaiter func(uint64) <-chan struct{}) (TokenProvider, error) {
tokenType, typeSpecificOpts, err := decomposeOpts(tokenOpts)
if err != nil {
return nil, ErrInvalidAuthOpts
}

if bcryptCost < bcrypt.MinCost || bcryptCost > bcrypt.MaxCost {
plog.Errorf("Invalid bcrypt-cost: %d", bcryptCost)
Copy link
Contributor

Choose a reason for hiding this comment

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

if lg != nil {
  lg.Warn(
    "invalid bcrypt cost",
    zap.Int("min-cost", bcrypt.MinCost),
    zap.Int("max-cost", bcrypt.MaxCost),
    zap.Int("given-cost", bcryptCost),
  )
}

return nil, ErrInvalidAuthOpts
}
BcryptCost = bcryptCost
Copy link
Contributor

Choose a reason for hiding this comment

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

auth should pass around bcyptCost values rather than sharing a global BcryptCost (racey as CI fails).

Copy link
Author

Choose a reason for hiding this comment

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

Will make it private. BcryptCost is only referenced in clientv3/main_test.go and clientv3/main_test.go does not trigger anything related to bcrypt.


switch tokenType {
case "simple":
if lg != nil {
Expand Down
14 changes: 6 additions & 8 deletions auth/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ import (
"google.golang.org/grpc/metadata"
)

func init() { BcryptCost = bcrypt.MinCost }

func dummyIndexWaiter(index uint64) <-chan struct{} {
ch := make(chan struct{})
go func() {
Expand All @@ -50,7 +48,7 @@ func TestNewAuthStoreRevision(t *testing.T) {
b, tPath := backend.NewDefaultTmpBackend()
defer os.Remove(tPath)

tp, err := NewTokenProvider(zap.NewExample(), "simple", dummyIndexWaiter)
tp, err := NewTokenProvider(zap.NewExample(), "simple", bcrypt.MinCost, dummyIndexWaiter)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -78,7 +76,7 @@ func TestNewAuthStoreRevision(t *testing.T) {
func setupAuthStore(t *testing.T) (store *authStore, teardownfunc func(t *testing.T)) {
b, tPath := backend.NewDefaultTmpBackend()

tp, err := NewTokenProvider(zap.NewExample(), "simple", dummyIndexWaiter)
tp, err := NewTokenProvider(zap.NewExample(), "simple", bcrypt.MinCost, dummyIndexWaiter)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -515,7 +513,7 @@ func TestAuthInfoFromCtxRace(t *testing.T) {
b, tPath := backend.NewDefaultTmpBackend()
defer os.Remove(tPath)

tp, err := NewTokenProvider(zap.NewExample(), "simple", dummyIndexWaiter)
tp, err := NewTokenProvider(zap.NewExample(), "simple", bcrypt.MinCost, dummyIndexWaiter)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -581,7 +579,7 @@ func TestRecoverFromSnapshot(t *testing.T) {

as.Close()

tp, err := NewTokenProvider(zap.NewExample(), "simple", dummyIndexWaiter)
tp, err := NewTokenProvider(zap.NewExample(), "simple", bcrypt.MinCost, dummyIndexWaiter)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -663,7 +661,7 @@ func TestRolesOrder(t *testing.T) {
b, tPath := backend.NewDefaultTmpBackend()
defer os.Remove(tPath)

tp, err := NewTokenProvider(zap.NewExample(), "simple", dummyIndexWaiter)
tp, err := NewTokenProvider(zap.NewExample(), "simple", bcrypt.MinCost, dummyIndexWaiter)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -709,7 +707,7 @@ func TestAuthInfoFromCtxWithRoot(t *testing.T) {
b, tPath := backend.NewDefaultTmpBackend()
defer os.Remove(tPath)

tp, err := NewTokenProvider(zap.NewExample(), "simple", dummyIndexWaiter)
tp, err := NewTokenProvider(zap.NewExample(), "simple", bcrypt.MinCost, dummyIndexWaiter)
if err != nil {
t.Fatal(err)
}
Expand Down
7 changes: 5 additions & 2 deletions embed/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import (
"github.com/ghodss/yaml"
"go.uber.org/zap"
"go.uber.org/zap/zapcore"
"golang.org/x/crypto/bcrypt"
"google.golang.org/grpc"
"google.golang.org/grpc/grpclog"
)
Expand Down Expand Up @@ -249,7 +250,8 @@ type Config struct {
// embed.StartEtcd(cfg)
ServiceRegister func(*grpc.Server) `json:"-"`

AuthToken string `json:"auth-token"`
AuthToken string `json:"auth-token"`
BcryptCost uint `json:"bcrypt-cost"`

ExperimentalInitialCorruptCheck bool `json:"experimental-initial-corrupt-check"`
ExperimentalCorruptCheckTime time.Duration `json:"experimental-corrupt-check-time"`
Expand Down Expand Up @@ -370,7 +372,8 @@ func NewConfig() *Config {
CORS: map[string]struct{}{"*": {}},
HostWhitelist: map[string]struct{}{"*": {}},

AuthToken: "simple",
AuthToken: "simple",
BcryptCost: uint(bcrypt.DefaultCost),

PreVote: false, // TODO: enable by default in v3.5

Expand Down
1 change: 1 addition & 0 deletions embed/etcd.go
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ func StartEtcd(inCfg *Config) (e *Etcd, err error) {
StrictReconfigCheck: cfg.StrictReconfigCheck,
ClientCertAuthEnabled: cfg.ClientTLSInfo.ClientCertAuth,
AuthToken: cfg.AuthToken,
BcryptCost: cfg.BcryptCost,
CORS: cfg.CORS,
HostWhitelist: cfg.HostWhitelist,
InitialCorruptCheck: cfg.ExperimentalInitialCorruptCheck,
Expand Down
31 changes: 31 additions & 0 deletions embed/serve_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"testing"

"github.com/coreos/etcd/auth"
"golang.org/x/crypto/bcrypt"
)

// TestStartEtcdWrongToken ensures that StartEtcd with wrong configs returns with error.
Expand All @@ -36,3 +37,33 @@ func TestStartEtcdWrongToken(t *testing.T) {
t.Fatalf("expected %v, got %v", auth.ErrInvalidAuthOpts, err)
}
}

// TestStartEtcdLargeBcryptCost ensures that StartEtcd with invalid large bcrypt-cost returns with error.
Copy link
Contributor

Choose a reason for hiding this comment

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

Tests in embed are no needed. Just unit tests around NewTokenProvider should be enough.

func TestStartEtcdLargeBcryptCost(t *testing.T) {
tdir, err := ioutil.TempDir(os.TempDir(), "large-bcrypt-cost-test")
if err != nil {
t.Fatal(err)
}
defer os.RemoveAll(tdir)
cfg := NewConfig()
cfg.Dir = tdir
cfg.BcryptCost = uint(bcrypt.MaxCost) + 1
if _, err = StartEtcd(cfg); err != auth.ErrInvalidAuthOpts {
t.Fatalf("expected %v, got %v", auth.ErrInvalidAuthOpts, err)
}
}

// TestStartEtcdSmallBcryptCost ensures that StartEtcd with invalid small bcrypt-cost returns with error.
func TestStartEtcdSmallBcryptCost(t *testing.T) {
tdir, err := ioutil.TempDir(os.TempDir(), "small-bcrypt-cost-test")
if err != nil {
t.Fatal(err)
}
defer os.RemoveAll(tdir)
cfg := NewConfig()
cfg.Dir = tdir
cfg.BcryptCost = uint(bcrypt.MinCost) - 1
if _, err = StartEtcd(cfg); err != auth.ErrInvalidAuthOpts {
t.Fatalf("expected %v, got %v", auth.ErrInvalidAuthOpts, err)
}
}
1 change: 1 addition & 0 deletions etcdmain/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ func newConfig() *config {

// auth
fs.StringVar(&cfg.ec.AuthToken, "auth-token", cfg.ec.AuthToken, "Specify auth token specific options.")
fs.UintVar(&cfg.ec.BcryptCost, "bcrypt-cost", cfg.ec.BcryptCost, "Bcrypt algorithm cost / strength for hashing auth passwords")
Copy link
Contributor

Choose a reason for hiding this comment

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

"Specify bcrypt algorithm cost factor for auth password hashing."


// experimental
fs.BoolVar(&cfg.ec.ExperimentalInitialCorruptCheck, "experimental-initial-corrupt-check", cfg.ec.ExperimentalInitialCorruptCheck, "Enable to check data corruption before serving any client/peer traffic.")
Expand Down
2 changes: 2 additions & 0 deletions etcdmain/help.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,8 @@ Security:
Auth:
--auth-token 'simple'
Specify a v3 authentication token type and its options ('simple' or 'jwt').
--bcrypt-cost 10
Copy link
Contributor

Choose a reason for hiding this comment

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

+ fmt.Sprintf("%d", bcrypt.DefaultCost) +?

Specify the cost / strength of the bcrypt algorithm for hashing auth passwords. Valid values are between 4 and 31.

Profiling:
--enable-pprof 'false'
Expand Down
3 changes: 2 additions & 1 deletion etcdserver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,8 @@ type ServerConfig struct {
// ClientCertAuthEnabled is true when cert has been signed by the client CA.
ClientCertAuthEnabled bool

AuthToken string
AuthToken string
BcryptCost uint

// InitialCorruptCheck is true to check data corruption on boot
// before serving any peer/client traffic.
Expand Down
2 changes: 1 addition & 1 deletion etcdserver/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@ func NewServer(cfg ServerConfig) (srv *EtcdServer, err error) {
}()

srv.consistIndex.setConsistentIndex(srv.kv.ConsistentIndex())
tp, err := auth.NewTokenProvider(cfg.Logger, cfg.AuthToken,
tp, err := auth.NewTokenProvider(cfg.Logger, cfg.AuthToken, int(cfg.BcryptCost),
func(index uint64) <-chan struct{} {
return srv.applyWait.Wait(index)
},
Expand Down
2 changes: 2 additions & 0 deletions integration/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import (

"github.com/coreos/pkg/capnslog"
"github.com/soheilhy/cmux"
"golang.org/x/crypto/bcrypt"
"google.golang.org/grpc"
"google.golang.org/grpc/grpclog"
"google.golang.org/grpc/keepalive"
Expand Down Expand Up @@ -605,6 +606,7 @@ func mustNewMember(t *testing.T, mcfg memberConfig) *member {
m.MaxRequestBytes = embed.DefaultMaxRequestBytes
}
m.AuthToken = "simple" // for the purpose of integration testing, simple token is enough
m.BcryptCost = uint(bcrypt.MinCost)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed when we have unit tests inside auth package.


m.grpcServerOpts = []grpc.ServerOption{}
if mcfg.grpcKeepAliveMinTime > time.Duration(0) {
Expand Down
7 changes: 7 additions & 0 deletions tests/e2e/cluster_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ type etcdProcessClusterConfig struct {
noStrictReconfig bool
initialCorruptCheck bool
authTokenOpts string
bcryptCost int
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove changes from tests/e2e.

}

// newEtcdProcessCluster launches a new cluster from etcd processes, returning
Expand Down Expand Up @@ -249,6 +250,12 @@ func (cfg *etcdProcessClusterConfig) etcdServerProcessConfigs() []*etcdServerPro
args = append(args, "--auth-token", cfg.authTokenOpts)
}

if cfg.bcryptCost > 0 {
args = append(args,
"--bcrypt-cost", fmt.Sprintf("%d", cfg.bcryptCost),
)
}

etcdCfgs[i] = &etcdServerProcessConfig{
execPath: cfg.execPath,
args: args,
Expand Down