Skip to content

Commit 47adbb1

Browse files
committed
provisioner: Fix ssh key validation (SOC-11126)
Fixed ssh keys validation wrt (optional) options field before the type/method in key line. Additional improvement is proper handling of comments with spaces and extended list of supported key types. (cherry picked from commit a19750f)
1 parent ab1d53b commit 47adbb1

File tree

6 files changed

+278
-8
lines changed

6 files changed

+278
-8
lines changed

chef/cookbooks/barclamp/libraries/barclamp_library.rb

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#
1515

1616
require_relative "conduit_resolver.rb"
17+
require_relative "ssh_key_parser.rb"
1718

1819
module BarclampLibrary
1920
class Barclamp
@@ -474,5 +475,9 @@ def load(group, barclamp, instance = nil)
474475
end
475476
end
476477
end
478+
479+
class SSHKeyParser
480+
include Crowbar::SSHKeyParser
481+
end
477482
end
478483
end
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
../../../../crowbar_framework/lib/crowbar/ssh_key_parser.rb

chef/cookbooks/provisioner/recipes/keys.rb

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,13 @@
3838

3939
# Add additional keys
4040
nullnodecounter = 0
41+
key_parser = BarclampLibrary::Barclamp::SSHKeyParser.new
4142
node["provisioner"]["access_keys"].strip.split("\n").each do |key|
4243
key.strip!
43-
unless key.empty?
44-
nodename = key.split(" ")[2] || "hostless-key-#{nullnodecounter += 1}"
45-
access_keys[nodename] = key
46-
end
44+
next if key.empty? || key[0] == "#"
45+
_, _, _, comment = key_parser.split_key_line(key)
46+
nodename = comment || "hostless-key-#{nullnodecounter += 1}"
47+
access_keys[nodename] = key
4748
end
4849

4950
# Find provisioner servers and include them.

crowbar_framework/app/models/provisioner_service.rb

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
#
1717

1818
class ProvisionerService < ServiceObject
19+
include Crowbar::SSHKeyParser
20+
1921
def initialize(thelogger = nil)
2022
super
2123
@bc_name = "provisioner"
@@ -57,11 +59,11 @@ def validate_proposal_after_save proposal
5759
proposal["attributes"]["provisioner"]["access_keys"].strip.split("\n").each do |key|
5860
key.strip!
5961

60-
next if key.empty?
62+
next if key.empty? || key[0] == "#"
6163

62-
method, public_key, comment = key.split(" ")
63-
unless ["ssh-rsa", "ssh-ed25519", "ssh-dss"].include? method
64-
validation_error("SSH key \"#{key}\" should start with a supported \"ssh-*\" method.")
64+
_, method, public_key, comment = split_key_line(key)
65+
unless valid_key_types.include? method
66+
validation_error("SSH key \"#{key}\" should contain a supported \"ssh-*\" method.")
6567
end
6668
if public_key.nil? || public_key.length < 64
6769
validation_error("SSH key \"#{key}\" has missing public key part or is too short.")
Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
#
2+
# Copyright 2020, SUSE
3+
#
4+
# Licensed under the Apache License, Version 2.0 (the "License");
5+
# you may not use this file except in compliance with the License.
6+
# You may obtain a copy of the License at
7+
#
8+
# http://www.apache.org/licenses/LICENSE-2.0
9+
#
10+
# Unless required by applicable law or agreed to in writing, software
11+
# distributed under the License is distributed on an "AS IS" BASIS,
12+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
# See the License for the specific language governing permissions and
14+
# limitations under the License.
15+
#
16+
17+
module Crowbar
18+
module SSHKeyParser
19+
# Splits authorized ssh key line into parts as described in
20+
# Returns [options, type, key, comment] (options and comment could be nil)
21+
# https://man.openbsd.org/sshd
22+
def split_key_line(line)
23+
line = line.clone
24+
res = []
25+
field = ""
26+
escape = false # was last char a backslash (so current one is escaped)?
27+
quotes = 0 # number of double quotes in current field
28+
type_found_at = nil # index of valid key type (if found)
29+
until line.empty?
30+
char = line.slice!(0)
31+
# count quotes for proper space handling
32+
quotes += 1 if char == "\"" && !escape
33+
# end of field, store and reset
34+
if char == " " && quotes.even?
35+
type_found_at = res.size if valid_key_types.include? field
36+
res.push(field) unless field.empty?
37+
field = ""
38+
quotes = 0
39+
# break parsing if we already have three fields (rest is comment)
40+
# OR we found a valid key type and one more field (the key)
41+
# this is equivalent to:
42+
# ... if res.size == 3 || \
43+
# type_found_at && res.size == type_found_at + 2
44+
break if res.size == (type_found_at || 1) + 2
45+
else
46+
field += char
47+
end
48+
escape = char == "\\"
49+
end
50+
# take the rest as-is
51+
field += line
52+
field.strip!
53+
res.push(field) unless field.empty?
54+
55+
# add missing options field
56+
res.unshift(nil) if type_found_at && type_found_at.zero?
57+
# add missing comment field
58+
res.push(nil) if res.size < 4
59+
res
60+
end
61+
62+
## Return valid key types
63+
def valid_key_types
64+
[
65+
"sk-ecdsa-sha2-nistp256@openssh.com",
66+
"ecdsa-sha2-nistp256",
67+
"ecdsa-sha2-nistp384",
68+
"ecdsa-sha2-nistp521",
69+
"sk-ssh-ed25519@openssh.com",
70+
"ssh-ed25519",
71+
"ssh-dss",
72+
"ssh-rsa"
73+
]
74+
end
75+
end
76+
end
Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
#
2+
# Copyright 2020, SUSE
3+
#
4+
# Licensed under the Apache License, Version 2.0 (the "License");
5+
# you may not use this file except in compliance with the License.
6+
# You may obtain a copy of the License at
7+
#
8+
# http://www.apache.org/licenses/LICENSE-2.0
9+
#
10+
# Unless required by applicable law or agreed to in writing, software
11+
# distributed under the License is distributed on an "AS IS" BASIS,
12+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
# See the License for the specific language governing permissions and
14+
# limitations under the License.
15+
#
16+
17+
require "spec_helper"
18+
19+
class DummyKeyParser
20+
include Crowbar::SSHKeyParser
21+
end
22+
23+
describe Crowbar::SSHKeyParser do
24+
subject { Crowbar::SSHKeyParser }
25+
26+
# parser doesn't verify key length or contens so we can test with short one
27+
DUMMY_KEY_CONTENTS = "QmV3YXJlIG9mIHRoZSBMZW9wYXJkLgo=".freeze
28+
29+
context "good key lines" do
30+
it "basic" do
31+
line = "ssh-rsa %s user@example.net" % DUMMY_KEY_CONTENTS
32+
(options, type, key, comment) = DummyKeyParser.new.split_key_line(line)
33+
expect(options).to eq(nil)
34+
expect(type).to eq("ssh-rsa")
35+
expect(key).to eq(DUMMY_KEY_CONTENTS)
36+
expect(comment).to eq("user@example.net")
37+
end
38+
39+
it "with rogue spaces and comment" do
40+
line = " ssh-rsa %s user@example.net " % DUMMY_KEY_CONTENTS
41+
(options, type, key, comment) = DummyKeyParser.new.split_key_line(line)
42+
expect(options).to eq(nil)
43+
expect(type).to eq("ssh-rsa")
44+
expect(key).to eq(DUMMY_KEY_CONTENTS)
45+
expect(comment).to eq("user@example.net")
46+
end
47+
48+
it "with rogue spaces and no comment" do
49+
line = " ssh-rsa %s " % DUMMY_KEY_CONTENTS
50+
(options, type, key, comment) = DummyKeyParser.new.split_key_line(line)
51+
expect(options).to eq(nil)
52+
expect(type).to eq("ssh-rsa")
53+
expect(key).to eq(DUMMY_KEY_CONTENTS)
54+
expect(comment).to eq(nil)
55+
end
56+
57+
it "with basic options" do
58+
line = 'from="*.sales.example.net,!pc.sales.example.net" ssh-rsa %s john@example.net' % DUMMY_KEY_CONTENTS
59+
(options, type, key, comment) = DummyKeyParser.new.split_key_line(line)
60+
expect(options).to eq('from="*.sales.example.net,!pc.sales.example.net"')
61+
expect(type).to eq("ssh-rsa")
62+
expect(key).to eq(DUMMY_KEY_CONTENTS)
63+
expect(comment).to eq("john@example.net")
64+
end
65+
66+
it "with mixed options with spaces" do
67+
line = 'command="dump /home",no-pty,no-port-forwarding ssh-rsa %s example.net' % DUMMY_KEY_CONTENTS
68+
(options, type, key, comment) = DummyKeyParser.new.split_key_line(line)
69+
expect(options).to eq('command="dump /home",no-pty,no-port-forwarding')
70+
expect(type).to eq("ssh-rsa")
71+
expect(key).to eq(DUMMY_KEY_CONTENTS)
72+
expect(comment).to eq("example.net")
73+
end
74+
75+
it "with options and no comment" do
76+
line = 'permitopen="192.0.2.1:80",permitopen="192.0.2.2:25" ssh-rsa %s' % DUMMY_KEY_CONTENTS
77+
(options, type, key, comment) = DummyKeyParser.new.split_key_line(line)
78+
expect(options).to eq('permitopen="192.0.2.1:80",permitopen="192.0.2.2:25"')
79+
expect(type).to eq("ssh-rsa")
80+
expect(key).to eq(DUMMY_KEY_CONTENTS)
81+
expect(comment).to eq(nil)
82+
end
83+
84+
it "with options with spaces and comment" do
85+
line = 'tunnel="0",command="sh /etc/netstart tun0" ssh-rsa %s jane@example.net' % DUMMY_KEY_CONTENTS
86+
(options, type, key, comment) = DummyKeyParser.new.split_key_line(line)
87+
expect(options).to eq('tunnel="0",command="sh /etc/netstart tun0"')
88+
expect(type).to eq("ssh-rsa")
89+
expect(key).to eq(DUMMY_KEY_CONTENTS)
90+
expect(comment).to eq("jane@example.net")
91+
end
92+
93+
it "with mixed options and comment" do
94+
line = 'restrict,pty,command="nethack" ssh-rsa %s user2@example.net' % DUMMY_KEY_CONTENTS
95+
(options, type, key, comment) = DummyKeyParser.new.split_key_line(line)
96+
expect(options).to eq('restrict,pty,command="nethack"')
97+
expect(type).to eq("ssh-rsa")
98+
expect(key).to eq(DUMMY_KEY_CONTENTS)
99+
expect(comment).to eq("user2@example.net")
100+
end
101+
102+
it "with non-ssh-rsa type" do
103+
line = "no-touch-required sk-ecdsa-sha2-nistp256@openssh.com %s user3@example.net" % DUMMY_KEY_CONTENTS
104+
(options, type, key, comment) = DummyKeyParser.new.split_key_line(line)
105+
expect(options).to eq("no-touch-required")
106+
expect(type).to eq("sk-ecdsa-sha2-nistp256@openssh.com")
107+
expect(key).to eq(DUMMY_KEY_CONTENTS)
108+
expect(comment).to eq("user3@example.net")
109+
end
110+
111+
it "with quotes and spaces in options" do
112+
line = 'environment="MYVAR=I have a quote\" in my middle" ssh-rsa %s user4@example.net' % DUMMY_KEY_CONTENTS
113+
(options, type, key, comment) = DummyKeyParser.new.split_key_line(line)
114+
expect(options).to eq('environment="MYVAR=I have a quote\" in my middle"')
115+
expect(type).to eq("ssh-rsa")
116+
expect(key).to eq(DUMMY_KEY_CONTENTS)
117+
expect(comment).to eq("user4@example.net")
118+
end
119+
120+
it "with spaces in comment" do
121+
line = "ssh-rsa %s comment with spaces user@example.net " % DUMMY_KEY_CONTENTS
122+
(options, type, key, comment) = DummyKeyParser.new.split_key_line(line)
123+
expect(options).to eq(nil)
124+
expect(type).to eq("ssh-rsa")
125+
expect(key).to eq(DUMMY_KEY_CONTENTS)
126+
expect(comment).to eq("comment with spaces user@example.net")
127+
end
128+
end
129+
130+
context "bad key lines" do
131+
it "empty" do
132+
line = ""
133+
(options, type, key, comment) = DummyKeyParser.new.split_key_line(line)
134+
expect(options).to eq(nil)
135+
expect(type).to eq(nil)
136+
expect(key).to eq(nil)
137+
expect(comment).to eq(nil)
138+
end
139+
140+
it "with one field" do
141+
line = "invalid"
142+
(options, type, key, comment) = DummyKeyParser.new.split_key_line(line)
143+
expect(options).to eq("invalid")
144+
expect(type).to eq(nil)
145+
expect(key).to eq(nil)
146+
expect(comment).to eq(nil)
147+
end
148+
149+
it "with two fields" do
150+
line = "invalid two"
151+
(options, type, key, comment) = DummyKeyParser.new.split_key_line(line)
152+
expect(options).to eq("invalid")
153+
expect(type).to eq("two")
154+
expect(key).to eq(nil)
155+
expect(comment).to eq(nil)
156+
end
157+
158+
it "with three fields" do
159+
line = "invalid line three"
160+
(options, type, key, comment) = DummyKeyParser.new.split_key_line(line)
161+
expect(options).to eq("invalid")
162+
expect(type).to eq("line")
163+
expect(key).to eq("three")
164+
expect(comment).to eq(nil)
165+
end
166+
167+
it "with four fields" do
168+
line = "invalid line four field"
169+
(options, type, key, comment) = DummyKeyParser.new.split_key_line(line)
170+
expect(options).to eq("invalid")
171+
expect(type).to eq("line")
172+
expect(key).to eq("four")
173+
expect(comment).to eq("field")
174+
end
175+
176+
it "with more fields " do
177+
line = "invalid line which is not a key at all"
178+
(options, type, key, comment) = DummyKeyParser.new.split_key_line(line)
179+
expect(options).to eq("invalid")
180+
expect(type).to eq("line")
181+
expect(key).to eq("which")
182+
expect(comment).to eq("is not a key at all")
183+
end
184+
end
185+
end

0 commit comments

Comments
 (0)