-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Conversation
@heyitsanthony @jxuan I see, the possible inconsistent state (the diverged bcrypt costs) is similar to #9475 . How about adding a new option for configuring the cost to |
@mitake I checked the bcrypt code and the problem I was worried about isn't an issue. It can check |
@heyitsanthony Ah I see. Then the inconsistent state is acceptable. Thanks for checking! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes are good, thanks! Could you fix the trivial problem I pointed out? Also please fix the commit title for following the standard style e.g. *: make bcrypt cost configurable
embed/serve_test.go
Outdated
@@ -36,3 +37,33 @@ func TestStartEtcdWrongToken(t *testing.T) { | |||
t.Fatalf("expected %v, got %v", auth.ErrInvalidAuthOpts, err) | |||
} | |||
} | |||
|
|||
// TestStartEtcdLargeBcryptCost ensures that StartEtcd with good configs returns with error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trivial nit: good configs
should be bad configs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
@mitake the code is updated based on your feedback. Please check @heyitsanthony Thank you for checking. |
@mitake The CI errors are not related to my code change. I ran those tests manually on my mac and they all pass. Anyway to re-trigger these tests? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please reformat the git commit title to *: make bcrypt ...
. And rebase from current master branch. Defer to @mitake @heyitsanthony
auth/store.go
Outdated
plog.Errorf("Invalid bcrypt-cost: %d", bcryptCost) | ||
return nil, ErrInvalidAuthOpts | ||
} | ||
BcryptCost = bcryptCost |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
auth/store.go
Outdated
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) |
There was a problem hiding this comment.
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),
)
}
embed/serve_test.go
Outdated
@@ -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. |
There was a problem hiding this comment.
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.
etcdmain/config.go
Outdated
@@ -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") |
There was a problem hiding this comment.
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."
etcdmain/help.go
Outdated
@@ -148,6 +148,8 @@ Security: | |||
Auth: | |||
--auth-token 'simple' | |||
Specify a v3 authentication token type and its options ('simple' or 'jwt'). | |||
--bcrypt-cost 10 |
There was a problem hiding this comment.
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) +
?
tests/e2e/cluster_test.go
Outdated
@@ -123,6 +123,7 @@ type etcdProcessClusterConfig struct { | |||
noStrictReconfig bool | |||
initialCorruptCheck bool | |||
authTokenOpts string | |||
bcryptCost int |
There was a problem hiding this comment.
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
.
integration/cluster.go
Outdated
@@ -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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. Fixed in new version.
auth/store.go
Outdated
plog.Errorf("Invalid bcrypt-cost: %d", bcryptCost) | ||
return nil, ErrInvalidAuthOpts | ||
} | ||
BcryptCost = bcryptCost |
There was a problem hiding this comment.
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.
@jxuan Can you squash commits? Or separate per package levels? |
Sure. Once all tests pass, I will squash everything into one commit. |
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
*: highlight "--log-package-levels" deprecation in v3.5
Documentation: binding listeners must be IP.
@jxuan |
To squash commits, do you recommend starting a new branch? |
@jxuan You can just cherry-pick commits to your local branch and force push. Or just keep rebasing from latest master branch, and force push to your local branch. We don't include merge commits. |
Right now it only fails on the title format check 'commit_title' started at Wed May 2 21:50:52 UTC 2018 Can you start to review to code and see if there is anything else that you would like to change? In the mean time, I am working on a squashed version for submission. Thanks, |
I created a squashed version in #9687 |
This change is to implement the feature request in #9615