PATCH OAuth 2.0 Client overwrites client_secret

[marcuz]

Preflight checklist

• I could not find a solution in the existing issues, docs, nor discussions.
• I agree to follow this project's Code of Conduct.
• I have read and am following this repository's Contribution Guidelines.
• This issue affects my Ory Cloud project.
• I have joined the Ory Community Slack.
• I am signed up to the Ory Security Patch Newsletter.

Describe the bug

A PATCH request against $hydra_uri/clients/$client_id causes Hydra to overwrite the client_secret even if not explicitly specified.

> select * from hydra_client where id='db2c455b-fe60-48ff-8c79-1d136746225d'\G
*************************** 1. row ***************************
                                  id: db2c455b-fe60-48ff-8c79-1d136746225d
                         client_name: 
                       client_secret: $2a$10$F49N51kib7xkieqQDbLVo.ksds.pq8qawwZZWL9gtDYx7XB7JDyDy
                       redirect_uris: 
                         grant_types: client_credentials
                      response_types: token
                               scope: [REDACTED]
                               owner: 
                          policy_uri: 
                             tos_uri: 
                          client_uri: 
                            logo_uri: 
                            contacts: 
            client_secret_expires_at: 0
               sector_identifier_uri: 
                                jwks: {}
                            jwks_uri: 
                        request_uris: 
          token_endpoint_auth_method: client_secret_basic
          request_object_signing_alg: 
        userinfo_signed_response_alg: none
                        subject_type: public
                allowed_cors_origins: 
                                  pk: 803
                            audience: 
                          created_at: [REDACTED]
                          updated_at: 2021-11-23 10:34:15
             frontchannel_logout_uri: 
frontchannel_logout_session_required: 0
           post_logout_redirect_uris: 
              backchannel_logout_uri: 
 backchannel_logout_session_required: 0
                            metadata: {}
     token_endpoint_auth_signing_alg: 

Issue a PATCH request to update scope:

$ curl -s -H "Content-Type:application/json" -X PATCH [REDACTED]:4445/clients/db2c455b-fe60-48ff-8c79-1d136746225d -d'[ { "op": "replace", "path": "/scope", "value": "[REDACTED]" } ]' | jq .
{
  "client_id": "db2c455b-fe60-48ff-8c79-1d136746225d",
  "client_name": "",
  "client_secret": "$2a$10$F49N51kib7xkieqQDbLVo.ksds.pq8qawwZZWL9gtDYx7XB7JDyDy",
  "redirect_uris": [],
  "grant_types": [
    "client_credentials"
  ],
  "response_types": [
    "token"
  ],
  "scope": "[REDACTED]",
  "audience": [],
  "owner": "",
  "policy_uri": "",
  "allowed_cors_origins": [],
  "tos_uri": "",
  "client_uri": "",
  "logo_uri": "",
  "contacts": [],
  "client_secret_expires_at": 0,
  "subject_type": "public",
  "jwks": {},
  "token_endpoint_auth_method": "client_secret_basic",
  "userinfo_signed_response_alg": "none",
  "created_at": "[REDACTED]",
  "updated_at": "2021-11-23T10:35:32.797716Z",
  "metadata": {}
}

Note the client_secret is returned (not in cleartext) and it matches the one in the database.

Check database again and client_secret has been modified:

> select * from hydra_client where id='db2c455b-fe60-48ff-8c79-1d136746225d'\G
*************************** 1. row ***************************
                                  id: db2c455b-fe60-48ff-8c79-1d136746225d
                         client_name: 
                       client_secret: $2a$10$1wuA.gmzAFiSnpj.yU6RGuq6djkv3Fl10TR2Dve0tNpthaQGRNn.K
                       redirect_uris: 
                         grant_types: client_credentials
                      response_types: token
                               scope: [REDACTED]
                               owner: 
                          policy_uri: 
                             tos_uri: 
                          client_uri: 
                            logo_uri: 
                            contacts: 
            client_secret_expires_at: 0
               sector_identifier_uri: 
                                jwks: {}
                            jwks_uri: 
                        request_uris: 
          token_endpoint_auth_method: client_secret_basic
          request_object_signing_alg: 
        userinfo_signed_response_alg: none
                        subject_type: public
                allowed_cors_origins: 
                                  pk: 803
                            audience: 
                          created_at: [REDACTED]
                          updated_at: 2021-11-23 10:35:33
             frontchannel_logout_uri: 
frontchannel_logout_session_required: 0
           post_logout_redirect_uris: 
              backchannel_logout_uri: 
 backchannel_logout_session_required: 0
                            metadata: {}
     token_endpoint_auth_signing_alg: 

If I PATCH the client_secret field updating the secret, it is returned in the response in cleartext as documented:

$ curl -s -H "Content-Type:application/json" -X PATCH [REDACTED]:4445/clients/db2c455b-fe60-48ff-8c79-1d136746225d -d'[ { "op": "replace", "path": "/client_secret", "value": "prdcprdcprdcprdcprdcprdcprdc" } ]' | jq .
{
  "client_id": "db2c455b-fe60-48ff-8c79-1d136746225d",
  "client_name": "",
  "client_secret": "prdcprdcprdcprdcprdcprdcprdc",
  "redirect_uris": [],
  "grant_types": [
    "client_credentials"
  ],
  "response_types": [
    "token"
  ],
  "scope": "[REDACTED]",
  "audience": [],
  "owner": "",
  "policy_uri": "",
  "allowed_cors_origins": [],
  "tos_uri": "",
  "client_uri": "",
  "logo_uri": "",
  "contacts": [],
  "client_secret_expires_at": 0,
  "subject_type": "public",
  "jwks": {},
  "token_endpoint_auth_method": "client_secret_basic",
  "userinfo_signed_response_alg": "none",
  "created_at": "[REDACTED]",
  "updated_at": "2021-11-23T11:18:54.4841Z",
  "metadata": {}
}

Reproducing the bug

1. PATCH OAuth 2.0 Client without passing client_secret

Relevant log output

{
  "audience": "application",
  "error": {
    "debug": "crypto/bcrypt: hashedPassword is not the hash of the given password",
    "message": "invalid_client",
    "reason": "",
    "status": "Unauthorized",
    "status_code": 401
  },
  "http_request": {
    "headers": {},
    "host": "[REDACTED]:4444",
    "method": "POST",
    "path": "/oauth2/token",
    "query": null,
    "remote": "[REDACTED]",
    "scheme": "http"
  },
  "level": "info",
  "msg": "access denied",
  "service_name": "ORY Hydra",
  "service_version": "v1.10.7",
  "time": "2021-11-23T11:01:59Z"
}

Relevant configuration

No response

Version

1.10.6, 1.10.7

On which operating system are you observing this issue?

Linux

In which environment are you deploying?

Kubernetes with Helm

Additional Context

According to the docs this is a bug:

Patch an existing OAuth 2.0 Client. If you pass client_secret the secret will be updated and returned via the API. This is the only time you will be able to retrieve the client secret, so write it down and keep it safe.

[aeneasr]

Thank you for the report! My hunch says that we're re-hashing the hash which was set before. Most likely because are fetching the client from the store

hydra/client/handler.go

Line 188 in 31af257

c, err := h.r.ClientManager().GetConcreteClient(r.Context(), id)

but are not nulling the secret but instead just apply the patch on it. Compare this to the PUT method

hydra/client/handler.go

Line 147 in 31af257

if err := json.NewDecoder(r.Body).Decode(&c); err != nil {

which receives the full payload from the caller, thus not having the hashed password available.

Would be awesome if you could supply a patch/PR with a failing test case!

[lpedrosa]

Hey @aeneasr, thanks for the reply!

I can submit the failing test case. Would you be able to point out some other test examples where I can base this from?

[lpedrosa]

This was pretty straightforward to fix. Much like you've mentioned, nulling the secret if it hasn't changed does the trick.

I have also added a test case that captures the failure and now it is all green.