fleetctl appends port number in ~/.fleetctl/known_hosts #410

jonboulle · 2014-05-08T01:17:08Z

By default when generating a known_hosts file, fleet appends the port number. This is problematic for two reasons.

if the port number is 22 it should not be included
if the port number is not 22 then the ip address should be enclosed in square braces and the port should be appended afterwards (e.g. [192.168.1.1]:22). This is because IPv6 addresses utilize colons as a delimiter. For more information, man sshd - section "SSH_KNOWN_HOSTS FILE FORMAT".

This change also adds sshDefaultPort in the ssh module only if necessary rather than tacking it on arbitrarily in fleetctl.

jonboulle · 2014-05-08T01:17:21Z

@brianredbeard ptal

jonboulle · 2014-05-08T02:45:13Z

Actually, stand by - I'm going to rework the known hosts functionality to better match the spec.

jonboulle · 2014-05-08T08:28:33Z

Well, that was a fun rabbit hole. It turns out the known hosts spec is rather.. intricate, and we were barely scratching the surface of it.

This implements the vast majority of it (and adds coverage for pretty much all new code), but still a bit more cleanup to be done.

bcwaldon · 2014-05-08T14:53:36Z

ssh/known_hosts.go

+	var i, j int
+	for i < len(p) {
+		if p[i] == '*' {
+			/* Skip the asterisk. */


Its like these comments read with an Australian accent...

Finnish, actually...

http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/match.c?rev=1.29;content-type=text%2Fplain

Could we keep with the style of the rest of the codebase and use double-slash comments?

bcwaldon · 2014-05-08T19:39:49Z

ssh/known_hosts_test.go

@@ -92,6 +92,7 @@ func TestHostLine(t *testing.T) {
 }

 // TestHostKeyFile tests to read and write from HostKeyFile
+/*


Patience young padawan

Ok, should I hold off on reviewing this PR for now?

Well I'm reworking the tests, but feel free to tear into the rest.

bcwaldon · 2014-05-09T19:00:56Z

@jonboulle Not sure where we're at here, but I tested it out anyways:

% cat ~/.fleetctl/known_hosts
127.0.0.1:2222 ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCki6T4lAFTlJ0HzuboJoB83aiDCjDiXm2DCjg0FLsAunQrMH3Z9tEko5j7d3G/GJTdBqdhFAkMSbbS7ceJYUTkRuHLE1VXQSbyhJCeNT/Cw8rQxfi2pq45AylGzHw304IUBZxOwsnSKsK9uWK3XRMNO+1oQs0ZrPvFkiPkq7Szd0e7FkaF7RDoe4ZHPw8EONcnbhiCfFw6W1ZaCoGTFdk6B3Rn9JoGsYHsyU+42yz7oGxeFQZzUmko4xc842aAFoT9geWhsD1m0AQgrsZ/e1l73WaAiaYhPzgaHcUN2naewsfP9e6qqBvaMPTmPoy9RP3JsZREEh6NLImoo3/kPTxf

The hostname in that entry doesn't seem correct. Shouldn't it be [127.0.0.1]:2222?

jonboulle · 2014-05-09T19:59:34Z

You are correct. I misread net.JoinHostPort, it doesn't actually do what we want - so now we just use a direct translation from openssh.

jonboulle · 2014-05-09T20:00:01Z

(also, the test was wrong - fixed)

bcwaldon · 2014-05-09T20:17:37Z

Working as expected now, but what do you think about compatibility with the old host format? I ran both latest and this fleetctl and they ignored eachother in the known_hosts file. Maybe this patch should take into account the old host format?

% cat ~/.fleetctl/known_hosts
127.0.0.1:2222 ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCki6T4lAFTlJ0HzuboJoB83aiDCjDiXm2DCjg0FLsAunQrMH3Z9tEko5j7d3G/GJTdBqdhFAkMSbbS7ceJYUTkRuHLE1VXQSbyhJCeNT/Cw8rQxfi2pq45AylGzHw304IUBZxOwsnSKsK9uWK3XRMNO+1oQs0ZrPvFkiPkq7Szd0e7FkaF7RDoe4ZHPw8EONcnbhiCfFw6W1ZaCoGTFdk6B3Rn9JoGsYHsyU+42yz7oGxeFQZzUmko4xc842aAFoT9geWhsD1m0AQgrsZ/e1l73WaAiaYhPzgaHcUN2naewsfP9e6qqBvaMPTmPoy9RP3JsZREEh6NLImoo3/kPTxf
[127.0.0.1]:2222 ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCki6T4lAFTlJ0HzuboJoB83aiDCjDiXm2DCjg0FLsAunQrMH3Z9tEko5j7d3G/GJTdBqdhFAkMSbbS7ceJYUTkRuHLE1VXQSbyhJCeNT/Cw8rQxfi2pq45AylGzHw304IUBZxOwsnSKsK9uWK3XRMNO+1oQs0ZrPvFkiPkq7Szd0e7FkaF7RDoe4ZHPw8EONcnbhiCfFw6W1ZaCoGTFdk6B3Rn9JoGsYHsyU+42yz7oGxeFQZzUmko4xc842aAFoT9geWhsD1m0AQgrsZ/e1l73WaAiaYhPzgaHcUN2naewsfP9e6qqBvaMPTmPoy9RP3JsZREEh6NLImoo3/kPTxf

brianredbeard · 2014-05-09T20:19:14Z

I think if it politely ignores old entries and doesn't put undue burden on
the users it should be fine. Thoughts?

On Fri, May 9, 2014 at 1:17 PM, Brian Waldon [email protected]:

Working as expected now, but what do you think about compatibility with
the old host format? I ran both latest and this fleetctl and they ignored
eachother in the known_hosts file. Maybe this patch should take into
account the old host format?

% cat ~/.fleetctl/known_hosts127.0.0.1:2222 ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCki6T4lAFTlJ0HzuboJoB83aiDCjDiXm2DCjg0FLsAunQrMH3Z9tEko5j7d3G/GJTdBqdhFAkMSbbS7ceJYUTkRuHLE1VXQSbyhJCeNT/Cw8rQxfi2pq45AylGzHw304IUBZxOwsnSKsK9uWK3XRMNO+1oQs0ZrPvFkiPkq7Szd0e7FkaF7RDoe4ZHPw8EONcnbhiCfFw6W1ZaCoGTFdk6B3Rn9JoGsYHsyU+42yz7oGxeFQZzUmko4xc842aAFoT9geWhsD1m0AQgrsZ/e1l73WaAiaYhPzgaHcUN2naewsfP9e6qqBvaMPTmPoy9RP3JsZREEh6NLImo
o3/kPTxf
[127.0.0.1]:2222 ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCki6T4lAFTlJ0HzuboJoB83aiDCjDiXm2DCjg0FLsAunQrMH3Z9tEko5j7d3G/GJTdBqdhFAkMSbbS7ceJYUTkRuHLE1VXQSbyhJCeNT/Cw8rQxfi2pq45AylGzHw304IUBZxOwsnSKsK9uWK3XRMNO+1oQs0ZrPvFkiPkq7Szd0e7FkaF7RDoe4ZHPw8EONcnbhiCfFw6W1ZaCoGTFdk6B3Rn9JoGsYHsyU+42yz7oGxeFQZzUmko4xc842aAFoT9geWhsD1m0AQgrsZ/e1l73WaAiaYhPzgaHcUN2naewsfP9e6qqBvaMPTmPoy9RP3JsZREEh6NLImoo3/kPTxf

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/410#issuecomment-42708925
.

bcwaldon · 2014-05-09T20:24:41Z

@brianredbeard I think I agree, just wanted to have the conversation on the record. At least we don't have to worry about fleetctl ignoring host keys or something terrible like that.

jonboulle · 2014-05-09T20:39:49Z

Yeah, I'd rather avoid accounting for the old host format - it complicates the logic a bit, and technically it's an invalid format anyway (our bad).

bcwaldon · 2014-05-09T20:44:40Z

🐑 it

bcwaldon · 2014-05-09T22:36:34Z

btw that 🐑 it is a 🚢 it

jonboulle · 2014-05-09T22:38:00Z

Ship from phone.

fleetctl appends port number in ~/.fleetctl/known_hosts

bcwaldon · 2014-05-09T22:38:42Z

Like a boss

brianredbeard added the bug label May 7, 2014

jonboulle assigned brianredbeard and jonboulle and unassigned brianredbeard May 7, 2014

bcwaldon added this to the v0.3.1 milestone May 7, 2014

fix(ssh): format host entries in known_hosts correctly

63fee4e

This change also adds sshDefaultPort in the ssh module only if necessary rather than tacking it on arbitrarily in fleetctl.

refactor(ssh): improve known hosts handling to better match spec

ee2f73c

bcwaldon reviewed May 8, 2014
View reviewed changes

chore(ssh): add LICENSE information

83a3302

bcwaldon reviewed May 8, 2014
View reviewed changes

jonboulle added 5 commits May 8, 2014 12:42

chore(ssh): change comments to slashes

8e7b4b5

refactor(ssh): remove unused code, clean up

455630a

chore(ssh): switch to glog

7fd7587

chore(ssh): switch to glog

5560f3b

refactor(ssh): split out match/matchHost to separate files

f36f76a

fix(ssh): fix tests, further cleanup for simplification

1f5a9f4

fix(ssh): finish docstring

52c1870

jonboulle added a commit that referenced this pull request May 9, 2014

Merge pull request #410 from jonboulle/410

96494da

fleetctl appends port number in ~/.fleetctl/known_hosts

jonboulle merged commit 96494da into coreos:master May 9, 2014

jonboulle deleted the 410 branch May 9, 2014 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fleetctl appends port number in ~/.fleetctl/known_hosts #410

fleetctl appends port number in ~/.fleetctl/known_hosts #410

jonboulle commented May 8, 2014

jonboulle commented May 8, 2014

jonboulle commented May 8, 2014

jonboulle commented May 8, 2014

bcwaldon May 8, 2014

jonboulle May 8, 2014

bcwaldon May 8, 2014

jonboulle May 8, 2014

bcwaldon May 8, 2014

jonboulle May 8, 2014

bcwaldon May 8, 2014

jonboulle May 8, 2014

bcwaldon commented May 9, 2014

jonboulle commented May 9, 2014

jonboulle commented May 9, 2014

bcwaldon commented May 9, 2014

brianredbeard commented May 9, 2014

bcwaldon commented May 9, 2014

jonboulle commented May 9, 2014

bcwaldon commented May 9, 2014

bcwaldon commented May 9, 2014

jonboulle commented May 9, 2014

bcwaldon commented May 9, 2014

fleetctl appends port number in ~/.fleetctl/known_hosts #410

fleetctl appends port number in ~/.fleetctl/known_hosts #410

Conversation

jonboulle commented May 8, 2014

jonboulle commented May 8, 2014

jonboulle commented May 8, 2014

jonboulle commented May 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcwaldon commented May 9, 2014

jonboulle commented May 9, 2014

jonboulle commented May 9, 2014

bcwaldon commented May 9, 2014

brianredbeard commented May 9, 2014

bcwaldon commented May 9, 2014

jonboulle commented May 9, 2014

bcwaldon commented May 9, 2014

bcwaldon commented May 9, 2014

jonboulle commented May 9, 2014

bcwaldon commented May 9, 2014