Token signing key is now randomly generated on demand if not already present #135
Conversation
auth/token.go
Outdated
| TokenSigningKey = "ccn$!~56" | ||
| // this is the datastore key, not the actual bytes used for signing. | ||
| // since changes are not handled by an abstraction layer, construct the path manually | ||
| tokenSigningKey = types.AuthProxyDir + "/token_signing_key" |
There was a problem hiding this comment.
This the place we maintain all the datastore paths https://github.com/contiv/auth_proxy/blob/master/db/constants.go. And, we use GetPath to get the absolute datastore path like here https://github.com/contiv/auth_proxy/blob/master/db/local.go#L61
I recommend to follow the same style.
auth/token.go
Outdated
|
|
||
| var tokenCharacters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890") | ||
|
|
||
| func generateTokenSigningKey() string { |
There was a problem hiding this comment.
What do you think of following approach 3 which seems to be fast http://stackoverflow.com/questions/22892120/how-to-generate-a-random-string-of-a-fixed-length-in-golang
And, this func can be named generateRandString. Yay??
There was a problem hiding this comment.
I don't think there's any value in extracting this into a reusable function right now. The valid characters are also specific to the token signing key.
This function is being called extremely infrequently. The cumulative CPU time of everyone who will ever have this code execute on their systems would never add up to the time it would take to replace it with an optimized version. The third option there also doesn't have the same probability for all characters to be chosen.
auth/token.go
Outdated
| defer signingKeyMutex.Unlock() | ||
|
|
||
| // check for an existing existing key | ||
| existingKey, err := stateDrv.Read(tokenSigningKey) |
There was a problem hiding this comment.
I feel this is more clear this way
switch(err) {
case err == auth_errors.ErrKeyNotFound:
case err == auth_errors.ErrKeyExists:
default:
return "", err
}
common/crypto.go
Outdated
| @@ -24,6 +27,10 @@ const ( | |||
| cost = 13 | |||
| ) | |||
|
|
|||
| func init() { | |||
There was a problem hiding this comment.
This package was calling crypto-related stuff without a guaranteed-to-be-seeded PRNG
|
PTAL again |
|
Please add test so we can be sure that this works fine. |
dseevr
force-pushed
the
signing_key
branch
4 times, most recently
from
e397c6b to
ee836c6
Compare
systemtests/auth_test.go
Outdated
|
|
||
| // make sure the old token does not work | ||
| resp, _ = proxyGet(c, firstToken, endpoint) | ||
| c.Assert(resp.StatusCode, Equals, 400) |
There was a problem hiding this comment.
Please check the body to be Bad Token
There was a problem hiding this comment.
Done, but this didn't get collapsed because I didn't edit this line
…present Additionally, ensure that the PRNG has been seeded Signed-off-by: Bill Robinson <[email protected]>
…han calling `make` from test code Test code runs as root in the docker containers and this results in a root-owned "local_certs" directory Signed-off-by: Bill Robinson <[email protected]>
* sidebarmenu active menu items highlighted. * sidebarmenu item being highlighted * Updated code comments
Additionally, ensure that the PRNG has been seeded
Signed-off-by: Bill Robinson [email protected]