-
Notifications
You must be signed in to change notification settings - Fork 53
CPD-11979: Replace SSH with Teleport (tsh ssh) for instance access #337
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
base: proxy-cli-hook-aws-v3
Are you sure you want to change the base?
Changes from all commits
97dac33
2107b76
818342b
0ed6f09
f5c1fa0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -186,10 +186,13 @@ def ssh | |||||||||||||||||||||||||||||||||
| @config.ssh_instance ||= SSHTargetSelector.new( | ||||||||||||||||||||||||||||||||||
| stack, asg_name: @config.ssh_auto_scaling_group_name | ||||||||||||||||||||||||||||||||||
| ).choose! | ||||||||||||||||||||||||||||||||||
| cb = SSHCommandBuilder.new(@config.ssh_config, @config.ssh_instance) | ||||||||||||||||||||||||||||||||||
| teleport_config = TeleportConfig.new( | ||||||||||||||||||||||||||||||||||
| stack.name, @config.ssh_config.ssh_user, ENV['AWS_REGION'] | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+189
to
+190
|
||||||||||||||||||||||||||||||||||
| teleport_config = TeleportConfig.new( | |
| stack.name, @config.ssh_config.ssh_user, ENV['AWS_REGION'] | |
| region = ENV['AWS_REGION'].to_s.strip | |
| if region.empty? | |
| raise 'AWS_REGION environment variable must be set (for example, "us-east-1") to establish a Teleport SSH connection.' | |
| end | |
| teleport_config = TeleportConfig.new( | |
| stack.name, @config.ssh_config.ssh_user, region |
Copilot
AI
Feb 27, 2026
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 @config.ssh_config.ssh_user is nil (for example, if neither the environment variable MOONSHOT_SSH_USER nor a configuration value is set), it will be converted to an empty string by TeleportConfig. This will result in an invalid Teleport hostname format like '@i-xxx.region.account' (missing username) and bot_user? will always return false (since '' != 'clouddatabot'). Consider validating that ssh_user is set before constructing the TeleportConfig, or add validation within TeleportConfig.initialize.
| teleport_config = TeleportConfig.new( | |
| stack.name, @config.ssh_config.ssh_user, ENV['AWS_REGION'] | |
| ssh_user = @config.ssh_config.ssh_user | |
| if ssh_user.nil? || ssh_user.to_s.empty? | |
| raise 'SSH user is not configured. Please set MOONSHOT_SSH_USER or configure ssh_user in your Moonshot configuration.' | |
| end | |
| teleport_config = TeleportConfig.new( | |
| stack.name, ssh_user, ENV['AWS_REGION'] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,33 +3,30 @@ | |
| require 'shellwords' | ||
|
|
||
| module Moonshot | ||
| # Create an ssh command from configuration. | ||
| # Create a tsh ssh command from configuration. | ||
| class SSHCommandBuilder | ||
| Result = Struct.new(:cmd, :ip) | ||
| Result = Struct.new(:cmd, :host) | ||
|
|
||
| def initialize(ssh_config, instance_id) | ||
| @config = ssh_config | ||
| @instance_id = instance_id | ||
| def initialize(ssh_config, instance_id, teleport_config) | ||
|
||
| @config = ssh_config | ||
| @instance_id = instance_id | ||
| @teleport_config = teleport_config | ||
| end | ||
|
Comment on lines
+10
to
14
|
||
|
|
||
| def build(command = nil) | ||
| cmd = ['ssh', '-t'] | ||
| cmd = ['tsh', 'ssh'] | ||
| cmd << @config.ssh_options if @config.ssh_options | ||
| cmd << "-i #{@config.ssh_identity_file}" if @config.ssh_identity_file | ||
| cmd << "-l #{@config.ssh_user}" if @config.ssh_user | ||
| cmd << instance_ip | ||
| cmd << "--proxy=#{@teleport_config.proxy_url}" | ||
| cmd << "-i #{@teleport_config.identity_file}" if @teleport_config.bot_user? | ||
| cmd << "#{@teleport_config.ssh_user}@#{instance_host}" | ||
|
Comment on lines
+17
to
+21
|
||
| cmd << Shellwords.escape(command) if command | ||
| Result.new(cmd.join(' '), instance_ip) | ||
| Result.new(cmd.join(' '), instance_host) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def instance_ip | ||
| @instance_ip ||= Aws::EC2::Client.new | ||
| .describe_instances(instance_ids: [@instance_id]) | ||
| .reservations.first.instances.first.public_ip_address | ||
| rescue StandardError | ||
| raise "Failed to determine public IP address for instance #{@instance_id}!" | ||
| def instance_host | ||
| @instance_host ||= @teleport_config.host_for(@instance_id) | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,57 @@ | ||||||||||||||||||||||
| # frozen_string_literal: true | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| module Moonshot | ||||||||||||||||||||||
| # Encapsulates Teleport SSH configuration derived from the stack name, | ||||||||||||||||||||||
| # SSH user, and AWS region. Determines the correct proxy URL, account ID, | ||||||||||||||||||||||
| # and identity file path for both normal users and bot users. | ||||||||||||||||||||||
| class TeleportConfig | ||||||||||||||||||||||
|
Comment on lines
+4
to
+7
|
||||||||||||||||||||||
| PROD_ACCOUNT_ID = '546349603759' | ||||||||||||||||||||||
| DEV_ACCOUNT_ID = '672327909798' | ||||||||||||||||||||||
|
Comment on lines
+8
to
+9
|
||||||||||||||||||||||
| PROD_ACCOUNT_ID = '546349603759' | |
| DEV_ACCOUNT_ID = '672327909798' | |
| PROD_ACCOUNT_ID = ENV['TELEPORT_PROD_ACCOUNT_ID'] || '546349603759' | |
| DEV_ACCOUNT_ID = ENV['TELEPORT_DEV_ACCOUNT_ID'] || '672327909798' |
Copilot
AI
Mar 11, 2026
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.
TeleportConfig's identity file constants use '/opt/machine-id/...', but the new spec expects 'tbot-auth-.../identity' paths for the bot user. As written, TeleportConfig#identity_file will not match the spec (and likely the intended runtime path). Please align the constants/logic and the spec so they agree on the actual identity file location.
| PROD_IDENTITY_TEMPLATE = '/opt/machine-id/%<region>s/identity' | |
| DEV_IDENTITY = '/opt/machine-id/dev-us-east-1/identity' | |
| PROD_IDENTITY_TEMPLATE = '/opt/tbot-auth-%<region>s/identity' | |
| DEV_IDENTITY = '/opt/tbot-auth-dev-us-east-1/identity' |
Copilot
AI
Feb 27, 2026
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.
The initialize method converts nil parameters to empty strings via .to_s without validation. This leads to multiple issues: (1) empty region produces malformed URLs like '.teleport.cloudservices.acquia.io' and invalid identity paths like 'tbot-auth-/identity', (2) empty ssh_user produces invalid Teleport hostnames like '@instance.region.account', (3) empty stack_name will cause incorrect environment detection (will default to dev since 'prod' won't be found). Add validation to ensure all three parameters are present and non-empty before proceeding.
| def initialize(stack_name, ssh_user, region) | |
| def initialize(stack_name, ssh_user, region) | |
| missing_params = [] | |
| missing_params << 'stack_name' if stack_name.to_s.strip.empty? | |
| missing_params << 'ssh_user' if ssh_user.to_s.strip.empty? | |
| missing_params << 'region' if region.to_s.strip.empty? | |
| unless missing_params.empty? | |
| raise ArgumentError, "TeleportConfig requires non-empty: #{missing_params.join(', ')}" | |
| end |
Copilot
AI
Mar 11, 2026
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.
TeleportConfig coerces a nil region to an empty string via region.to_s, which can silently build an invalid proxy URL/hostname when AWS_REGION is unset. Previously this would fail earlier via AWS SDK missing-region behavior; consider validating region is present/non-empty and raising an ArgumentError when it isn't.
Copilot
AI
Feb 27, 2026
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.
The prod? method uses a simple string inclusion check (include?('prod')) to determine the environment. This could lead to false positives if 'prod' appears anywhere in the stack name, not just as the environment identifier. For example, a stack named 'product-dev' or 'reproduce-test' would incorrectly be identified as production. Consider using a more robust pattern match or explicit environment detection.
| @stack_name.include?('prod') | |
| @stack_name.match?(/\bprod\b/) |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -10,6 +10,10 @@ def initialize(response) | |||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| class SSH | ||||||||||||||||||||||||||||
| def initialize(resources) | ||||||||||||||||||||||||||||
| @resources = resources | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| # As per the standard it is raising correctly but still giving an error. | ||||||||||||||||||||||||||||
| def test_ssh_connection(instance_id) | ||||||||||||||||||||||||||||
| Retriable.retriable(base_interval: 5, tries: 3) do | ||||||||||||||||||||||||||||
|
|
@@ -29,7 +33,12 @@ def exec(command, instance_id) | |||||||||||||||||||||||||||
| private | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def build_command(command, instance_id) | ||||||||||||||||||||||||||||
| cb = SSHCommandBuilder.new(Moonshot.config.ssh_config, instance_id) | ||||||||||||||||||||||||||||
| teleport_config = Moonshot::TeleportConfig.new( | ||||||||||||||||||||||||||||
| @resources.controller.stack.name, | ||||||||||||||||||||||||||||
| Moonshot.config.ssh_config.ssh_user, | ||||||||||||||||||||||||||||
|
Comment on lines
+36
to
+38
|
||||||||||||||||||||||||||||
| teleport_config = Moonshot::TeleportConfig.new( | |
| @resources.controller.stack.name, | |
| Moonshot.config.ssh_config.ssh_user, | |
| ssh_user = Moonshot.config.ssh_config.ssh_user | |
| if ssh_user.nil? || ssh_user.strip.empty? | |
| raise ArgumentError, | |
| 'SSH user is not configured. Set MOONSHOT_SSH_USER or ssh_config.ssh_user.' | |
| end | |
| teleport_config = Moonshot::TeleportConfig.new( | |
| @resources.controller.stack.name, | |
| ssh_user, |
Copilot
AI
Feb 27, 2026
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.
The AWS_REGION environment variable is accessed without validation. If ENV['AWS_REGION'] is nil or empty, this will create a TeleportConfig with an empty string for the region parameter (due to .to_s conversion), which could lead to incorrect proxy URLs in production environments (e.g., ".teleport.cloudservices.acquia.io") and malformed hostnames. Consider adding validation to raise a clear error message if AWS_REGION is not set, similar to how CodeDeploySetup handles this case.
| teleport_config = Moonshot::TeleportConfig.new( | |
| @resources.controller.stack.name, | |
| Moonshot.config.ssh_config.ssh_user, | |
| ENV['AWS_REGION'] | |
| region = ENV['AWS_REGION'] | |
| if region.nil? || region.empty? | |
| raise 'AWS_REGION environment variable must be set for SSH teleport configuration' | |
| end | |
| teleport_config = Moonshot::TeleportConfig.new( | |
| @resources.controller.stack.name, | |
| Moonshot.config.ssh_config.ssh_user, | |
| region |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| describe Moonshot::TeleportConfig do | ||
| let(:instance_id) { 'i-0036e48e43b79740f' } | ||
|
|
||
| describe 'prod environment (stack name contains "prod")' do | ||
| subject { described_class.new('myapp-prod-us-east-1', 'joeuser', 'us-east-1') } | ||
|
|
||
| it 'uses region-based proxy URL' do | ||
| expect(subject.proxy_url).to eq('us-east-1.teleport.cloudservices.acquia.io') | ||
| end | ||
|
|
||
| it 'uses the prod account ID' do | ||
| expect(subject.account_id).to eq('546349603759') | ||
| end | ||
|
|
||
| it 'builds the Teleport hostname correctly' do | ||
| expect(subject.host_for(instance_id)).to eq( | ||
| 'i-0036e48e43b79740f.us-east-1.546349603759' | ||
| ) | ||
| end | ||
|
|
||
| it 'is not a bot user for a normal user' do | ||
| expect(subject.bot_user?).to be false | ||
| end | ||
|
|
||
| it 'returns nil identity_file for normal user' do | ||
| expect(subject.identity_file).to be_nil | ||
| end | ||
|
|
||
| context 'with bot user (clouddatabot)' do | ||
| subject { described_class.new('myapp-prod-us-east-1', 'clouddatabot', 'us-east-1') } | ||
|
|
||
| it 'is identified as a bot user' do | ||
| expect(subject.bot_user?).to be true | ||
| end | ||
|
|
||
| it 'uses the region-based identity file path' do | ||
| expect(subject.identity_file).to eq('/opt/machine-id/us-east-1/identity') | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe 'dev environment (stack name does not contain "prod")' do | ||
| subject { described_class.new('myapp-dev-jsmith', 'joeuser', 'us-east-1') } | ||
|
|
||
| it 'uses the shared dev proxy URL' do | ||
| expect(subject.proxy_url).to eq('teleport.dev.cloudservices.acquia.io') | ||
| end | ||
|
|
||
| it 'uses the dev account ID' do | ||
| expect(subject.account_id).to eq('672327909798') | ||
| end | ||
|
|
||
| it 'builds the Teleport hostname correctly' do | ||
| expect(subject.host_for(instance_id)).to eq( | ||
| 'i-0036e48e43b79740f.us-east-1.672327909798' | ||
| ) | ||
| end | ||
|
|
||
| it 'is not a bot user for a normal user' do | ||
| expect(subject.bot_user?).to be false | ||
| end | ||
|
|
||
| context 'with bot user (clouddatabot)' do | ||
| subject { described_class.new('myapp-dev-jsmith', 'clouddatabot', 'us-east-1') } | ||
|
|
||
| it 'is identified as a bot user' do | ||
| expect(subject.bot_user?).to be true | ||
| end | ||
|
|
||
| it 'uses the dev identity file path' do | ||
| expect(subject.identity_file).to eq('/opt/machine-id/dev-us-east-1/identity') | ||
| end | ||
| end | ||
| end | ||
| end |
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.
rexmlis added only in the:testgroup, but the PR description indicates it's required by neweraws-sdk-core(a runtime dependency). If this gem is needed at runtime, it should be added as a normal dependency (e.g., in the gemspec) or outside the:testgroup so non-test installs don't break.