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

[BOP-1204] Password encryption on the server side #6

Merged
merged 2 commits into from
Sep 26, 2024

Conversation

ranyodh
Copy link
Collaborator

@ranyodh ranyodh commented Sep 24, 2024

Description:

There are two changes in this PR:

  1. Update response/request envelopes to remove password field.
  2. When creating or updating user, the password (from the field named hash) should not be a base64 encoded plain text, rather than bcrypt encrypt string

Removed password envelope

Removed the password envelope from create user and list user endpoints.
Here are the new request/responses:

List User

Response

[
    {
        "email": "admin",
        "hash": "",
        "username": "admin",
        "userId": "1940d2cf-518b-4932-bd0f-44f8a02a1f97"
    },
    {
        "email": "[email protected]",
        "hash": "",
        "username": "ranyodh1",
        "userId": "ebd40c10-1b29-4e19-8915-1e4e949cff3c"
    },
    {
        "email": "[email protected]",
        "hash": "",
        "username": "ranyodh2",
        "userId": "ebd40c10-1b29-4e19-8915-1e4e949cff3c"
    },
]

Create User

Request

{
    "email": "[email protected]",
    "hash": "cGFzc3dvcmQx",
    "username": "ranyodh5",
    "userId": "ebd40c10-1b29-4e19-8915-1e4e949cff3e"
}

Base64 Encoded Plain Text Password

The bigger change is the accepting a plain text password (base64 encoded), instead of bcrypt encrypted string.

Create User Request

curl -k --location 'https://l63pjn-mke4-lb-27f36ee8e351c0a1.elb.us-west-1.amazonaws.com/api/dex//v1/users' \
--header 'Content-Type: application/json' \
--header 'Accept: application/json' \
--header 'Authorization: ••••••' \
--data-raw '{
    "email": "[email protected]",
    "hash": "cGFzc3dvcmQx",
    "username": "ranyodh5"
}
'
{
    "alreadyExists": false
}

Note: The userId field can also be ommited as this is also generated on the server side

Update User Password Request

curl -k --location --request PUT 'https://l63pjn-mke4-lb-27f36ee8e351c0a1.elb.us-west-1.amazonaws.com/api/dex//v1/users/[email protected]' \
--header 'Content-Type: application/json' \
--header 'Accept: application/json' \
--header 'Authorization: ••••••' \
--data '{
  "newHash": "cGFzc3dvcmQx"
}'
{
    "notFound": false
}

hash and newHash field for password

The name of the field that contains the password is still hash and newHash. This cannot be changed as we are currently generating code from the dex's proto buf.

Also, the password in hash and newHash field must be base64 encoded. This also cannot be modified at this moment

@ranyodh ranyodh marked this pull request as ready for review September 25, 2024 17:36
@@ -56,8 +56,11 @@ func run() error {
creds = insecure.NewCredentials()
}

// Create a gRPC server mux with the custom middlewares
mws := middlewares.GetMiddlewares(*disableAuth)
mux := runtime.NewServeMux(runtime.WithMiddlewares(mws...))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Switched to using middleware support that gRPC gateway provides. With the previous implementation, ran into the following issue: grpc-ecosystem/grpc-gateway#4290

Also, using runtime.WithMiddlewares() fix an issue where middlewares are called regardless if the endpoint actually exists. For example, /v1/api/someEndpoint will cause auth middlewares to be invoked. Using runtime.WithMiddlewares() fixes that.

@ranyodh ranyodh requested review from nwneisen and a team September 25, 2024 17:44
@ranyodh ranyodh merged commit 81ab985 into MirantisContainers:main Sep 26, 2024
@ranyodh ranyodh deleted the server-password-encryption branch September 26, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants