Move opkssh key files to a separate location

[net42-jkeil]

This PR changes how the key files opkssh creates on the client are stored.

Addressed problems:
It prevents problems in case the predefined allowed identity files (id_ecdsa / id_ed25519) are already in use. (#69)
If a user has to log into multiple systems handled by different providers the user has to run opkssh login and ssh in that order for each system because repeated calls of opkssh login will override the PK of the previous provider.

Changes:

• Identity files opkssh creates are named unique per provider and clientID
• Additionally they get created in ~/.ssh/opkssh/ to reduce clutter and make cleaning up easier
• Identity management happens in ~/.ssh/opkssh/config

Notable:
Identities that do not follow the standard naming convention or are not placed in the default location ~/.ssh/ are usually ignored. To fix this the Include ~/.ssh/opkssh/config directive has to be added to the users ssh config.
This is to prevent possible corruption when regularly modifying the users ssh config.

The README has been modified to reflect this change. Additionally the command opkssh config has been added which checks for correct configuration and instructs the user how to modify their config if needed.

With the upcoming PR #96 most users will not encounter those problems, but on systems without the ssh-agent this remains a problem.

[MrCriseas]

I know, we are from the same organisation but would be nice. Not everyone is using ssh-agent and the package would be directly useable without setting up the agent. Nevertheless to potentially overwrite an ssh certificate and check is it used beforehand by opk seems unlikely not an optimal solution.

We have the usecase to use multiple providers and that solution could address our needs massively and support this repository :).

[EthanHeilman]

Want to talk about this at this months community meeting?

OpenPubkey Community Meeting 2:00 PM (ET) 6:00pm (GMT)
Wednesday, April 16 · 2:00 – 3:00pm
Time zone: America/New_York
Google Meet joining info
Video call link: https://meet.google.com/hvc-dywm-wzk

[net42-jkeil]

As mentioned in the community meeting this PR will be on hold until client configuration is implemented.
This PR might need a complete rewrite at that point to cleanly integrate with the new configuration method.

[EthanHeilman]

Merged the client config.

We don't automatically create the client config you have to run opkssh login --create-config. That said, if the usability of this PR requires that we automatically create the client config. I am good with making that change.
#143

[net42-jkeil]

Finally got around to refactor the PR to use the client config.

The functionality changed slightly. I now use the ~/.opk directory for the opk ssh config. This seemed like the right location to me.

The opk config.yml got extended to include key_management with a default_key_dir and use_identity_config directive.
Those set the directory for opk's keys and whether or not it should add those keys as IdentityFile entries to a ssh config in ~/.opk/config.
If true, it adds the Include directive to the users ssh config.

I added this information to the readme.

Users with an existing config and users that create the config after this PR will see no change to the default behavior. It requires manual changes to the config to take effect.

[MrCriseas]

I propose to use https://specifications.freedesktop.org/basedir-spec/latest/ as standard definition for the locations and as fallback $HOME/.config/opk/... for default configs and .ssh/opk for the ssh keys.
The xdg is more likely the standard to define locations, isn't?

[net42-jkeil]

I fixed the login integration test that failed.
Are you ok with adding the SetConfig Method to LoginCMD or do you have a better solution to set a config?

[EthanHeilman]

@net42-jkeil The client config is only used by login at the moment so it seems natural that login manage it. That said, I'm not sure exactly what you mean here. What would the SetConfig function do? What is the intent?

[net42-jkeil]

With my PR the login function checks l.config.KeyManagement.DefaultKeyDir for a modified config.
If the variable is set it will handle the key placement and if nessesary add an Identity.

Alternatively I could extend the if check to prevent it from accessing the uninitialized config variable.
I was not expecting config to be uninitialized in the integration testing since it is always set in normal execution.

[EthanHeilman]

When you say config do you mean the client config ~/.opk/config.yml? I was originally opposed to automatically creating ~/.opk/config.yml on first call to opkssh login but I've been rethinking that. If it is easier to assume ~/.opk/config.yml exists, then create it on first opkssh login call.

[net42-jkeil]

What I meant was a variable in code.
I made an assumption in code that was not always true. This assumption was corrected in the last commit.

This way I could remove my changes to the integration test, too.

I for my part do not require a ~/.opk/config.yml.

[EthanHeilman]

Sorry, I misunderstood you.

The config variable on the login command struct should always be set regardless of it if a config file exists or not. It is set in login.Run.

But yeahl.config.KeyManagement.DefaultKeyDir might not be set.

[net42-jkeil]

@EthanHeilman let me know once you got to check the PR and if there is anything preventing a merge.
Thanks!

[EthanHeilman]

I'll have more free time on Thursday, so I don't get a review done by Wednesday, I'll give it a review Thursday night.

[EthanHeilman]

Thanks for your hard work on this

[net42-jkeil]

The git history should be corrected. README, error logs, var blocks and inline declaration changes made as requested.
I couldn't apply your suggestions because I had to rebase to fix the history.

[EthanHeilman]

That was a fast turn around! No worry about applying my suggestions. Suggestions typically break the linter because github doesn't apply lint suggests and my suggestions are intended to communicate an idea not be 100% correct code.

[net42-jkeil]

@EthanHeilman I think I addressed all suggestions and notes, please review when you have the time

[EthanHeilman]

@net42-jkeil Going to take this for a test drive today and give it a review.

[EthanHeilman]

I tried this, but I couldn't login in because I already had a config.yml which didn't define the default_key_dir so it defaulted to ~/ which meant ssh couldn't find it.

I added this to my config.yml

key_management:
  default_key_dir: ~/.ssh
  use_identity_config: false

but I got the following error

2025/05/22 16:29:57 Error executing login command: error logging in: failed to write SSH keys to ssh dir: open C:\Users\e0\.ssh\C:\Users\e0\.ssh\id_ecdsa: The filename, directory name, or volume label syntax is incorrect.
Error: error logging in: failed to write SSH keys to ssh dir: open C:\Users\e0\.ssh\C:\Users\e0\.ssh\id_ecdsa: The filename, directory name, or volume label syntax is incorrect.

[net42-jkeil]

@EthanHeilman I completely missed Windows here. I have no environment to test it, but using filepath build-ins should do the trick.
Please let me know if that fixed the problems 👍

[EthanHeilman]

@net42-jkeil Checked and it works on windows now when

key_management:
  default_key_dir: ~/.ssh
  use_identity_config: false

or no key_management set

1. Testing with default_key_dir != ~/.ssh and use_identity_config = false

When I change this to

key_management:
  default_key_dir: ~/.opk/config/
  use_identity_config: false

I get

PS C:\Users\e0\Documents\GitHub\opkssh> .\opkssh.exe login
2025/05/25 14:26:57 Error executing login command: failed key dir check: could not get stats of key directory: CreateFile C:\Users\e0\.opk\config: The system cannot find the file specified.
Error: failed key dir check: could not get stats of key directory: CreateFile C:\Users\e0\.opk\config: The system cannot find the file specified.

After creating this directory it worked.

This isn't a bug, but I think it is a better UX if we automatically create the directory for the user. When default_key_dir != ~/.ssh and use_identity_config != false we should output a warning to the console that the ssh might not be able to find the ssh keys.

2 . Testing with default_key_dir = ~/.opk/config/ and use_identity_config == true

I ran into the issue that if not ssh config exists I get

025/05/25 14:37:19 Error executing login command: error logging in: failed to write SSH keys to configured key dir (~/.opk/config/): failed to read opk ssh config: read C:\Users\e0\.opk\config: Incorrect function.
Error: error logging in: failed to write SSH keys to configured key dir (~/.opk/config/): failed to read opk ssh config: read C:\Users\e0\.opk\config: Incorrect function.

It did create this file C:\Users\e0\.ssh\config.new but ssh didn't seem to find it.

2025/05/25 14:45:12 Error executing login command: error logging in: failed to write SSH keys to configured key dir (/.opk/config/): failed to read opk ssh config: read C:\Users\e0.opk\config: Incorrect function.
Error: error logging in: failed to write SSH keys to configured key dir (/.opk/config/): failed to read opk ssh config: read C:\Users\e0.opk\config: Incorrect function.

3 . Testing with default_key_dir = ~/.ssh/opkssh/ and use_identity_config == true

Created ~/.ssh/opkssh/,
Created ~/.ssh/config,
Created ~/.ssh/opkssh/config

Writing opk ssh public key to C:\Users\e0\.ssh\opkssh\accounts.google.com-206584157355-7cbe4s6.pub and corresponding secret key to C:\Users\e0\.ssh\opkssh\accounts.google.com-206584157355-7cbe4s6
2025/05/25 14:54:52 Error executing login command: error logging in: failed to write SSH keys to configured key dir (~/.ssh/opkssh/): failed to read opk ssh config: read C:\Users\e0\.opk\config: Incorrect function.
Error: error logging in: failed to write SSH keys to configured key dir (~/.ssh/opkssh/): failed to read opk ssh config: read C:\Users\e0\.opk\config: Incorrect function.

It creates a file ~/.ssh/config.new whose contain is:

Include C:\Users\e0\.opk\config

I changed this to the correct value Include C:\Users\e0\.ssh\opkssh\config and reran opkssh login it changed it to.

Include C:\Users\e0\.opk\config
Include C:\Users\e0\.ssh\opkssh\config

So I can't get it to work

EDIT: I see what I messed up. I assumed if the keys were in ~/.ssh/opkssh/ then the config would be in there as well, but the config for the SSH keys is ~/.opk/config and then it points to ~/.ssh/opkssh/.

• Deleted ~/.opk/config/
• Created ~/.ssh/opkssh/

And it worked perfectly

[net42-jkeil]

Thank you for your review!

All of the Incorrect function errors were caused by using the config file paths as paths for directories. Once Go tries to access them as files this is the error you get.
As you noticed at the end those errors were corrected after removing the directories.
I implemented a simple check to warn the user if they do something similar.

better UX if we automatically create the directory for the user

I implemented this so that the config directory ~/.ssh also gets created if use_identity_config = true.
Path creation to the desired key dir was corrected.

It did create this file C:\Users\e0\.ssh\config.new but ssh didn't seem to find it.

This is only a temporary copy of the users SSH config I create. It is modified to include opkssh's SSH config and renamed to ~/.ssh/config overriding the original once I am sure that all steps were successful.

Testing in a Windows 10 VM things seem to work for me. Please retest with other paths.

default_key_dir: ~/.opk/mykeys

[EthanHeilman]

I'm going to delay merge this PR for a few days because I want to get a new release out. I want to keep the release small in case we run into any bugs.

This is very valuable PR and I really want to get non-agent based key management right.

[EthanHeilman]

I'm been giving this PR a lot of thought and I came to the realization that we are using the config file option use_identity_config to flag to opkssh to perform a setup task that we can't undo. Namely writing to ~/.ssh/config. We can't undo it because we can't tell the lines we add from the lines the user adds if the key directory changes. It is like we are trying to control the ssh config in the opkssh config.

This is my proposal:

We get rid of use_identity_config and default_key_directory and instead add opkssh login --configure which performs the following actions:

1. Creates the directory ~/.ssh/opkssh/,
2. Creates the file ~/.ssh/opkssh/keys,
3. Appends the line Include ~/.ssh/opkssh/keys to ~/.ssh/config where the keys file does the current job of ~/.opk/config but renamed to avoid confusion with ~/.opk/config.yml.

To determine if we should write to ~/.ssh/id_ecdsa or use new key names we check if the file ~/.ssh/opkssh/keys exists or not.

• This reduces the chance someone breaks opkssh by editing the default_key_directory config option.
• We don't have to handle the use_identity_config true --> false transition.

What do you think about this? This seems closer to what you originally proposed with this PR, but I am have been overly excited about using the client config file which was a mistake on my part.

[net42-jkeil]

I had a chat with my colleagues and they agree that everything related to SSH (config and keys) belong in the ~/.ssh directory. So the move there is a good idea.
However naming the configuration file keys is misleading. I would rather call it config too, since it is part of the users SSH config once included.

The --configure command will create ~/.ssh/opkssh, ~/.ssh/opkssh/configand add the config to the users/.ssh/config` along with quite a bit of logging.

I'm reworking this PR right now. I will force push a clean version build with the updated main branch as the base.

[EthanHeilman]

However naming the configuration file keys is misleading. I would rather call it config too, since it is part of the users SSH config once included.

Agreed, this is less likely to be confusing if it is in the .ssh direcotry

[net42-jkeil]

Please let me know once you had the chance to review this. Thank you

[EthanHeilman]

I don't have time this week, but I'm going to make a push to get this reviewed and merged next week. This is currently one PR ahead of this PR for merging

[EthanHeilman]

LGTM

[EthanHeilman]

@net42-jkeil Thanks for all your hard work on this. Really appreciate it and sticking with the PR review. This was an usually long review process. The benefit this brings to project especially when a user has different IDPs for different servers.

[net42-jkeil]

Thank you for being open to this change. Lets see if we find more opportunities for contributions while using it internally.