-
Notifications
You must be signed in to change notification settings - Fork 705
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
Added support for jwt secret creation of each user upon user login #4719
Conversation
Signed-off-by: Saranya-jena <[email protected]>
// UpdateUser updates user details in the database | ||
func (r repository) UpdateUser(user *entities.UserDetails) error { | ||
data, _ := toDoc(user) | ||
_, err := r.Collection.UpdateOne(context.Background(), bson.M{"_id": user.ID}, bson.M{"$set": data}) | ||
_, err := r.Collection.UpdateMany(context.Background(), bson.M{"_id": user.ID}, bson.M{"$set": data}) |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
user-provided value
Signed-off-by: Saranya-jena <[email protected]>
Signed-off-by: Saranya-jena <[email protected]>
Signed-off-by: Saranya-jena <[email protected]>
salt := utils.RandomString(6) | ||
newHashedSalt, err := bcrypt.GenerateFromPassword([]byte(salt), utils.PasswordEncryptionCost) | ||
if err != nil { | ||
log.Error(err) | ||
c.JSON(utils.ErrorStatusCodes[utils.ErrServerError], presenter.CreateErrorResponse(utils.ErrServerError)) | ||
return | ||
} | ||
|
||
err = service.UpdateUserByQuery(bson.D{ | ||
{"user_id", user.ID}, | ||
}, bson.D{ | ||
{"$set", bson.D{ | ||
{"salt", string(newHashedSalt)}, | ||
}}, | ||
}) | ||
if err != nil { | ||
log.Error(err) | ||
c.JSON(utils.ErrorStatusCodes[utils.ErrServerError], presenter.CreateErrorResponse(utils.ErrServerError)) | ||
return | ||
} |
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.
Can you please make this a new function which can be reused in both the places
@@ -206,10 +207,20 @@ func (r repository) CreateUser(user *entities.User) (*entities.User, error) { | |||
return user.SanitizedUser(), nil | |||
} | |||
|
|||
// UpdateUserByQuery updates user details in the database | |||
func (r repository) UpdateUserByQuery(filter bson.D, updateQuery bson.D) error { | |||
_, err := r.Collection.UpdateMany(context.Background(), filter, updateQuery) |
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.
This should be just Update not UpdateMany
func RandomString(n int) string { | ||
if n > 0 { | ||
var letters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-") | ||
rand.Seed(time.Now().UnixNano()) |
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.
this will give the same seed value for a given input which can be easy to guess, how about using crypto/rand?
Signed-off-by: Saranya-jena <[email protected]>
Signed-off-by: Saranya-jena <[email protected]>
Signed-off-by: Saranya-jena <[email protected]>
Signed-off-by: Saranya-jena <[email protected]>
Signed-off-by: Saranya-jena <[email protected]>
Signed-off-by: Saranya-jena <[email protected]>
return a.authConfigRepo.GetConfig(key) | ||
} | ||
|
||
func (a applicationService) UpdateConfig(ctx context.Context, key string, value interface{}) 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.
I don't think this will be ever used
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.
Have kept it for future use in case we need to add more such cases
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.
We can always add it back if needed. As they say "developers spend more time reading the code than writing". We can keep it if you can think of any use case in the future where this can be utilised, but if not then this can be removed.
Collection *mongo.Collection | ||
} | ||
|
||
func (r repository) CreateConfig(config AuthConfig) 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.
How about renaming this to CreateSalt and GetSalt, code will be more readable
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.
As mentioned above, if you look at the schema it's like key value pair, which can be used to store different types od data so to generalise it, I have given such naming.
package response | ||
|
||
import ( | ||
authConfig2 "github.com/litmuschaos/litmus/chaoscenter/authentication/pkg/authConfig" |
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.
No need to add alias to this import
@@ -138,7 +138,20 @@ func DexCallback(userService services.ApplicationService) gin.HandlerFunc { | |||
c.JSON(utils.ErrorStatusCodes[utils.ErrServerError], presenter.CreateErrorResponse(utils.ErrServerError)) | |||
return | |||
} | |||
jwtToken, err := userService.GetSignedJWT(signedInUser) | |||
|
|||
salt, err := userService.GetConfig("salt") |
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 salt value being returned here is an encrypted value and should be decrypted before using it to generate the JWT
@@ -294,7 +293,13 @@ func LoginUser(service services.ApplicationService) gin.HandlerFunc { | |||
return | |||
} | |||
|
|||
token, err := service.GetSignedJWT(user) | |||
salt, err := service.GetConfig("salt") |
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.
Same as above
Signed-off-by: Saranya-jena <[email protected]>
Signed-off-by: Saranya-jena <[email protected]>
@@ -294,7 +293,13 @@ func LoginUser(service services.ApplicationService) gin.HandlerFunc { | |||
return | |||
} | |||
|
|||
token, err := service.GetSignedJWT(user) | |||
salt, err := service.GetConfig("salt") |
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.
Can you create a constant for the "salt"?
encodedSalt := base64.StdEncoding.EncodeToString([]byte(salt)) | ||
|
||
config := authConfig.AuthConfig{ | ||
Key: "salt", |
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.
Use the constant here.
@@ -90,7 +95,12 @@ func (a applicationService) CreateApiToken(user *entities.User, request entities | |||
claims["username"] = user.Username | |||
claims["exp"] = expiresAt | |||
|
|||
tokenString, err := token.SignedString([]byte(utils.JwtSecret)) | |||
salt, err := a.GetConfig("salt") |
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.
same
|
||
func (m *MongoOperations) GetAuthConfig(ctx context.Context, key string) (*AuthConfig, error) { | ||
|
||
authDb := MgoClient.Database("auth") |
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.
From constants?
} | ||
|
||
func (c *Operator) GetAuthConfig(ctx context.Context) (*mongodb.AuthConfig, error) { | ||
salt, err := c.operator.GetAuthConfig(ctx, "salt") |
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.
constant
Signed-off-by: Saranya-jena <[email protected]>
Signed-off-by: Saranya-jena <[email protected]>
Signed-off-by: Saranya-jena <[email protected]>
Signed-off-by: Saranya-jena <[email protected]>
chaoscenter/authentication/api/handlers/rest/dex_auth_handler.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Saranya-jena <[email protected]>
Signed-off-by: Saranya-jena <[email protected]>
Signed-off-by: Saranya-jena <[email protected]>
ef0c21a
to
228a255
Compare
…itmuschaos#4719) * Added support for jwt secret creation of each user upon logic Signed-off-by: Saranya-jena <[email protected]> * Fixed imports Signed-off-by: Saranya-jena <[email protected]> * Add fixes in dex service Signed-off-by: Saranya-jena <[email protected]> * Fixed UTs Signed-off-by: Saranya-jena <[email protected]> * resolved comments Signed-off-by: Saranya-jena <[email protected]> * updated logic Signed-off-by: Saranya-jena <[email protected]> * fixed UTs and removed unecessary test cases Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> * resolved comments Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> * resolved comments Signed-off-by: Saranya-jena <[email protected]> * added server endpoint in allowed origins Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> * minor chnages Signed-off-by: Saranya-jena <[email protected]> * minor chnages Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> --------- Signed-off-by: Saranya-jena <[email protected]> Signed-off-by: andoriyaprashant <[email protected]>
…itmuschaos#4719) * Added support for jwt secret creation of each user upon logic Signed-off-by: Saranya-jena <[email protected]> * Fixed imports Signed-off-by: Saranya-jena <[email protected]> * Add fixes in dex service Signed-off-by: Saranya-jena <[email protected]> * Fixed UTs Signed-off-by: Saranya-jena <[email protected]> * resolved comments Signed-off-by: Saranya-jena <[email protected]> * updated logic Signed-off-by: Saranya-jena <[email protected]> * fixed UTs and removed unecessary test cases Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> * resolved comments Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> * resolved comments Signed-off-by: Saranya-jena <[email protected]> * added server endpoint in allowed origins Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> * minor chnages Signed-off-by: Saranya-jena <[email protected]> * minor chnages Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> --------- Signed-off-by: Saranya-jena <[email protected]> Signed-off-by: andoriyaprashant <[email protected]>
…itmuschaos#4719) * Added support for jwt secret creation of each user upon logic Signed-off-by: Saranya-jena <[email protected]> * Fixed imports Signed-off-by: Saranya-jena <[email protected]> * Add fixes in dex service Signed-off-by: Saranya-jena <[email protected]> * Fixed UTs Signed-off-by: Saranya-jena <[email protected]> * resolved comments Signed-off-by: Saranya-jena <[email protected]> * updated logic Signed-off-by: Saranya-jena <[email protected]> * fixed UTs and removed unecessary test cases Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> * resolved comments Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> * resolved comments Signed-off-by: Saranya-jena <[email protected]> * added server endpoint in allowed origins Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> * minor chnages Signed-off-by: Saranya-jena <[email protected]> * minor chnages Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> --------- Signed-off-by: Saranya-jena <[email protected]> Signed-off-by: andoriyaprashant <[email protected]>
…itmuschaos#4719) * Added support for jwt secret creation of each user upon logic Signed-off-by: Saranya-jena <[email protected]> * Fixed imports Signed-off-by: Saranya-jena <[email protected]> * Add fixes in dex service Signed-off-by: Saranya-jena <[email protected]> * Fixed UTs Signed-off-by: Saranya-jena <[email protected]> * resolved comments Signed-off-by: Saranya-jena <[email protected]> * updated logic Signed-off-by: Saranya-jena <[email protected]> * fixed UTs and removed unecessary test cases Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> * resolved comments Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> * resolved comments Signed-off-by: Saranya-jena <[email protected]> * added server endpoint in allowed origins Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> * minor chnages Signed-off-by: Saranya-jena <[email protected]> * minor chnages Signed-off-by: Saranya-jena <[email protected]> * fixed imports Signed-off-by: Saranya-jena <[email protected]> --------- Signed-off-by: Saranya-jena <[email protected]> Signed-off-by: sagnik3788 <[email protected]>
Proposed changes
As part of the fix we are storing the JWT secret in the DB which is generated using randomization and then encoded into base64 format before inserting into DB. This secret is generated once during litmus installation and is being used for token validation.
Summarize your changes here to communicate with the maintainers and make sure to put the link of that issue
Types of changes
What types of changes does your code introduce to Litmus? Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Dependency
Special notes for your reviewer: