Cleanup ssh auth keys type

grep supports multiple patterns, it doesn't have greps wrapper around grep for more than one pattern. __ssh_authorized_keys supports multiple keys in authorized keys file (since this entity contains multiple entries), and it does one thing: it manages ssh authorized_keys file.

As for the unix way of doing things, I don't see violations, I see it like `grep`. `grep` prints lines matching pattern. New `__ssh_authorized_keys` manages ssh authorized keys file. `grep` supports multiple patterns, it doesn't have `greps` wrapper around `grep` for more than one pattern. `__ssh_authorized_keys` supports multiple keys in authorized keys file (since this entity contains multiple entries), and it does one thing: it manages ssh `authorized_keys` file.

ungleich-gitea commented

2021-11-20 15:29:06 +00:00

Author

Owner

mentioned in commit 17a9a86588

mentioned in commit 17a9a865880f4c37ed776bb1be627d98d4e57c85

ungleich-gitea commented

2021-11-20 15:29:08 +00:00

Author

Owner

@steven What do you think about the code in https://code.ungleich.ch/ungleich-public/cdist/-/compare/master...cleanup%2Fssh-auth-keys-types ?

This is only one, __ssh_authorized_keys file which manipulates one entity, i.e. ssh authorized_keys file with supported multiple key parameters.

@steven What do you think about the code in https://code.ungleich.ch/ungleich-public/cdist/-/compare/master...cleanup%2Fssh-auth-keys-types ? This is only one, `__ssh_authorized_keys` file which manipulates one entity, i.e. ssh `authorized_keys` file with supported multiple `key` parameters.

ungleich-gitea commented

2021-11-20 15:29:09 +00:00

Author

Owner

There is no mess and also no duplication. __ssh_authorized_keys uses __ssh_authorized_key. That's all.
Anyway, you are off course free to waste your time.

There is no mess and also no duplication. __ssh_authorized_keys uses __ssh_authorized_key. That's all. Anyway, you are off course free to waste your time.

ungleich-gitea commented

2021-11-20 15:29:10 +00:00

Author

Owner

mentioned in commit 137c48dc81227b89eb217b1a49240885839e128d

ungleich-gitea commented

2021-11-20 15:29:12 +00:00

Author

Owner

@steven I agree, nothing is broken. But it is a kind of a mess, with duplicating parts in both types. What we do with one key is the same we should do with more keys.

I have played with local manipulation + __file approach in 61268de876, but now I see that this should be generated code to be executed on the remote (@nico comment above - I forgot this).

I don't know if the code will be cleaner, simpler, if it will execute faster than many __ssh_authorized_key invocations, but let's see the final result and then decide. I don't mind if we throw it away and leave things as they are currently. If that will be the case I don't mind wasting my time.

@steven I agree, nothing is broken. But it is a kind of a mess, with duplicating parts in both types. What we do with one key is the same we should do with more keys. I have played with local manipulation + `__file` approach in https://code.ungleich.ch/ungleich-public/cdist/-/commit/61268de876c14d8dcfc969d7dbf12deb7d2e668f, but now I see that this should be generated code to be executed on the remote (@nico comment above - I forgot this). I don't know if the code will be cleaner, simpler, if it will execute faster than many `__ssh_authorized_key` invocations, but let's see the final result and then decide. I don't mind if we throw it away and leave things as they are currently. If that will be the case I don't mind wasting my time.

ungleich-gitea commented

2021-11-20 15:29:13 +00:00

Author

Owner

@nico For the other part, I see what you mean.

Hm... I guess the code I had in mind should be generated to be executed on the remote side, not locally.

@nico For the other part, I see what you mean. Hm... I guess the code I had in mind should be generated to be executed on the remote side, not locally.

ungleich-gitea commented

2021-11-20 15:29:14 +00:00

Author

Owner

Non managed keys are not the problem. Currently, __ssh_authorized_keys is ignoring them, except when remove-unknown parameter is used.

Non managed keys are not the problem. Currently, `__ssh_authorized_keys` is ignoring them, except when `remove-unknown` parameter is used.

ungleich-gitea commented

2021-11-20 15:29:16 +00:00

Author

Owner

mentioned in commit 61268de876c14d8dcfc969d7dbf12deb7d2e668f

ungleich-gitea commented

2021-11-20 15:29:17 +00:00

Author

Owner

The abstraction __ssh_authorized_key uses is good, as I use it to provisioning ssh keys for chroots (lxc containers). I think both are legit, because __ssh_authorized_key manage a single key and __ssh_authorized_keys provide a nice frontend for it.

The abstraction `__ssh_authorized_key` uses is good, as I use it to provisioning ssh keys for chroots (lxc containers). I think both are legit, because `__ssh_authorized_key` manage a single key and `__ssh_authorized_keys` provide a nice frontend for it.

ungleich-gitea commented

2021-11-20 15:29:18 +00:00

Author

Owner

Nothing is broken. Everything is working fine. Why fix something that is not broken?

__ssh_authorized_key is fully functional by itself. It is used to manage a single key in an authorized_keys file. It's 80 or so lines of straight forward simple shell code. It's working well. Nobody had to change anything substantial in the ~7 years since I wrote it.

__ssh_authorized_keys is a higher level type that allows managing a whole authorized_keys file. It relies on and uses __ssh_authorized_key because that does a good job at that one thing that it is meant to do.

To me this is just the unix way of doing things. Simple tools do one thing well. Put them to work together to archive something bigger.

I don't see any added value by trying to merge these two into one type.

Nothing is broken. Everything is working fine. Why fix something that is not broken? __ssh_authorized_key is fully functional by itself. It is used to manage a single key in an authorized_keys file. It's 80 or so lines of straight forward simple shell code. It's working well. Nobody had to change anything substantial in the ~7 years since I wrote it. __ssh_authorized_keys is a higher level type that allows managing a whole authorized_keys file. It relies on and uses __ssh_authorized_key because that does a good job at that one thing that it is meant to do. To me this is just the unix way of doing things. Simple tools do one thing well. Put them to work together to archive something bigger. I don't see any added value by trying to merge these two into one type.

ungleich-gitea commented

2021-11-20 15:29:20 +00:00

Author

Owner

@poljakowski I currently only use __ssh_authorized_keys because it's a nice wrapper and suits my needs, but I can see use cases where having __ssh_authorized_key can come in handy.
As @nico already said __ssh_authorized_key can be used from different locations (and they won't conflict).

One made up example could be a barebones Git server where you manage git@ users through a cdist type, but you also want your backup host to have access so that it can pull backups.
Doing something like this would be difficult with just one __ssh_authorized_keys implementation that takes control of the whole file.

If I had to decide for just one of the two types, I'd probably go with __ssh_authorized_key, because it is more flexible, even if it requires more cdist objects and maybe some more manifest code to achieve the common/simple things.

What I would support without reservation, is to clean up the code of the two types, because honestly it is a bit of a mess currently.

@poljakowski I currently only use `__ssh_authorized_keys` because it's a nice wrapper and suits my needs, but I can see use cases where having `__ssh_authorized_key` can come in handy. As @nico already said `__ssh_authorized_key` can be used from different locations (and they won't conflict). One made up example could be a barebones Git server where you manage `git@` users through a cdist type, but you also want your backup host to have access so that it can pull backups. Doing something like this would be difficult with just one `__ssh_authorized_keys` implementation that takes control of the whole file. If I had to decide for just one of the two types, I'd probably go with `__ssh_authorized_key`, because it is more flexible, even if it requires more cdist objects and maybe some more manifest code to achieve the common/simple things. What I would support without reservation, is to clean up the code of the two types, because honestly it is a bit of a mess currently.

ungleich-gitea commented

2021-11-20 15:29:21 +00:00

Author

Owner

This approach does not work, as you can use the type at multiple locations multiple times for the same user. Also the user might add / modify other keys that are not managed.

TL;DR: use __line.

This approach does not work, as you can use the type at multiple locations multiple times for the same user. Also the user might add / modify other keys that are not managed. TL;DR: use *__line*.

ungleich-gitea commented

2021-11-20 15:29:22 +00:00

Author

Owner

@ander @ssrq @matze @evilham @fnux

cc: @nico @steven

I started analyzing this one.

We have __ssh_authorized_key and __ssh_authorized_keys.

I would deprecate __ssh_authorized_key and modify __ssh_authorized_keys since its name reflects ssh authorized_keys file name (plural). So the idea is to merge __ssh_authorized_key type functionality into __ssh_authorized_keys type so that it does not depend on __ssh_authorized_key type.

I noticed that in __ssh_authorized_keys we have explorer keys which gets the entire specified authorized_keys file.
So, since we have remote authorized_keys file locally, what do you think about the simplification that we do what needs to be done with that file locally and finally transfer modified file to the remote, say using __file type?

@ander @ssrq @matze @evilham @fnux cc: @nico @steven I started analyzing this one. We have `__ssh_authorized_key` and `__ssh_authorized_keys`. I would deprecate `__ssh_authorized_key` and modify `__ssh_authorized_keys` since its name reflects ssh `authorized_keys` file name (plural). So the idea is to merge `__ssh_authorized_key` type functionality into `__ssh_authorized_keys` type so that it does not depend on `__ssh_authorized_key` type. I noticed that in `__ssh_authorized_keys` we have explorer `keys` which gets the entire specified `authorized_keys` file. So, since we have remote authorized_keys file locally, what do you think about the simplification that we do what needs to be done with that file locally and finally transfer modified file to the remote, say using `__file` type?

ungleich-gitea commented

2021-11-20 15:29:24 +00:00

Author

Owner

I believe that we should rethink the types and come up with a good API for the new __ssh_authorized_key(s).
So I think which one wins and is being merged into, doesn't matter so much.

I believe that we should rethink the types and come up with a good API for the new `__ssh_authorized_key(s)`. So I think which one wins and is being merged into, doesn't matter so much.

ungleich-gitea commented

2021-11-20 15:29:25 +00:00

Author

Owner

If you want to use the __file type with ~ for user directories, it will not work because it's quoted and therefor not expanded. Implementation requires custom logic for expansion. But working in the user directory is possible with the __dot_file type as well, which does the transition (but not for directories like the __ssh_dot_ssh type does).

If you want to use the `__file` type with `~` for user directories, it will not work because it's quoted and therefor not expanded. Implementation requires custom logic for expansion. But working in the user directory is possible with the `__dot_file` type as well, which does the transition (but not for directories like the `__ssh_dot_ssh` type does).

ungleich-gitea commented

2021-11-20 15:29:27 +00:00

Author

Owner

As of current (quite short) analysis I propose merging __ssh_authorized_key into __ssh_authroized_keys type.
__ssh_authorized_keys type has wider options, e.g. it has owner option.
Having both is superfluous, adding one or more than one keys should be done with one type.

As of current (quite short) analysis I propose merging `__ssh_authorized_key` into `__ssh_authroized_keys` type. `__ssh_authorized_keys` type has wider options, e.g. it has owner option. Having both is superfluous, adding one or more than one keys should be done with one type.

Cleanup ssh auth keys type #37