cc_keys_to_console: add option to disable key emission

[mwhudson]

cc_keys_to_console: add option to disable key emission

Specifically:

ssh:                                                                            
  emit_keys_to_console: false

We also port the cc_keys_to_console cloud tests to the new integration
testing framework, and add a test for this new option.

LP: #1915460

Checklist:

• My code follows the process laid out in the documentation
• I have updated or added any unit tests accordingly
• I have updated or added any documentation accordingly

[mwhudson]

the integration test is a total guess, not sure how to run that myself

[smoser]

Can we reverse the name and default. Instead of no_keys_to_console defaulting to false, change to 'keys_to_console'. Also, as a general rule, were trying not to busy polute the top level namespace anymore. As example, 'apt' now is a top level key with many options under it, where before there were several apt related options on the top level.

…

On Thu, Feb 11, 2021, 5:24 PM Michael Hudson-Doyle ***@***.***> wrote: the integration test is a total guess, not sure how to run that myself — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#811 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAETMJMRXBXHVKZ433G6LTS6RKKVANCNFSM4XPUYHUQ> .

[mwhudson]

That all makes sense. ssh stuff seems to be particularly bad for having a random grab bag of things at top level (probably because it is all so old and fundamental) so I don't quite know where to put it. A new ssh: top level key?

[dermotbradley]

@mwhudson Doesn't the following user-data achieve what you want?

ssh_key_console_blacklist: [ 'ssh-dsa', 'ssh-rsa', 'ssh-ecdsa', 'ssh-ed25519' ]

I use this (only for ED25519 host keys) along with a similar "ssh_fp_console_blacklist" line to reduce console output.

The only issue with the above is that the BEGIN/END header and footer lines (but no keys) are still printed. I've been meaning to raise a PR to fix this as well as another minor issue (the string "ec2" is hardcoded in logger_opts regardless of DataSource) in tools/write-ssh-key-fingerprints.

[mwhudson]

Yeah I want no output at all. If write-ssh-key-fingerprints didn't output the header and footer unless it output a key, that would work for my purposes.

[dermotbradley]

If write-ssh-key-fingerprints didn't output the header and footer unless it output a key, that would work for my purposes.

I've modified write-ssh-key-fingerprints to achieve this - I'm testing it currently locally and intend to submit a PR for it some time in the next couple of hours.

[dermotbradley]

Yeah I want no output at all. If write-ssh-key-fingerprints didn't output the header and footer unless it output a key, that would work for my purposes.

#817 should achieve this.

[OddBloke]

Yeah I want no output at all. If write-ssh-key-fingerprints didn't output the header and footer unless it output a key, that would work for my purposes.

#817 should achieve this.

I think these two PRs are complementary: this one allows the user to control whether or not this info is emitted at all, whereas #817 cleans up an inconsistency in what we output.

My specific concern about using #817 for this usecase is that subiquity and other consumers may have to play whack-a-mole with SSH host key types if/when that set expands. (This also has the minor advantage of executing less code over the #817 proposal, but given the simplicity of the code in question, I doubt that's significant; 0.02s in a container I have lying around, not all of which would be removed).

[OddBloke]

That all makes sense. ssh stuff seems to be particularly bad for having a random grab bag of things at top level (probably because it is all so old and fundamental) so I don't quite know where to put it. A new ssh: top level key?

I think this looks good:

ssh:
  emit_keys_to_console: true

@smoser @mwhudson Thoughts?

[dermotbradley]

I think this looks good:

ssh:
  emit_keys_to_console: true

ssh_fp_console_blacklist: [ all ]
ssh_key_console_blacklist: [ all ]

perhaps?

[OddBloke]

I think this looks good:

ssh:
  emit_keys_to_console: true

ssh_fp_console_blacklist: [ all ]
ssh_key_console_blacklist: [ all ]

perhaps?

I'm not super-keen on having a magic string (albeit a fairly straightforward one); YAML has a boolean type, which more closely matches what we want here. We'd also need to consider special handling so that people passing ssh_fp_console_blacklist: all don't end up disabling "a", "l" and "l" (and therefore, of course, disabling nothing). A bool is preferable from a schema validation perspective, too: we can catch fasle but can't really catch "alll" because, for all our schema validation knows, "alll" is a valid value.

[mwhudson]

On Tue, 23 Feb 2021, 06:25 Daniel Watkins, ***@***.***> wrote: I think this looks good: ssh: emit_keys_to_console: true @smoser <https://github.com/smoser> @mwhudson <https://github.com/mwhudson> Thoughts?

WFM. Do you want me to make this change or can you do it?

…

[OddBloke]

WFM. Do you want me to make this change or can you do it?

40a5655 ^_^

[dermotbradley]

WFM. Do you want me to make this change or can you do it?

40a5655 ^_^

Plus remove the reference to "no_ssh_fingerprints" from doc/examples/cloud-config-ssh-keys.txt?

[OddBloke]

On Mon, Feb 22, 2021 at 10:23:28AM -0800, dermotbradley wrote: > > WFM. Do you want me to make this change or can you do it? > > [40a5655](40a5655) ^_^ Plus remove the reference to "no_ssh_fingerprints" from doc/examples/cloud-config-ssh-keys.txt?

no_ssh_fingerprints is already supported by cloud-init, so this is just including it in this documentation: https://github.com/canonical/cloud-init/blob/master/cloudinit/config/cc_ssh_authkey_fingerprints.py#L24

[TheRealFalcon]

Looks good to me as long as we remove the doc and cloud test additions.

[TheRealFalcon]

Nit: Might be useful to have a reference to LP: #1915460 or gh-811 somewhere in the integration test, but +1 regardless.

[dermotbradley]

no_ssh_fingerprints is already supported by cloud-init, so this is just including it in this documentation: https://github.com/canonical/cloud-init/blob/master/cloudinit/config/cc_ssh_authkey_fingerprints.py#L24

My mistake - I was getting it confused with a new config parameter to control the display of host key fingerprints, rather than that to control user key fingerprints.

[OddBloke]

no_ssh_fingerprints is already supported by cloud-init, so this is just including it in this documentation: https://github.com/canonical/cloud-init/blob/master/cloudinit/config/cc_ssh_authkey_fingerprints.py#L24

My mistake - I was getting it confused with a new config parameter to control the display of host key fingerprints, rather than that to control user key fingerprints.

Yep, took me a double-take before I figured it out too. :)