Use dynamic identity file names by default

[arixmkii]

Fixes #16710 #17521

Dynamically generated names supports up to four hex digits at the end to detect available file names. The range scan starts from VM SSH port to reduce collision probability. It is still possible to override dynamically generated file names using command line option during machine init.

machine init --identity <filename> supports relative and absolute paths, but for WSL machine there still be a check that files should reside in the default folder due to the way how they are generated inside the VM.

identityFlagName default value is not extracted to common lib, because it could have only one reasonable value, which is empty string (unset).

Does this PR introduce a user-facing change?

Podman machine init supports --identity parameter to change default ssh key file names (default key file names are now dynamically chosen instead of being equal to VM name)

[NO NEW TESTS NEEDED]

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: arixmkii
Once this PR has been reviewed and has the lgtm label, please assign flouthoc for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[arixmkii]

@n1hility please check, when you have time, because there are some changes around WSL machine.

[arixmkii]

Longer explanation of how this idea was formed could be found here #16710 (comment)

[baude]

i would imagine this change is also applicable to the hyperv and applehv implementations?

[arixmkii]

Hyper-V was covered in original changeset. I added applevh changes. Haven't tested either of them locally (for applehv it seems there are some patches needed to enable it), but the implementations of the ssh keys part looked exactly like ones from QEMU implementation (which is logical as all these flavors are using CoreOS).

[baude]

applehv code should all be in main now .. it is not complete but booting it works. See https://fedorapeople.org/groups/podman/testing/applehv/

[arixmkii]

applehv code should all be in main now

I noticed it. The parts, which were available before my change should be enough to implement the functionality for applehv (the change I added), but I can rebase to latest main if you insist. Will test applehv locally later this week.

[rhatdan]

@mheon @vrothberg @baude @mtrmac @giuseppe PTAL

[n1hility]

@arixmkii sorry for the delay in getting to this I recently got back from a conf and still playing catchup. I did see this and at first thought its fine; at least technically, the change looks good.

However, after thinking more about this, I realized we have a larger fundamental problem. Keys aren't the only namespace collision, we also get them on connection names as well. A random suffix is ok in the case of a keyfile which isn't normally referred to manually (but can be). However, if the same strategy were used in the case of a connection name, it would lead to a fairly poor UX. So a question I think we should ask ourselves is can we find a common mostly similar strategy to naming organization (connections, machine files, keys etc)?

In the case of both, the guaranteed property, which you note in your comment you link from this issue, is that a name is required to be unique within the namespace of the backend provider. This suggests that connection configurations and ssh keys can be addressed by establishing a scope matching the backend. Your original approach seems to have gone that route with using a backend prefix, and if I understand correctly, you abandoned this out of concern of conflict with existing names, including the ability for a manually configured name. However for connections, we will most likely end up with a prefix / suffix since they are currently modeled as a map, and so must be globally unique. I imagine we would probably introduce some sort of inferred lookup, so for example, you can use -c foo, and baring an entry for foo, podman would also look for a foo-backend using the current configured backend. Your original approach would mirror this model, so seems like a good way to go.

However, that does not address the collision concerns., One simple solution to reduce them could be to introduce a podman-machine subdirectory in .ssh. Although even without that step these collisions still be very uncommon, since how often does someone want to name a key "podman-machine-default-qemu"?

wdyt?

[openshift-merge-robot]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[arixmkii]

Keys aren't the only namespace collision

Unfortunately, this is very true.

Your original approach would mirror this model, so seems like a good way to go.

Yes, this was a lot simpler and cleaner implementation.

One simple solution to reduce them could be to introduce a podman-machine subdirectory in .ssh.

What about having subdirectories like podman-machine-<provider>, where providers will be responsible for the layout further, but the directories themselves would be just like workspaces for every of the machine implementations? Again, this might evolve and change going further if the need arises for providers doing some sort of cooperation.

As for connections and their potential collisions. Just simple naming convention having prefix/suffix of the provider would make things more safe.

To mitigate this fully, one would need to develop something like isolated storages for user defined connections and machine API defined connections (per provider), where later the connection list would be aggregation of user defined and active provider and giving option to the user to narrow the list to machine ones only, otherwise only user defined (global scope as it is now) would take precedence in case of the name collision. But this would be not a simple, not a compact and not an easy to grasp to maintain peace of code, which would only improve UX in the very specific area.

[arixmkii]

A separate question to discuss. What about the option to override key file name independently of the machine name during init command (the -identity <filename> part of this PR). The same effect now can be achieved by creating machine as is, then renaming the identity files and editing machine config and connections manually. If this functionality is cleared to be ok, I can extract it to a separate PR and proceed.

[github-actions]

A friendly reminder that this PR had no activity for 30 days.

[arixmkii]

@n1hility Do you know if anything is happening on this topic?

To mitigate this fully, one would need to develop something like isolated storages for user defined connections and machine API defined connections (per provider)

I can re-iterate my work on this, but would like to get some more hints how Podman team sees it.

[rhatdan]

@baude @n1hility @vrothberg Thoughts.

[vrothberg]

@baude what should happen with this PR?

[baude]

@arixmkii not ignoring you ... but i am contemplating proposed podman 5 changes and this PR ... and in some ways they have conflicting approaches ... this is bug week for us, which is why im looking at it again.

[arixmkii]

@baude if this is not relevant and will be addressed elsewhere in a more appropriate way you can close this PR.

[arixmkii]

Closing this, as it will require a lot of changes (potentially including design changes).