Cleanup ssh auth keys type #37

Closed
opened 2021-11-20 11:25:02 +00:00 by ungleich-gitea · 20 comments

We should clean them up, there are too many types.

@ssrq @ander If you have a smart proposal, feel free to share it.

We should clean them up, there are too many types. @ssrq @ander If you have a smart proposal, feel free to share it.
ungleich-gitea added the
doing
cleanup
improvement
labels 2021-11-20 11:25:02 +00:00
poljakowski was assigned by ungleich-gitea 2021-11-20 11:25:02 +00:00
Author
Owner

Since there is no (active, recent) interest in this one I am closing it.

Since there is no (active, recent) interest in this one I am closing it.
Author
Owner

mentioned in merge request !982

mentioned in merge request !982
Author
Owner

mentioned in commit e1c5263c37

mentioned in commit e1c5263c37d2e5d3ba2a3d6afba4b5e8511f5e2c
Author
Owner

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.

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.
Author
Owner

mentioned in commit 17a9a86588

mentioned in commit 17a9a865880f4c37ed776bb1be627d98d4e57c85
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.
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.
Author
Owner

mentioned in commit 137c48dc81227b89eb217b1a49240885839e128d

mentioned in commit 137c48dc81227b89eb217b1a49240885839e128d
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.
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.
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.
Author
Owner

mentioned in commit 61268de876c14d8dcfc969d7dbf12deb7d2e668f

mentioned in commit 61268de876c14d8dcfc969d7dbf12deb7d2e668f
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.
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.
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.
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*.
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?
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.
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).
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.
Sign in to join this conversation.
No Milestone
No project
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: ungleich-public/cdist#37
No description provided.