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

Lance/repros support #486

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from
Draft

Lance/repros support #486

wants to merge 20 commits into from

Conversation

lrvick
Copy link
Contributor

@lrvick lrvick commented Oct 28, 2024

Summary & Motivation (Problem vs. Solution)

How I Tested These Changes

Pre merge check list

  • Update CHANGELOG.MD

@lrvick lrvick requested a review from a team as a code owner October 28, 2024 23:20
@cr-tk
Copy link
Collaborator

cr-tk commented Oct 30, 2024

Hi @lrvick , I have a series of questions and comments on this.

First, is this a branch that's still in flux (= draft PR) or a full PR that's ready for comments and review?

I'm not aware of an engineering ticket on Linear that's linked to this (not totally surprising based on recent conversations). However, the PR itself also has no code comments, notes on non-trivial/potentially unexpected parts, or a high-level summary.
As such, I think it's under-documented, which makes it hard to give good feedback on the PR or do the (eventual) code review.

The primary reason I'm mentioning this is are the changes to .github/workflows/artifacts.yml, which tingle my security sense 🕷️ 🙂

What is this 144.76.154.76 host and git repository that the ssh-based git checkout access pushes to? Reverse lookups and whois information suggest this is a physical rented server at Hetzner in Germany. I wasn't aware Turnkey has any machines or Git repository there.

The StrictHostKeyChecking no ssh config option intentionally weakens security for the ssh connection, which allows MITM network attackers to impersonate the target server, and (likely) read pushed data or cause other impacts that are worse.
AFAIK ssh-keyscan -H ${{matrix.host}} >> ~/.ssh/known_hosts does not prevent this - it pre-fills the known_hosts file with what the check sees at runtime (which may already be malicious), but since strict host key checking is off, it won't reliably stop MITM attacks even if they happen between this command and the following git push repros-lance HEAD step.

I expect StrictHostKeyChecking accept-new to be a slightly better variant of this, since it will automatically trust the presented hostkey on first use (TOFU principle) without the user interaction of the ask setting but strongly enforce host key identity afterwards. It's not good enough, though.

As outlined, I think TOFU isn't a good choice here, since the TOFU check and the usage are so close to each other and will repeat on every GitHub action run. Instead, I highly recommend configuring the runner with the pre-determined fixed expected host keys, namely:

ssh-keyscan -H 144.76.154.76
# 144.76.154.76:22 SSH-2.0-OpenSSH_9.8
|1|+VwPWywRZn1LgL2cDFGbtuK5GR8=|/7HRKIjUntxCUuollX5hmlcLORA= ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQCtuvpTCcfre5CyiuaiY+/6UTpp9uCUgfidGgFfCLWA1Xoy4JB7wB2uFj6atmWxsL5cPp3sLQ9CAHEo+RPIpb+C1IXyJ/yLx1rgHA+3DVSuZdwmchk4ufZ066HTAcXZQYTxInlUaT8Guza3KtZfYow4YzJu7w99lKF4AqRYJMBhVHDeR1XildX/i1jwGHM1ha1p8BDxHQDxKbUhV9TJ0EFDTofXoxhBqXkW5LWcKBKfDF6OmelddUlarMYeGFsy+4ZbU1tm3xLiFFdNRP0b5kzRABH/uXkDso+VhSPGAe39b99dJyDb+EXHXsTMxE0YxLsPfmaWNdYZ2odg9qscIIyb1W/sezKfFPEshPF6Qd5Iav68sFm0Nr3k9cbwSMSYQXyJVVUmcrHIItmx7oyHSR3AeZNJemHRP/T3DwccNpzC/EiAv81FSwsuZl0TwKQAJob4PaS4WPrHdS8zhaAhzKFarTWvj6x4mLhI3wyuwbnZg2z/wpdvVHf07j3UvogdI4s=
# 144.76.154.76:22 SSH-2.0-OpenSSH_9.8
|1|3eCUr8YmgaD6U/cvZvYS/U11mfI=|COD/H9+MM4oi//WrS0v35KW+48k= ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBDl2cxE8PADyJoFfYStCdp9X3OOQN6zmmrsMktxQsclpBMUwy2n3xhQhfvClbP40ZbEbeyeiPHIqU9lF+S/ldq4=
# 144.76.154.76:22 SSH-2.0-OpenSSH_9.8
|1|VCPdbrd4ytMPF4IJ5bPbnWMaOCc=|zjhcSjM2986XPRA3BmAccYB9LGw= ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIHuwf76zC66gC+3T/9XMvju3lcE4P0o5Ngxo5ZK6QSgS
# 144.76.154.76:22 SSH-2.0-OpenSSH_9.8
# 144.76.154.76:22 SSH-2.0-OpenSSH_9.8

(Side note, the benefit from the -H = hashed behavior is probably marginal, since the target IP address is present in multiple other places in the job / on the runner, but it doesn't really hurt to keep the flag).

One practical scenario where the current definition would hurt us is if we cancel the Hetzner server, someone else gets authoritative control over the IP after it gets reassigned to another customer, and we forget about cancelling our runner job in time. Then they could setup a kind of ssh honeypot that accepts the incoming ssh + git requests and cause problems. This attack would not have to be Turnkey-specific or even targeted at us.

@cr-tk
Copy link
Collaborator

cr-tk commented Oct 30, 2024

On a related note, it would be helpful for https://github.com/tkhq/repros-sigs to have a README.md explaining the contents, and a short GitHub description.

@lrvick lrvick marked this pull request as draft November 20, 2024 19:30
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