Added support for jwt secret creation of each user upon user login #4719

Saranya-jena · 2024-06-19T13:52:28Z

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 apply

New feature (non-breaking change which adds functionality)
Bugfix (non-breaking change which fixes an issue)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Update (if none of the other choices applies)

Checklist

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.

I have read the CONTRIBUTING doc
I have signed the commit for DCO to be passed.
Lint and unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have added necessary documentation (if appropriate)

Dependency

Please add the links to the dependent PR need to be merged before this (if any).

Special notes for your reviewer:

Signed-off-by: Saranya-jena <[email protected]>

chaoscenter/authentication/pkg/user/repository.go

 // 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})


Signed-off-by: Saranya-jena <[email protected]>

SarthakJain26 · 2024-06-25T14:05:30Z

chaoscenter/authentication/api/handlers/rest/user_handlers.go

+ 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
+ }


Can you please make this a new function which can be reused in both the places

SarthakJain26 · 2024-06-25T14:06:54Z

chaoscenter/authentication/pkg/user/repository.go

@@ -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)


This should be just Update not UpdateMany

SarthakJain26 · 2024-06-25T14:08:53Z

chaoscenter/authentication/pkg/utils/sanitizers.go

+func RandomString(n int) string {
+ if n > 0 {
+ var letters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-")
+ rand.Seed(time.Now().UnixNano())


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]>

SarthakJain26 · 2024-06-27T11:08:29Z

chaoscenter/authentication/pkg/services/auth_config_service.go

+ return a.authConfigRepo.GetConfig(key)
+}
+
+func (a applicationService) UpdateConfig(ctx context.Context, key string, value interface{}) error {


I don't think this will be ever used

Have kept it for future use in case we need to add more such cases

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.

SarthakJain26 · 2024-06-27T11:09:35Z

chaoscenter/authentication/pkg/authConfig/repository.go

+ Collection *mongo.Collection
+}
+
+func (r repository) CreateConfig(config AuthConfig) error {


How about renaming this to CreateSalt and GetSalt, code will be more readable

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.

SarthakJain26 · 2024-06-27T11:09:52Z

chaoscenter/authentication/api/handlers/salt.go

+package response
+
+import (
+ authConfig2 "github.com/litmuschaos/litmus/chaoscenter/authentication/pkg/authConfig"


No need to add alias to this import

SarthakJain26 · 2024-06-27T11:14:53Z

chaoscenter/authentication/api/handlers/rest/dex_auth_handler.go

@@ -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")


The salt value being returned here is an encrypted value and should be decrypted before using it to generate the JWT

SarthakJain26 · 2024-06-27T11:15:07Z

chaoscenter/authentication/api/handlers/rest/user_handlers.go

@@ -294,7 +293,13 @@ func LoginUser(service services.ApplicationService) gin.HandlerFunc {
 return
 }

- token, err := service.GetSignedJWT(user)
+ salt, err := service.GetConfig("salt")


Same as above

Signed-off-by: Saranya-jena <[email protected]>

amityt · 2024-06-28T06:19:06Z

chaoscenter/authentication/api/handlers/rest/user_handlers.go

@@ -294,7 +293,13 @@ func LoginUser(service services.ApplicationService) gin.HandlerFunc {
 return
 }

- token, err := service.GetSignedJWT(user)
+ salt, err := service.GetConfig("salt")


Can you create a constant for the "salt"?

amityt · 2024-06-28T06:19:19Z

chaoscenter/authentication/api/handlers/salt.go

+ encodedSalt := base64.StdEncoding.EncodeToString([]byte(salt))
+
+ config := authConfig.AuthConfig{
+ Key: "salt",


Use the constant here.

amityt · 2024-06-28T06:19:38Z

chaoscenter/authentication/pkg/services/session_service.go

@@ -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")


amityt · 2024-06-28T06:22:02Z

chaoscenter/graphql/server/pkg/database/mongodb/operations.go

+
+func (m *MongoOperations) GetAuthConfig(ctx context.Context, key string) (*AuthConfig, error) {
+
+ authDb := MgoClient.Database("auth")


From constants?

amityt · 2024-06-28T06:25:21Z

chaoscenter/graphql/server/pkg/database/mongodb/authConfig/operations.go

+}
+
+func (c *Operator) GetAuthConfig(ctx context.Context) (*mongodb.AuthConfig, error) {
+ salt, err := c.operator.GetAuthConfig(ctx, "salt")


Signed-off-by: Saranya-jena <[email protected]>

…wt-secret-fix

Signed-off-by: Saranya-jena <[email protected]>

chaoscenter/authentication/api/handlers/rest/dex_auth_handler.go

Signed-off-by: Saranya-jena <[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]>

Added support for jwt secret creation of each user upon logic

bcad589

Signed-off-by: Saranya-jena <[email protected]>

Saranya-jena requested review from Jonsy13 and SarthakJain26 June 19, 2024 13:52

Saranya-jena self-assigned this Jun 19, 2024

github-advanced-security bot found potential problems Jun 19, 2024

View reviewed changes

Saranya-jena added 3 commits June 19, 2024 19:28

Fixed imports

3e46bb7

Signed-off-by: Saranya-jena <[email protected]>

Add fixes in dex service

1bd4ffe

Signed-off-by: Saranya-jena <[email protected]>

Fixed UTs

8bc31a0

Signed-off-by: Saranya-jena <[email protected]>

SarthakJain26 requested changes Jun 25, 2024

View reviewed changes

Saranya-jena added 6 commits June 25, 2024 20:08

updated branch

7c38510

Signed-off-by: Saranya-jena <[email protected]>

resolved comments

5e9ac34

Signed-off-by: Saranya-jena <[email protected]>

updated logic

2cc3e14

Signed-off-by: Saranya-jena <[email protected]>

fixed UTs and removed unecessary test cases

7cdb585

Signed-off-by: Saranya-jena <[email protected]>

fixed imports

4390e84

Signed-off-by: Saranya-jena <[email protected]>

fixed imports

2e7a37e

Signed-off-by: Saranya-jena <[email protected]>

SarthakJain26 requested changes Jun 27, 2024

View reviewed changes

Saranya-jena added 2 commits June 27, 2024 21:03

resolved comments

1f92210

Signed-off-by: Saranya-jena <[email protected]>

fixed imports

095e944

Signed-off-by: Saranya-jena <[email protected]>

amityt requested changes Jun 28, 2024

View reviewed changes

SarthakJain26 approved these changes Jun 28, 2024

View reviewed changes

Saranya-jena added 5 commits June 30, 2024 15:52

resolved comments

dd5054b

Signed-off-by: Saranya-jena <[email protected]>

updated branch

1db5978

Signed-off-by: Saranya-jena <[email protected]>

added server endpoint in allowed origins

52322b0

Signed-off-by: Saranya-jena <[email protected]>

Merge branch 'master' of https://github.com/litmuschaos/litmus into j…

4695016

…wt-secret-fix

fixed imports

cdd3b88

Signed-off-by: Saranya-jena <[email protected]>

Jonsy13 reviewed Jul 2, 2024

View reviewed changes

chaoscenter/authentication/api/handlers/rest/dex_auth_handler.go Outdated Show resolved Hide resolved

minor chnages

2bd2d16

Signed-off-by: Saranya-jena <[email protected]>

Jonsy13 approved these changes Jul 2, 2024

View reviewed changes

Saranya-jena added 2 commits July 2, 2024 15:46

minor chnages

6915351

Signed-off-by: Saranya-jena <[email protected]>

fixed imports

228a255

Signed-off-by: Saranya-jena <[email protected]>

Saranya-jena force-pushed the jwt-secret-fix branch from ef0c21a to 228a255 Compare July 4, 2024 10:10

Saranya-jena added 2 commits July 4, 2024 16:21

Merge branch 'master' into jwt-secret-fix

5a7c9bf

Merge branch 'master' into jwt-secret-fix

7f54fb1

Saranya-jena merged commit 9d58d8b into litmuschaos:master Jul 5, 2024
18 of 19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for jwt secret creation of each user upon user login #4719

Added support for jwt secret creation of each user upon user login #4719

Saranya-jena commented Jun 19, 2024 •

edited

Loading

SarthakJain26 Jun 25, 2024

SarthakJain26 Jun 25, 2024

SarthakJain26 Jun 25, 2024

SarthakJain26 Jun 27, 2024

Saranya-jena Jun 27, 2024

SarthakJain26 Jun 28, 2024

SarthakJain26 Jun 27, 2024

Saranya-jena Jun 27, 2024

SarthakJain26 Jun 27, 2024

SarthakJain26 Jun 27, 2024

SarthakJain26 Jun 27, 2024

amityt Jun 28, 2024

amityt Jun 28, 2024

amityt Jun 28, 2024

amityt Jun 28, 2024

amityt Jun 28, 2024


		func (m MongoOperations) GetAuthConfig(ctx context.Context, key string) (AuthConfig, error) {

		authDb := MgoClient.Database("auth")

Added support for jwt secret creation of each user upon user login #4719

Added support for jwt secret creation of each user upon user login #4719

Conversation

Saranya-jena commented Jun 19, 2024 • edited Loading

Proposed changes

Types of changes

Checklist

Dependency

Special notes for your reviewer:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saranya-jena commented Jun 19, 2024 •

edited

Loading