From b38d56f12853e7f95c864c08d7dd000f3279dcb8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 18 Jan 2023 13:57:40 +0100 Subject: [PATCH 1/4] Support (and default to) limit-access-to-actor == auto It is relatively easy to forget to limit access to the tmate session and accidentally make secrets accessible to anyone with an SSH client. Let's introduce a `limit-access-to-actor` mode that defaults to using SSH public keys registered with the actor's GitHub profile, if any, and fall back to using potentially unsafe tmate session that anybdy can connect to. This strikes a balance between security and ease of use. Signed-off-by: Johannes Schindelin --- README.md | 2 +- action.yml | 4 ++-- src/index.js | 17 ++++++++++------- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index f0ffb17f..26c219ac 100644 --- a/README.md +++ b/README.md @@ -127,7 +127,7 @@ jobs: ## Use registered public SSH key(s) -By default anybody can connect to the tmate session. You can opt-in to install the public SSH keys [that you have registered with your GitHub profile](https://docs.github.com/en/github/authenticating-to-github/adding-a-new-ssh-key-to-your-github-account). +If [you have registered one or more public SSH keys with your GitHub profile](https://docs.github.com/en/github/authenticating-to-github/adding-a-new-ssh-key-to-your-github-account), tmate will be started such that only those keys are authorized to connect, otherwise anybody can connect to the tmate session. If you want to require a public SSH key to be installed with the tmate session, no matter whether the user who started the workflow has registered any in their GitHub profile, you will need to configure the setting `limit-access-to-actor` to `true`, like so: ```yaml name: CI diff --git a/action.yml b/action.yml index dc30e6d8..1e8e6822 100644 --- a/action.yml +++ b/action.yml @@ -16,9 +16,9 @@ inputs: required: false default: 'true' limit-access-to-actor: - description: 'If only the public SSH keys of the user triggering the workflow should be authorized' + description: 'Whether to authorize only the public SSH keys of the user triggering the workflow (defaults to true if the GitHub profile of the user has a public SSH key)' required: false - default: 'false' + default: 'auto' tmate-server-host: description: 'The hostname for your tmate server (e.g. ssh.example.org)' required: false diff --git a/src/index.js b/src/index.js index d1e158d3..6fe885e9 100644 --- a/src/index.js +++ b/src/index.js @@ -79,7 +79,8 @@ export async function run() { } let newSessionExtra = "" - if (core.getInput("limit-access-to-actor") === "true") { + const limitAccessToActor = core.getInput("limit-access-to-actor") + if (limitAccessToActor === "true" || limitAccessToActor === "auto") { const { actor, apiUrl } = github.context const auth = core.getInput('github-token') const octokit = new Octokit({ auth, baseUrl: apiUrl }) @@ -88,13 +89,15 @@ export async function run() { username: actor }) if (keys.data.length === 0) { - throw new Error(`No public SSH keys registered with ${actor}'s GitHub profile`) + if (limitAccessToActor === "auto") core.info(`No public SSH keys found for ${actor}; continuing without them`) + else throw new Error(`No public SSH keys registered with ${actor}'s GitHub profile`) + } else { + const sshPath = path.join(os.homedir(), ".ssh") + await fs.promises.mkdir(sshPath, { recursive: true }) + const authorizedKeysPath = path.join(sshPath, "authorized_keys") + await fs.promises.writeFile(authorizedKeysPath, keys.data.map(e => e.key).join('\n')) + newSessionExtra = `-a "${authorizedKeysPath}"` } - const sshPath = path.join(os.homedir(), ".ssh") - await fs.promises.mkdir(sshPath, { recursive: true }) - const authorizedKeysPath = path.join(sshPath, "authorized_keys") - await fs.promises.writeFile(authorizedKeysPath, keys.data.map(e => e.key).join('\n')) - newSessionExtra = `-a "${authorizedKeysPath}"` } const tmate = `${tmateExecutable} -S /tmp/tmate.sock`; From d39ec9a5da49b2f6d1d099439c1e4a6eee51ce36 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 18 Jan 2023 14:00:31 +0100 Subject: [PATCH 2/4] When using public key authorization, show also `-i` cmdline The line shown by `action-tmate` with the `ssh` invocation assumes that either no public key authorization was enabled, or that there is only one private key present locally that `ssh` picks up by default. However, many developers use multiple, distinct private keys for distinct purposes, so that one compromised private key does not make _all_ of the hosts vulnerable that are accessible to the user. In such an instance, the private key must be specified explicitly via the `-i` option. The message shown by `action-tmate` is meant to be copy/paste'able, but for invocations requiring the `-i` option that is not true. We cannot _really_ make it completely copy/paste'able, but we can at least make it easier to copy/paste/edit the invocation. Signed-off-by: Johannes Schindelin --- src/index.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/index.js b/src/index.js index 6fe885e9..ceca5ac5 100644 --- a/src/index.js +++ b/src/index.js @@ -79,6 +79,7 @@ export async function run() { } let newSessionExtra = "" + let tmateSSHDashI = "" const limitAccessToActor = core.getInput("limit-access-to-actor") if (limitAccessToActor === "true" || limitAccessToActor === "auto") { const { actor, apiUrl } = github.context @@ -97,6 +98,7 @@ export async function run() { const authorizedKeysPath = path.join(sshPath, "authorized_keys") await fs.promises.writeFile(authorizedKeysPath, keys.data.map(e => e.key).join('\n')) newSessionExtra = `-a "${authorizedKeysPath}"` + tmateSSHDashI = "ssh -i " } } @@ -140,6 +142,9 @@ export async function run() { core.info(`Web shell: ${tmateWeb}`); } core.info(`SSH: ${tmateSSH}`); + if (tmateSSHDashI) { + core.info(`or: ${tmateSSH.replace(/^ssh/, tmateSSHDashI)}`) + } if (continueFileExists()) { core.info("Exiting debugging session because the continue file was created") From ff35d61c2137c7acbb22a9ec1b3d8c219ebc574a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 18 Jan 2023 14:05:04 +0100 Subject: [PATCH 3/4] Warn more strongly when no public SSH key was found It is very, very easy to start an action-tmate session and forgetting that secrets are made available that way that should be protected. For example, `actions/checkout` persists credentials by default, and if the tmate session is not secured, theoretically anybody could exfiltrate those credentials. Let's warn a lot louder about this and strongly encourage allowing authenticated access only. Signed-off-by: Johannes Schindelin --- src/index.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/index.js b/src/index.js index ceca5ac5..ee07bcf1 100644 --- a/src/index.js +++ b/src/index.js @@ -80,6 +80,7 @@ export async function run() { let newSessionExtra = "" let tmateSSHDashI = "" + let publicSSHKeysWarning = "" const limitAccessToActor = core.getInput("limit-access-to-actor") if (limitAccessToActor === "true" || limitAccessToActor === "auto") { const { actor, apiUrl } = github.context @@ -90,7 +91,7 @@ export async function run() { username: actor }) if (keys.data.length === 0) { - if (limitAccessToActor === "auto") core.info(`No public SSH keys found for ${actor}; continuing without them`) + if (limitAccessToActor === "auto") publicSSHKeysWarning = `No public SSH keys found for ${actor}; continuing without them even if it is less secure (please consider adding an SSH key, see https://docs.github.com/en/authentication/connecting-to-github-with-ssh/adding-a-new-ssh-key-to-your-github-account)` else throw new Error(`No public SSH keys registered with ${actor}'s GitHub profile`) } else { const sshPath = path.join(os.homedir(), ".ssh") @@ -138,6 +139,9 @@ export async function run() { core.debug("Entering main loop") while (true) { + if (publicSSHKeysWarning) { + core.warning(publicSSHKeysWarning) + } if (tmateWeb) { core.info(`Web shell: ${tmateWeb}`); } From 4d571a0f39394f14cc6b4496c288c3ccdd6dcb4d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 18 Jan 2023 14:02:02 +0100 Subject: [PATCH 4/4] npm run build Signed-off-by: Johannes Schindelin --- lib/index.js | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/lib/index.js b/lib/index.js index ab808329..1b0716bb 100644 --- a/lib/index.js +++ b/lib/index.js @@ -12630,7 +12630,10 @@ async function run() { } let newSessionExtra = "" - if (core.getInput("limit-access-to-actor") === "true") { + let tmateSSHDashI = "" + let publicSSHKeysWarning = "" + const limitAccessToActor = core.getInput("limit-access-to-actor") + if (limitAccessToActor === "true" || limitAccessToActor === "auto") { const { actor, apiUrl } = github.context const auth = core.getInput('github-token') const octokit = new dist_node/* Octokit */.v({ auth, baseUrl: apiUrl }) @@ -12639,13 +12642,16 @@ async function run() { username: actor }) if (keys.data.length === 0) { - throw new Error(`No public SSH keys registered with ${actor}'s GitHub profile`) + if (limitAccessToActor === "auto") publicSSHKeysWarning = `No public SSH keys found for ${actor}; continuing without them even if it is less secure (please consider adding an SSH key, see https://docs.github.com/en/authentication/connecting-to-github-with-ssh/adding-a-new-ssh-key-to-your-github-account)` + else throw new Error(`No public SSH keys registered with ${actor}'s GitHub profile`) + } else { + const sshPath = external_path_default().join(external_os_default().homedir(), ".ssh") + await external_fs_default().promises.mkdir(sshPath, { recursive: true }) + const authorizedKeysPath = external_path_default().join(sshPath, "authorized_keys") + await external_fs_default().promises.writeFile(authorizedKeysPath, keys.data.map(e => e.key).join('\n')) + newSessionExtra = `-a "${authorizedKeysPath}"` + tmateSSHDashI = "ssh -i " } - const sshPath = external_path_default().join(external_os_default().homedir(), ".ssh") - await external_fs_default().promises.mkdir(sshPath, { recursive: true }) - const authorizedKeysPath = external_path_default().join(sshPath, "authorized_keys") - await external_fs_default().promises.writeFile(authorizedKeysPath, keys.data.map(e => e.key).join('\n')) - newSessionExtra = `-a "${authorizedKeysPath}"` } const tmate = `${tmateExecutable} -S /tmp/tmate.sock`; @@ -12684,10 +12690,16 @@ async function run() { core.debug("Entering main loop") while (true) { + if (publicSSHKeysWarning) { + core.warning(publicSSHKeysWarning) + } if (tmateWeb) { core.info(`Web shell: ${tmateWeb}`); } core.info(`SSH: ${tmateSSH}`); + if (tmateSSHDashI) { + core.info(`or: ${tmateSSH.replace(/^ssh/, tmateSSHDashI)}`) + } if (continueFileExists()) { core.info("Exiting debugging session because the continue file was created")