-
Notifications
You must be signed in to change notification settings - Fork 26
Refs #38478 - Introduce SSH CA certificate support #126
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -21,6 +21,15 @@ def public_key_file | |
| File.expand_path("#{private_key_file}.pub") | ||
| end | ||
|
|
||
| def cert_file | ||
| File.expand_path("#{private_key_file}-cert.pub") | ||
| end | ||
|
|
||
| def ca_public_key_file | ||
| path = Plugin.settings.ssh_user_ca_public_key_file | ||
| File.expand_path(path) if present?(path) | ||
| end | ||
|
|
||
| def validate_mode! | ||
| Plugin.settings.mode = Plugin.settings.mode.to_sym | ||
|
|
||
|
|
@@ -50,14 +59,23 @@ def validate_ssh_settings! | |
| end | ||
|
|
||
| unless File.exist?(private_key_file) | ||
| raise "SSH public key file #{private_key_file} doesn't exist.\n"\ | ||
| raise "SSH private key file #{private_key_file} doesn't exist.\n"\ | ||
| "You can generate one with `ssh-keygen -t rsa -b 4096 -f #{private_key_file} -N ''`" | ||
| end | ||
|
|
||
| unless File.exist?(public_key_file) | ||
| raise "SSH public key file #{public_key_file} doesn't exist" | ||
| end | ||
|
|
||
| if present?(Plugin.settings.ssh_user_ca_public_key_file) | ||
| { ca_public_key_file: 'CA public key', cert_file: 'certificate' }.each do |file, label| | ||
| file_path = public_send(file) | ||
| unless file_path && File.exist?(file_path) | ||
| raise "SSH #{label} file '#{file_path}' doesn't exist" | ||
| end | ||
| end | ||
|
Comment on lines
+71
to
+76
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the recent changes to how ca_public_key_file setting is handled (possibly returning nil), you can run into errors like this if you only set it is not technically wrong, but it looks odd
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for catching it, I decided to handle client_cert_file differently. |
||
| end | ||
|
|
||
| validate_ssh_log_level! | ||
| end | ||
|
|
||
|
|
@@ -100,6 +118,12 @@ def job_storage | |
| def with_mqtt? | ||
| Proxy::RemoteExecution::Ssh::Plugin.settings.mode == :'pull-mqtt' | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def present?(value) | ||
| value && !value.empty? | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change | |||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -60,6 +60,8 @@ def initialize(options, logger:) | ||||||||||||||||||||||||||||
| @host_public_key = options.fetch(:host_public_key, nil) | |||||||||||||||||||||||||||||
| @verify_host = options.fetch(:verify_host, nil) | |||||||||||||||||||||||||||||
| @client_private_key_file = settings.ssh_identity_key_file | |||||||||||||||||||||||||||||
| @client_ca_known_hosts_file = settings.ssh_ca_known_hosts_file | |||||||||||||||||||||||||||||
| @client_cert_file = Proxy::RemoteExecution::Ssh.cert_file if File.exist?(Proxy::RemoteExecution::Ssh.cert_file) | |||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||
| @local_working_dir = options.fetch(:local_working_dir, settings.local_working_dir) | |||||||||||||||||||||||||||||
| @socket_working_dir = options.fetch(:socket_working_dir, settings.socket_working_dir) | |||||||||||||||||||||||||||||
|
|
@@ -154,9 +156,14 @@ def establish_ssh_options | ||||||||||||||||||||||||||||
| ssh_options << "-o User=#{@ssh_user}" | |||||||||||||||||||||||||||||
| ssh_options << "-o Port=#{@ssh_port}" if @ssh_port | |||||||||||||||||||||||||||||
| ssh_options << "-o IdentityFile=#{@client_private_key_file}" if @client_private_key_file | |||||||||||||||||||||||||||||
| ssh_options << "-o CertificateFile=#{@client_cert_file}" if @client_cert_file | |||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
won't this always evaluate to true because
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is true, and also it definitely should not work like that. I Fixed the cert_file method to only return path if file is present. |
|||||||||||||||||||||||||||||
| ssh_options << "-o IdentitiesOnly=yes" | |||||||||||||||||||||||||||||
| ssh_options << "-o StrictHostKeyChecking=accept-new" | |||||||||||||||||||||||||||||
| ssh_options << "-o UserKnownHostsFile=#{prepare_known_hosts}" if @host_public_key | |||||||||||||||||||||||||||||
| ssh_options << "-o StrictHostKeyChecking=#{@client_ca_known_hosts_file ? 'yes' : 'accept-new'}" | |||||||||||||||||||||||||||||
| if @host_public_key | |||||||||||||||||||||||||||||
| ssh_options << "-o UserKnownHostsFile=#{prepare_known_hosts}" | |||||||||||||||||||||||||||||
| elsif @client_ca_known_hosts_file | |||||||||||||||||||||||||||||
| ssh_options << "-o UserKnownHostsFile=#{@client_ca_known_hosts_file}" | |||||||||||||||||||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we have a known_hosts file with cert authorities listed, should we change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO yes. Otherwise, we get "if you trust this cert, connect and if you don't, connect anyway".
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For host keys, those should be all the combinations that can happen:
Could we agree that this table describes the expected behaviour? One could argue that proxy expecting a cert and host not providing any (row 4) should fail rather than falling back to traditional pubkey auth.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I too would say that row 4 could fail, but there is currently option to only have one host CA per smart proxy so it would be setting behavior for all hosts that use that proxy for REX. That is probably the only point that I see why it should fallback to plain SSH key instead of fail.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Until we decide on one or the other, I will treat the expected behavior of row 4 to be failure. I updated the StrictHostKeyChecking param accordingly. I will also make the same edits to ansible.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So row 4 should indeed result in a failure, added two more rows to the table describing one other case that we previously missed.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can confirm that rows 7 and 8 also behave as intended, so the behavior of the whole feature should work as expected. |
|||||||||||||||||||||||||||||
| end | |||||||||||||||||||||||||||||
| ssh_options << "-o LogLevel=#{ssh_log_level(true)}" | |||||||||||||||||||||||||||||
| ssh_options << "-o ControlMaster=auto" | |||||||||||||||||||||||||||||
| ssh_options << "-o ControlPath=#{socket_file}" | |||||||||||||||||||||||||||||
|
|
|||||||||||||||||||||||||||||
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.
Should we require this strict naming convention for cert file
similarly as we do for public key file, or should we make this an editable
value in settings, with this format being the possible default?