known_hosts: Add full key validation

[GideonBear]

SUMMARY

When the key parameter to known_hosts contains a newline (\n or \r to be thorough), fail the module.
Keys may not contain newlines, and only a single key may be supplied. A newline in the key parameter may indicate the user is attempting to supply multiple keys. Example of faulty usage:

- name: Get host key
  ansible.builtin.command: ssh-keyscan -q 172.17.0.1
  changed_when: false
  register: host_key

- name: Add host key to known_hosts
  ansible.builtin.known_hosts:
    path: "/whatever/known_hosts"
    name: 172.17.0.1
    key: "{{ host_key.stdout }}"  # Note that the output of ssh-keyscan contains multiple keys by default
    state: present

This previously silently worked in some cases, and had silently incorrect behavior in others. This is due to any trailing parts of the key being ignored, and being split on any whitespace (including newlines).
Now, this fails with Argument 'key' contains newlines. This module only accepts a single key..

I added an integration test for this.

Concerns/questions:

• Am I correct in assuming a key may never contain a newline?
• This is technically a breaking change, but only breaks already silently broken tasks.
• Right now, the check is implemented in normalize_known_hosts_key, which also handles normalizing for the output of ssh-keygen. Since the key that's passed into there (l) is .split('\n') it can never contain a newline, and this is thus safe to do; but it might have benefit to do separate this anyway.
• Improvements to the error message are welcome!

Closes #85637

ISSUE TYPE

• Bugfix Pull Request

[bcoca]

just rstrip \r from the key after split, there is no case in which \n should make it to this point as we split on it before calling the function. No need for errors, \r is invalid in the middle of the key but it is probably part of the line break if it was saved from a Windows machine, that case we can optimize for, any other case should fail on key/file validation

[GideonBear]

Where do we split on key before calling normalize_known_hosts_key? Do you mean line 259? That's only on the output of ssh-keygen, not the module argument key.

[bcoca]

ah, mixed those, still, this is not something we should validate this way, we either do full 'correct key format' validation or leave as is

[GideonBear]

this is not something we should validate this way, we either do full 'correct key format' validation or leave as is

Sure, I'm up for implementing full validation. But out of curiosity, why? What's wrong with a simple check for a footgun (specifically passing multiple keys) if you know it's never valid?

[bcoca]

it is piecemeal, we would keep adding each case as it appears and make a tangle of the code, while a full format validation should cover all cases off the bat in a much cleaner way

[GideonBear]

Fair!
I'll mark this as draft and start working on full validation. I will also take a look at other issues to see if they could be solved at once.

[s-hertel]

Why not do this in the function called sanity_check instead? It's already responsible for validating some aspects of the key, and then we wouldn't need to thread the module through this function.

+    if len(key.splitlines()) > 1:
+        module.fail_json(msg="Argument 'key' contains newlines. This module only accepts a single key.")

[GideonBear]

You're right, I missed that. But I assume that doesn't invalidate @bcoca's criticism?