Multiple worker_gateway.authorized_keys passed in as an array results in none being used by the TSA process

[edwardecook]

Bug Report

Hi, we (Redis for PCF) have a concourse deployment that includes both internal workers (which all have the same worker key) and external workers (which have a different worker key). We have recently started work on upgrading from 4.2.3 to 5.0.0 and found that the way in which we are templating in our web authorized keys:

properties:
  worker_gateway:
    authorized_keys: [((external_worker_key.public_key)), ((internal_worker_key.public_key))]

does not work despite the suggestion in the spec that it should:

concourse-bosh-release/jobs/web/spec

Lines 1100 to 1101 in 0265654

	Public keys to authorize for SSH connections. Either a string with one
	public key per line, or an array of public keys.

It appears that the issue revolves around the fact that the env_file_writer func,

concourse-bosh-release/jobs/web/templates/pre_start.erb

Lines 22 to 29 in 0265654

	def env_file_writer(v, env)
	path = "/var/vcap/jobs/web/config/env/#{env}"
	case v
	when Array
	v.collect.with_index do |v, i|
	"cat > #{path}_#{i} <<\"ENVGEN_EOF\"\n#{env_file_content(v)}ENVGEN_EOF\n"
	end.join("\n\n")

puts values into separate files when given an array and then passes through a comma separated list of files,

concourse-bosh-release/jobs/web/templates/bpm.yml.erb

Line 840 in 0265654

CONCOURSE_TSA_TEAM_AUTHORIZED_KEYS: <%= env_file_flag(v, "CONCOURSE_TSA_TEAM_AUTHORIZED_KEYS").to_json %>

while the TSA command expects to be passed a single file path: https://github.com/concourse/concourse/blob/c0422a90aed2ff18b8411b9f5dcaf87b784802b8/tsa/tsacmd/command.go#L35

• Concourse version: 5.0.0
• Deployment type (BOSH/Docker/binary): BOSH
• Infrastructure/IaaS: GCP
• Browser (if applicable): N/A
• Did this used to work? Yes

Note: as a workaround we have moved to passing the authorized keys line separated in a single string which works:

properties:
  worker_gateway:
    authorized_keys: |
      ((external_worker_key.public_key))
      ((internal_worker_key.public_key))

cc @jplebre

[cirocosta]

Hi @edwardecook ,

Thanks for putting the time to describe the issue so well!
Do you think it'd suffice to have an example as we have for team_authorized_keys?

concourse-bosh-release/jobs/web/spec

Lines 1104 to 1114 in 0265654

	worker_gateway.team_authorized_keys:
	env_file: CONCOURSE_TSA_TEAM_AUTHORIZED_KEYS
	description: |
	Public keys to authorize for per-team workers.
	Map from team name to authorized keys, either as a string with one key
	per line or an array of public keys.
	example:
	concourse: |
	ssh-rsa key key@pivotal.io
	default: {}

Such an example could perhaps have multiple keys concatenated there as you mentioned.

Wdyt?

thx!

[edwardecook]

Hi @cirocosta, I think an example would certainly be reasonable, the issue I would have is that it does feel like a bit of a regression with 5.0.0 as with 4.2.3 passing in an array worked.

[cirocosta]

Oh, I see, indeed!

(from 4.2.3 tag):

concourse-bosh-release/jobs/tsa/spec

Lines 70 to 73 in f3af9eb

	authorized_keys:
	description: |
	Public keys to authorize for SSH connections.
	default: []

[cirocosta]

cc @vito, how do you see these changes being documented? I remember talking about something related the other day, but I don't remember the output of that 😬 Should we document this separately from concourse-ci.org?

[vito]

Yeah, sorry about the lack of release notes for this. I wonder if we should start giving this repo its own release cycle and its own release notes; I tend to focus on the core feature set for concourse-ci.org - it's also doesn't even mention all our work on the Helm chart. 🤔

edit: opened #21 based on this

[vito]

Now that I look at it, this definitely doesn't seem to be working how I intended - as @edwardecook pointed out, the spec description says an array should work, and that description was written fresh for 5.0.

I'm almost certain I had this working at some point, but maybe I had to change it for some other property that required its array value to be passed one-by-one. Looking at the other properties, I can't find any though. The only hint I can find is that env_file_content handles array values by line-separating them. It's env_file_writer and env_file_flag that seem broken.

Probably a good time to backfill tests for all this. There's a start on this in #20.

[vito]

This will be out in 5.0.1 soon (aiming for today).