-
Notifications
You must be signed in to change notification settings - Fork 110
Opportunistic Token Encryption for Slack Actions #723
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
Open
jeffrey-martinez
wants to merge
6
commits into
master
Choose a base branch
from
feat/slack-token-encryption
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
f1e366d
Extend token encryption to Slack with fallback for backward compatibi…
jeffrey-martinez df1216c
Add comments to Slack encryption tests
jeffrey-martinez e739ea8
Refine Slack token encryption: optimize decryption, deduplicate code,…
jeffrey-martinez 1a1eb9f
feat: implement Tollbooth pattern for slack token encryption
jeffrey-martinez f7a1ada
build: compile typescript
jeffrey-martinez 741d4ce
Merge branch 'master' into feat/slack-token-encryption
jeffrey-martinez File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import {WebClient} from "@slack/web-api" | ||
| import {WebAPICallResult} from "@slack/web-api/dist/WebClient" | ||
| import * as winston from "winston" | ||
| import { AESTransitCrypto } from "../../crypto/aes_transit_crypto" | ||
| import {HTTP_ERROR} from "../../error_types/http_errors" | ||
| import * as Hub from "../../hub" | ||
| import {Error, errorWith} from "../../hub/action_response" | ||
|
|
@@ -37,9 +38,17 @@ export class SlackAction extends Hub.DelegateOAuthAction { | |
| usesStreaming = true | ||
| executeInOwnProcess = true | ||
|
|
||
| private readonly crypto = new AESTransitCrypto() | ||
|
|
||
| /** | ||
| * Executes the Slack action. | ||
| * Decrypts state_json if it was previously encrypted and passes it to the client manager. | ||
| * If execution succeeds and the feature flag is on, it encrypts the state before returning. | ||
| */ | ||
| async execute(request: Hub.ActionRequest) { | ||
| const resp = new Hub.ActionResponse() | ||
| const clientManager = new SlackClientManager(request, true) | ||
| const decryptedState = await this.decryptStateIfNeeded(request) | ||
| const clientManager = new SlackClientManager(request, true, decryptedState) | ||
| const selectedClient = clientManager.getSelectedClient() | ||
| if (!selectedClient) { | ||
| const error: Error = errorWith( | ||
|
|
@@ -55,12 +64,20 @@ export class SlackAction extends Hub.DelegateOAuthAction { | |
| winston.error(`${error.message}`, {error, webhookId: request.webhookId}) | ||
| return resp | ||
| } else { | ||
| return await handleExecute(request, selectedClient) | ||
| const executedResponse = await handleExecute(request, selectedClient) | ||
| await this.updateStateIfNeeded(executedResponse, decryptedState, request.params.state_json) | ||
| return executedResponse | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Retrieves the form fields for the action. | ||
| * Decrypts state_json before creating clients to fetch available workspaces. | ||
| * If the response state needs to be maintained, it encrypts it before sending it back to Looker. | ||
| */ | ||
| async form(request: Hub.ActionRequest) { | ||
| const clientManager = new SlackClientManager(request, false) | ||
| const decryptedState = await this.decryptStateIfNeeded(request) | ||
| const clientManager = new SlackClientManager(request, false, decryptedState) | ||
| if (!clientManager.hasAnyClients()) { | ||
| return this.loginForm(request) | ||
| } | ||
|
|
@@ -107,6 +124,7 @@ export class SlackAction extends Hub.DelegateOAuthAction { | |
| return this.loginForm(request, form) | ||
| } | ||
|
|
||
| await this.updateStateIfNeeded(form, decryptedState, request.params.state_json) | ||
| return form | ||
| } | ||
|
|
||
|
|
@@ -128,10 +146,14 @@ export class SlackAction extends Hub.DelegateOAuthAction { | |
| return form | ||
| } | ||
|
|
||
| /** | ||
| * Checks if the OAuth connection is valid. | ||
| * Opportunistically decrypts the state and returns whether the connection holds. | ||
| */ | ||
| async oauthCheck(request: Hub.ActionRequest) { | ||
| const form = new Hub.ActionForm() | ||
|
|
||
| const clientManager = new SlackClientManager(request) | ||
| const decryptedState = await this.decryptStateIfNeeded(request) | ||
| const clientManager = new SlackClientManager(request, false, decryptedState) | ||
| if (!clientManager.hasAnyClients()) { | ||
| form.error = AUTH_MESSAGE | ||
| winston.error(`${LOG_PREFIX} ${AUTH_MESSAGE}`, {webhookId: request.webhookId}) | ||
|
|
@@ -155,6 +177,7 @@ export class SlackAction extends Hub.DelegateOAuthAction { | |
| form.error = displayError[e.message] || e | ||
| winston.error(`${LOG_PREFIX} ${form.error}`, {webhookId: request.webhookId}) | ||
| } | ||
| await this.updateStateIfNeeded(form, decryptedState, request.params.state_json) | ||
| return form | ||
| } | ||
|
|
||
|
|
@@ -172,6 +195,54 @@ export class SlackAction extends Hub.DelegateOAuthAction { | |
| } | ||
| return result | ||
| } | ||
| /** | ||
| * Re-encrypts the state_json if it was decrypted successfully and the feature flag is on. | ||
| */ | ||
| private async updateStateIfNeeded( | ||
| response: Hub.ActionResponse | Hub.ActionForm, | ||
| decryptedState: string | undefined, | ||
| originalState: string | undefined, | ||
| ) { | ||
| if (decryptedState && decryptedState === originalState && process.env.ENCRYPT_PAYLOAD_SLACK_APP === "true") { | ||
| response.state = new Hub.ActionState() | ||
| response.state.data = await this.encryptStateJson(decryptedState) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Decrypts the state_json parsing it as plain text if decryption fails. | ||
| * This ensures backward compatibility with older, unencrypted states. | ||
| */ | ||
| private async decryptStateIfNeeded(request: Hub.ActionRequest): Promise<string | undefined> { | ||
| if (!request.params.state_json) { | ||
| return undefined | ||
| } | ||
| if (!request.params.state_json.startsWith("1")) { | ||
|
||
| return request.params.state_json | ||
| } | ||
| try { | ||
| return await this.crypto.decrypt(request.params.state_json) | ||
| } catch (e: any) { | ||
| winston.warn(`${LOG_PREFIX} Decryption failed, assuming plain text: ${e.message}`) | ||
| return request.params.state_json | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Encrypts the state_json string if the feature flag is enabled. | ||
| * Returns the encrypted string or the original state if encryption fails or is disabled. | ||
| */ | ||
| private async encryptStateJson(stateJson: string): Promise<string> { | ||
| if (process.env.ENCRYPT_PAYLOAD_SLACK_APP === "true") { | ||
| try { | ||
| return await this.crypto.encrypt(stateJson) | ||
| } catch (e: any) { | ||
| winston.error(`${LOG_PREFIX} Encryption failed: ${e.message}`) | ||
| return stateJson | ||
| } | ||
| } | ||
| return stateJson | ||
| } | ||
| } | ||
|
|
||
| Hub.addAction(new SlackAction()) | ||
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we decrypted the state then checked against request.params.state_json isn't that going to be encrypted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we check if it matches the original (unencrypted case) then we know to migrate them to use encryption.