[EmbeddedAnsible] Ensure newline for :ssh_key_data #20771

NickLaMuro · 2020-11-02T17:54:00Z

SSH formats like OPENSSH require that a newline exist on the last line, otherwise it is considered an invalid format.

This adds a before_save callback to the model to ensure that it adds a newline to the key in case it was stripped off by the UI or via other means.

Links

kbrock

this looks great.

kbrock · 2020-11-02T18:01:31Z

app/models/manageiq/providers/embedded_ansible/automation_manager/scm_credential.rb

@@ -54,6 +54,8 @@ class ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ScmCredential < M
  alias ssh_key_data   auth_key
  alias ssh_key_unlock auth_key_password

+  before_save :ensure_newline_for_ssh_key


would before_validation work as well?

feel like data manipulation should happen before validation rather than before save

Seems like it still passes the tests, so either way works and I have no strong preference myself.

@Fryguy since I expect that you will give a final say anyway, do you want to weigh in on which you prefer before I end up rebasing this multiple times?

Hi @NickLaMuro, Would preserving the private key as entered be an option here? The reason I ask is that an RSA private key without the newline works fine in Ansible. The problem we encountered was specific to ssh keys generated on 5.11 (rhel 8). There wasn't a problem when we used ssh keys generated on 5.11 (rhel 8) with the -m PEM option which creates the RSA private key as opposed tp the OPENSSH private key.

I think before_validation is the better choice

@tinaafitz I tested both types of private keys on a generic git clone with the following done to them:

$ echo "" >> ~/.ssh/id_rsa $ echo "" >> ~/.ssh/id_rsa $ echo "" >> ~/.ssh/id_rsa

So any number of newlines applied was no problem. However, lack of a \n seemed to be problematic, so I am airing on the side of "add a new line if it doesn't exist" universally instead of trying to parse specific keys.

If you find a key where this is not the case, we can address, but it seems like the following line:

-----END OPENSSH PRIVATE KEY-----

And

-----END RSA PRIVATE KEY-----

Are all that need to be in place, and subsequent lines are ignored.

Thanks @NickLaMuro. I feel better about it. :-)

NickLaMuro · 2020-11-02T18:09:03Z

cc @tinaafitz @djberg96

djberg96 · 2020-11-02T18:25:51Z

app/models/manageiq/providers/embedded_ansible/automation_manager/scm_credential.rb

@@ -66,4 +68,8 @@ def self.params_to_attributes(params)

    attrs
  end
+
+  def ensure_newline_for_ssh_key
+    self.auth_key = "#{auth_key}\n" unless auth_key.to_s[-1] == "\n"


Is the auth_key encoding already guaranteed? Just want to make sure to_s[-1] doesn't break with an encoding compatibility error.

Should we only do this for when the key has the OPENSSH guard kind?

Oh I just read the gitter channel, and yeah, if you think that all keys could benefit from a trailing newline, then I'm good with this.

Also I can't tell from the diff, but please make this a private method.

Oh I just noticed a problem here...if auth_key is nil (for a non-ssh based authentication), then will this change the nil to a blank + \n, which is probably not wanted?

Also I can't tell from the diff, but please make this a private method.

@Fryguy Missed this oringally, but will do.

Oh I just noticed a problem here...if auth_key is nil

@Fryguy fixed and added specs

Is the auth_key encoding already guaranteed? Just want to make sure to_s[-1] doesn't break with an encoding compatibility error.

@djberg96 this inherits from credential.rb, which inherits from app/models/authentication.rb, and that model shows this attributes are covered:

manageiq/app/models/authentication.rb

Lines 20 to 24 in cf52121

encrypt_column :auth_key

encrypt_column :auth_key_password

encrypt_column :become_password

encrypt_column :password

encrypt_column :service_account

Fryguy

Couple comments

SSH formats like `OPENSSH` require that a newline exist on the last line, otherwise it is considered an invalid format. This adds a `before_validation` callback to the model to ensure that it adds a newline to the key (if a key exists) in case it was stripped off by the UI or via other means.

NickLaMuro · 2020-11-02T19:42:36Z

@Fryguy just pushed all of the requested fixes (after your approval). Hopefully those are good.

miq-bot · 2020-11-02T19:45:18Z

Checked commit NickLaMuro@89886e3 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🏆

kbrock · 2020-11-02T20:35:05Z

In the general case, rails trimming the last "\n" makes sense to me.
It also makes sense that we don't want that behavior.

this solution looks like a great and would be a good candidate for backporting

…nsure_newline [EmbeddedAnsible] Ensure newline for :ssh_key_data (cherry picked from commit 1895a65)

simaishi · 2020-11-09T12:55:38Z

Kasparov backport details:

$ git log -1
commit a24481e6a1b2d444d6e0fcdf42f3b7b4d74ae599
Author: Keenan Brock <keenan@thebrocks.net>
Date:   Mon Nov 2 15:33:43 2020 -0500

    Merge pull request #20771 from NickLaMuro/embedded_ansible_scm_cred_ensure_newline

    [EmbeddedAnsible] Ensure newline for :ssh_key_data

    (cherry picked from commit 1895a653cbb81f0c472fd816f899dbc6f95265e5)

…nsure_newline [EmbeddedAnsible] Ensure newline for :ssh_key_data (cherry picked from commit 1895a65)

simaishi · 2020-11-09T13:21:38Z

Jansa backport details:

$ git log -1
commit 4a50d87f7f609a7c69b8b0bd63998985df3308cc
Author: Keenan Brock <keenan@thebrocks.net>
Date:   Mon Nov 2 15:33:43 2020 -0500

    Merge pull request #20771 from NickLaMuro/embedded_ansible_scm_cred_ensure_newline

    [EmbeddedAnsible] Ensure newline for :ssh_key_data

    (cherry picked from commit 1895a653cbb81f0c472fd816f899dbc6f95265e5)

simaishi · 2020-11-09T20:22:25Z

@NickLaMuro can this be ivanchuk/yes?

NickLaMuro · 2020-11-09T20:44:46Z

@simaishi Probably should be okay to do so. Might have conflicts though.

@miq-bot add_label ivanchuk/yes

…nsure_newline [EmbeddedAnsible] Ensure newline for :ssh_key_data (cherry picked from commit 1895a65) https://bugzilla.redhat.com/show_bug.cgi?id=1893014

simaishi · 2020-11-09T20:57:00Z

No conflicts 😄

Ivanchuk backport details:

$ git log -1
commit 1f6a3bacf60fa9b7d756678aad3355d532305f23
Author: Keenan Brock <keenan@thebrocks.net>
Date:   Mon Nov 2 15:33:43 2020 -0500

    Merge pull request #20771 from NickLaMuro/embedded_ansible_scm_cred_ensure_newline

    [EmbeddedAnsible] Ensure newline for :ssh_key_data

    (cherry picked from commit 1895a653cbb81f0c472fd816f899dbc6f95265e5)

    https://bugzilla.redhat.com/show_bug.cgi?id=1893014

NickLaMuro requested review from agrare, Fryguy and gtanzillo as code owners November 2, 2020 17:54

miq-bot added the core/embedded ansible label Nov 2, 2020

kbrock reviewed Nov 2, 2020

View reviewed changes

djberg96 reviewed Nov 2, 2020

View reviewed changes

Fryguy reviewed Nov 2, 2020

View reviewed changes

NickLaMuro force-pushed the embedded_ansible_scm_cred_ensure_newline branch from 4ee4dc9 to cf52121 Compare November 2, 2020 19:34

Fryguy approved these changes Nov 2, 2020

View reviewed changes

Fryguy self-assigned this Nov 2, 2020

Fryguy added bug jansa/yes? labels Nov 2, 2020

NickLaMuro force-pushed the embedded_ansible_scm_cred_ensure_newline branch from cf52121 to 89886e3 Compare November 2, 2020 19:41

kbrock merged commit 1895a65 into ManageIQ:master Nov 2, 2020

gtanzillo added jansa/yes and removed jansa/yes? labels Nov 5, 2020

simaishi pushed a commit that referenced this pull request Nov 9, 2020

Merge pull request #20771 from NickLaMuro/embedded_ansible_scm_cred_e…

a24481e

…nsure_newline [EmbeddedAnsible] Ensure newline for :ssh_key_data (cherry picked from commit 1895a65)

simaishi added kasparov/backported and removed kasparov/yes labels Nov 9, 2020

simaishi pushed a commit that referenced this pull request Nov 9, 2020

Merge pull request #20771 from NickLaMuro/embedded_ansible_scm_cred_e…

4a50d87

…nsure_newline [EmbeddedAnsible] Ensure newline for :ssh_key_data (cherry picked from commit 1895a65)

simaishi added jansa/backported and removed jansa/yes labels Nov 9, 2020

miq-bot added the ivanchuk/yes label Nov 9, 2020

simaishi added ivanchuk/backported and removed ivanchuk/yes labels Nov 9, 2020

NickLaMuro mentioned this pull request Nov 30, 2020

[EmbeddedAnsible] Ensure newline for auth_key in MachineCredential #20845

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EmbeddedAnsible] Ensure newline for :ssh_key_data #20771

[EmbeddedAnsible] Ensure newline for :ssh_key_data #20771

NickLaMuro commented Nov 2, 2020

kbrock left a comment

kbrock Nov 2, 2020

NickLaMuro Nov 2, 2020 •

edited

Loading

tinaafitz Nov 2, 2020

Fryguy Nov 2, 2020

NickLaMuro Nov 2, 2020

tinaafitz Nov 2, 2020

NickLaMuro commented Nov 2, 2020

djberg96 Nov 2, 2020

Fryguy Nov 2, 2020

Fryguy Nov 2, 2020

Fryguy Nov 2, 2020

Fryguy Nov 2, 2020 •

edited

Loading

NickLaMuro Nov 2, 2020

Fryguy left a comment

NickLaMuro commented Nov 2, 2020

miq-bot commented Nov 2, 2020

kbrock commented Nov 2, 2020

simaishi commented Nov 9, 2020

simaishi commented Nov 9, 2020

simaishi commented Nov 9, 2020

NickLaMuro commented Nov 9, 2020

simaishi commented Nov 9, 2020

	encrypt_column :auth_key
	encrypt_column :auth_key_password
	encrypt_column :become_password
	encrypt_column :password
	encrypt_column :service_account

[EmbeddedAnsible] Ensure newline for :ssh_key_data #20771

[EmbeddedAnsible] Ensure newline for :ssh_key_data #20771

Conversation

NickLaMuro commented Nov 2, 2020

Links

kbrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro Nov 2, 2020 • edited Loading

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

NickLaMuro commented Nov 2, 2020

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

Fryguy Nov 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy left a comment

Choose a reason for hiding this comment

NickLaMuro commented Nov 2, 2020

miq-bot commented Nov 2, 2020

kbrock commented Nov 2, 2020

simaishi commented Nov 9, 2020

simaishi commented Nov 9, 2020

simaishi commented Nov 9, 2020

NickLaMuro commented Nov 9, 2020

simaishi commented Nov 9, 2020

NickLaMuro Nov 2, 2020 •

edited

Loading

Fryguy Nov 2, 2020 •

edited

Loading