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

Encrypt and decrypt chat lsp communication #55

Merged
merged 17 commits into from
Oct 9, 2024

Conversation

angjordn
Copy link
Contributor

@angjordn angjordn commented Oct 4, 2024

Description

  • Iniitalizes the Amazon Q LSP Server into an encrypted mode
    • The Amazon Q LSP server waits for an encryption key to be sent over stdin before initializing server (signified by the --set-credentials-encryption-key flag in the start command)
    • Once the encryption key is retrieved by the LSP server, it stores the key to be used for future requests
  • The Amazon Q Plugin encrypts and decrypts chat requests and responses
    • Encrypts the ChatRequestParams into a jwt
    • Decrypts the partial result responses from the a jwt into ChatResult
    • Decrypts the final result response from a jwt into a ChatResult

@angjordn angjordn force-pushed the encrypt-and-decrypt-lsp-communication branch from 101d41f to 416b2cb Compare October 4, 2024 20:15
@angjordn angjordn changed the title Encrypt and decrypt lsp communication Encrypt and decrypt chat lsp communication Oct 4, 2024
@angjordn angjordn force-pushed the encrypt-and-decrypt-lsp-communication branch 2 times, most recently from 6d171d0 to d3916ba Compare October 7, 2024 23:24

import com.fasterxml.jackson.annotation.JsonProperty;

public record EncryptedChatParams(
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like both the encrypted classes have the same properties. Any reason we can't have a single class and use it for both use-cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to best match the models provided in the Language Server Runtime types repo, I think it'd be best to continue with two separate classes

See EncryptedChatParams
See EncryptedQuickActionParams

// The mapping entry no longer needs to be maintained once the final result is
// retrieved.
removePartialChatMessage(partialResultToken);

String serializedData = lspEncryptionManager.decrypt(jwt);
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't recall but do these calls get exception handled upstream from any calling code? Saw the throw calls within these decrypt/encrypt calls but don't rememeber if they should be caught or not to prevent any unexpected state to occur.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These exceptions should be caught at the top-level in the Q Chat Part (See

} catch (Exception e) {
PluginLogger.error("Error processing message from Browser", e);
}
)

quickActionParams.setPartialResultToken(token);

return chatMessageProvider.sendQuickAction(quickActionParams);
});
String jwt = lspEncryptionManager.encrypt(quickActionParams);
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we are using encrypted params should L79 instead be changed to EncryptedQuickActionParams for setting partial result token. Don't see the value in using that anymore

Copy link
Contributor Author

@angjordn angjordn Oct 8, 2024

Choose a reason for hiding this comment

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

Good question, the partialResultToken is an attribute in both the QuickActionParams as well as EncryptedQuickActionParams.

In QuickActionParams, we have:

    private final String tabId;
    private final String quickAction;
    private final String prompt;
    private String partialResultToken;

In EncryptedQuickActionParams, we have

   @JsonProperty("message") String message, // Message as encrypted jwt
   @JsonProperty("partialResultToken") String partialResultToken) {

Therefore the partialResultToken is located in both the encrypted message attibute as well as a direct attribute under the EncryptedQuickActionParams attribute.

After diving deeper into the EncryptedChat Communication, I see that the inner-most partialResultToken is being over written by the outer-most token (see https://github.com/aws/language-server-runtimes/blob/4bd707cb4220253620b5550e6dd48cd08a5350b3/runtimes/runtimes/chat/encryptedChat.ts#L76-L77)

To test my theory, I removed the line to set the partialResultToken in the QuickActionPrams and kept the partialResultToken on the EncryptedQuickActionParams - the partial results is continuing to work successfully. I am going to make the same change to ChatParams and EncryptedChatParams

Copy link
Contributor

@shruti0085 shruti0085 Oct 9, 2024

Choose a reason for hiding this comment

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

Great digging! Please inform Flare team about this overriding behavior since it introduces risk. We wouldn't want to get in a scenario where there is change in this data contract on their end and we get impacted by it. i.e a scenario in future where they start honoring the decrypted partial token present in the innermost chat request param object

@angjordn angjordn force-pushed the encrypt-and-decrypt-lsp-communication branch from d3916ba to 011a650 Compare October 8, 2024 22:15
@angjordn angjordn merged commit b8bc1a7 into main Oct 9, 2024
1 check passed
@breedloj breedloj deleted the encrypt-and-decrypt-lsp-communication branch November 22, 2024 17:17
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.

2 participants