-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Encrypt and store client_secret #822
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
Closed
cliffhall
wants to merge
2
commits into
modelcontextprotocol:main
from
cliffhall:encrypt-and-store-client-secret
Closed
Encrypt and store client_secret #822
cliffhall
wants to merge
2
commits into
modelcontextprotocol:main
from
cliffhall:encrypt-and-store-client-secret
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- a clientEncryptionKey is now generated by the proxy server and exposed via the /config endpoint.
- The client stores it in state and passes it to all OAuth-related providers and flows.
- Client-side session storage of OAuthClientInformation will encrypt client_secret when saving and decrypt when reading, using a simple XOR+base64 scheme via encodeWithKey and decodeWithKey.
- During the Auth Debugger flow, the client_secret is encrypted in the temporary SESSION_KEYS.AUTH_DEBUGGER_STATE before redirect and decrypted on the debug callback.
- Tests were updated to inject a deterministic clientEncryptionKey.
- Plus a minor drive-by linter fix: useCopy’s effect dependency array updated; JsonView effect added setCopied to dependencies
* In client/src/App.tsx
- Added state for clientEncryptionKey initialized to an empty string.
- Updated OAuthStateMachine construction to accept clientEncryptionKey as the second argument and forward updates callback.
- Updated onOAuthConnect handler dependency array to include clientEncryptionKey.
- When fetching /config, read clientEncryptionKey from the returned JSON and call setClientEncryptionKey if present.
- Passed clientEncryptionKey into AuthDebugger as prop.
- Passed clientEncryptionKey to OAuthCallback component.
- Passed clientEncryptionKey to OAuthDebugCallback component
* In client/src/lib/auth.ts
- Added encodeWithKey function
- XOR bytes of plaintext with key bytes, base64-encode the result.
- Added decodeWithKey
- base64-decode then XOR with key bytes to recover plaintext.
- Storage readers/writers updated to support encryption:
- getClientInformationFromSessionStorage
- now accepts clientEncryptionKey
- After parsing JSON into OAuthClientInformation, attempt to decrypt client_secret using clientEncryptionKey
- Log a warning if decryption fails and proceed with the parsed object.
- saveClientInformationToSessionStorage
- now accepts clientEncryptionKey
- If clientEncryptionKey and a client_secret are present, encrypt the client_secret before writing to sessionStorage
- In InspectorOAuthClientProvider
- Add protected clientEncryptionKey param to constructor
- in clientInformation method,
- Pass clientEncryptionKey through when calling getClientInformationFromSessionStorage for both preregistered and dynamically stored client info.
- In saveClientInformation method,
- Stop stripping client_secret.
- Now save full clientInformation to session storage via saveClientInformationToSessionStorage, passing clientEncryptionKey
* In client/src/components/tests/AuthDebugger.test.tsx
- Define test clientEncryptionKey
- Provide clientEncryptionKey in default props passed to AuthDebugger in tests
* In client/src/components/AuthDebugger.tsx
- Import encodeWithKey
- Added clientEncryptionKey to AuthDebuggerProps.
- When checking existing tokens, pass clientEncryptionKey to DebugInspectorOAuthClientProvider constructor.
- Added clientEncryptionKey to the useEffect dependency array.
- Where creating the OAuthStateMachine, pass clientEncryptionKey to constructor.
- Before storing state in sessionStorage before redirect, encrypt oauthClientInfo.client_secret if present using encodeWithKey
- Store the modified stateToStore.
- Add clientEncryptionKey to the dependencies of the proceedToNextStep callback.
- In handleClearOAuth,
- pass clientEncryptionKey to DebugInspectorOAuthClientProvider constructor
- Add clientEncryptionKey to handleClearOAuth dependencies.
- Pass clientEncryptionKey down into OAuthFlowProgress component
* In client/src/lib/oauth-state-machine.ts
- In OAuthStateMachine constructor, added private clientEncryptionKey parameter.
- When creating DebugInspectorOAuthClientProvider, pass clientEncryptionKey to constructor
* In client/src/components/OAuthCallback.tsx
- Added clientEncryptionKey to OAuthCallbackProps.
- Component now accepts clientEncryptionKey prop.
- Pass clientEncryptionKey to InspectorOAuthClientProvider constructor
- Add clientEncryptionKey to the useEffect dependency array
* In client/src/components/OAuthDebugCallback.tsx
- Import decodeWithKey
- Added clientEncryptionKey to OAuthCallbackProps prop and accept it.
- When loading restoredState from sessionStorage, if oauthClientInfo.client_secret is present, decrypt it using decodeWithKey and write back the plaintext to restoredState.oauthClientInfo.client_secret prior to use.
- Remove stored state afterward.
- Add clientEncryptionKey to the useEffect dependencies
* In client/src/components/OAuthFlowProgress.tsx
- Added clientEncryptionKey to OAuthFlowProgressProps and accept it.
- Pass clientEncryptionKey to DebugInspectorOAuthClientProvider
- Add clientEncryptionKey to useMemo dependencies
* In client/src/lib/hooks/tests/useConnection.test.tsx
- Define test clientEncryptionKey
- Include clientEncryptionKey when rendering/testing useConnection hook options
* In client/src/lib/hooks/useConnection.ts
- Added clientEncryptionKey to UseConnectionOptions
- Accept clientEncryptionKey parameter
- Pass clientEncryptionKey into saveClientInformationToSessionStorage.
- Add it to the useEffect dependency array
- Whenever constructing InspectorOAuthClientProvider pass clientEncryptionKey.
* In server/src/index.ts
- Generate clientEncryptionKey on the server side:
- Create clientEncryptionKey in same way as mcpProxyAuthToken, allowing it to be provided explicitly as MCP_CLIENT_ENCRYPTION_KEY in the environment
- Include clientEncryptionKey in the JSON response of /config endpoint along with existing defaults so the client can retrieve and use it
* In client/src/components/JsonView.tsx
- In an effect that references setCopied, the dependency array is updated to satisfy exhaustive-deps
* In client/src/lib/hooks/useCopy.ts
- Effect dependency array corrected to include setCopied and timeout for proper cleanup and lint compliance
Member
Author
|
Ok, closing this product of a wild goose chase. Although it solved the problem raised by #722, that issue is fundamentally wrong in expecting a public client to provide a Here's the final breakdown: #726 (comment) |
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
/configendpoint.OAuthClientInformationwill encryptclient_secretwhen saving and decrypt when reading, using a simple XOR+base64 scheme via newencodeWithKeyanddecodeWithKeyfunctions.client_secretis encrypted in the temporarySESSION_KEYS.AUTH_DEBUGGER_STATEbefore redirect and decrypted on the debug callback.clientEncryptionKey.useCopy&JsonVieweffect dependency arrays updated to silence linterIn
client/src/App.tsxIn
client/src/lib/auth.tsencodeWithKeyfunctiondecodeWithKeyfunctiongetClientInformationFromSessionStorageOAuthClientInformation, attempt to decryptclient_secretusingclientEncryptionKeysaveClientInformationToSessionStorageclientEncryptionKeyclientEncryptionKeyand aclient_secretare present, encrypt theclient_secretbefore writing to session storageInspectorOAuthClientProviderclientEncryptionKeyparam to constructorclientInformationmethod,clientEncryptionKeythrough when callinggetClientInformationFromSessionStoragefor both preregistered and dynamically stored client info.saveClientInformationmethod,client_secret.clientInformationto session storage viasaveClientInformationToSessionStorage, passing clientEncryptionKeyIn
client/src/components/tests/AuthDebugger.test.tsxclientEncryptionKeyclientEncryptionKeyin default props passed toAuthDebuggerin testsIn
client/src/components/AuthDebugger.tsxencodeWithKeyclientEncryptionKeytoAuthDebuggerProps.clientEncryptionKeytoDebugInspectorOAuthClientProviderconstructor.clientEncryptionKeyto theuseEffectdependency array.OAuthStateMachine, passclientEncryptionKeyto constructor.oauthClientInfo.client_secretif present usingencodeWithKeystateToStore.clientEncryptionKeyto the dependencies of theproceedToNextStepcallback.handleClearOAuth,clientEncryptionKeytoDebugInspectorOAuthClientProviderconstructorclientEncryptionKeytohandleClearOAuthdependencies.clientEncryptionKeydown intoOAuthFlowProgresscomponentIn
client/src/lib/oauth-state-machine.tsOAuthStateMachineconstructor, added privateclientEncryptionKeyparameter.DebugInspectorOAuthClientProvider, passclientEncryptionKeyto constructorIn
client/src/components/OAuthCallback.tsxclientEncryptionKeytoOAuthCallbackProps.clientEncryptionKeyprop.clientEncryptionKeytoInspectorOAuthClientProviderconstructorclientEncryptionKeyto theuseEffectdependency arrayIn
client/src/components/OAuthDebugCallback.tsxdecodeWithKeyclientEncryptionKeytoOAuthCallbackPropsprop and accept it.restoredStatefrom session storage, ifoauthClientInfo.client_secretis present, decrypt it usingdecodeWithKeyand write back the plaintext torestoredState.oauthClientInfo.client_secretprior to use.clientEncryptionKeyto theuseEffectdependenciesIn
client/src/components/OAuthFlowProgress.tsxclientEncryptionKeytoOAuthFlowProgressPropsand accept it.clientEncryptionKeytoDebugInspectorOAuthClientProviderclientEncryptionKeytouseMemodependenciesIn
client/src/lib/hooks/tests/useConnection.test.tsxclientEncryptionKeyclientEncryptionKeywhen rendering/testinguseConnectionhook optionsIn
client/src/lib/hooks/useConnection.tsclientEncryptionKeytoUseConnectionOptionsclientEncryptionKeyparameterclientEncryptionKeyintosaveClientInformationToSessionStorage.useEffectdependency arrayInspectorOAuthClientProvider, passclientEncryptionKey.In
server/src/index.tsclientEncryptionKeyon the server side:clientEncryptionKeyin same way asmcpProxyAuthToken, allowing it to be provided explicitly asMCP_CLIENT_ENCRYPTION_KEYin the environmentclientEncryptionKeyin the JSON response of/configendpoint along with existing defaults so the client can retrieve and use itIn
client/src/components/JsonView.tsxsetCopied, the dependency array is updated to satisfyexhaustive-depsIn
client/src/lib/hooks/useCopy.tssetCopiedandtimeoutfor proper cleanup and lint compliance.Motivation and Context
While researching #726, I discovered that a change made in Inspector Release 0.16.4 - specifically fix sensitive local storage, where we removed
client_secretfrom the client info stored inlocalStoragewas causing authentication failures.The reason is that after an OAuth redirect, that information is read back from storage, and used to create the Authentication header for the
/tokencall. In that call, theclient_idandclient_secretmust be sent to the AS either via post body or Authentication header depending upon the challenge type (client_secret_postorclient_secret_basic).Using an example Python/Flask OAuth server provided by the author of #726, I was able to see that the versions of the Inspector prior to 0.16.4 will get the
/tokenendpoint to return a token, but 0.16.4 and later could not.I suspected the removal of
client_secretfrom storage was the problem, so I commented it out and was able to show that the/tokenendpoint would once again return a token.We clearly need to preserve the
client_secretacross the OAuth redirect boundary.At the time of the fix that removed it from local storage, we discussed encrypting it before storing, but could not come up with a non-obvious way of doing so. The key would need to be ephemeral, and not itself placed in local storage. That rules out generating it or hardcoding it on the client side.
Faced with this dilemma, I decided to generate the key on the proxy server and provide it to the client via the
/configendpoint, which is called each the client loads. That server is protected with an ephemeralMCP_PROXY_AUTH_TOKENso that only the client can make connections. So long as the client does not store this retrieved encryption key, it could use it to encryptclient_secretanytime the client information is stored in local storage, and decrypt it upon reading it back from local storage after an OAuth redirect.How Has This Been Tested?
The hosted, auth-enabled, version of the Everything server - discovered that it is actually not checking for
client_secreton itsclient_secret_postchallenge!The example Python/Flask OAuth server provided in Regression (OAuth flow): Calling token endpoint does not honor "token_endpoint_auth_methods_supported" #726
Breaking Changes
Nope.
Types of changes
Checklist
Additional context