Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Host keys auto generator outside config dir #124

Closed
sergejostir opened this issue May 28, 2020 · 13 comments
Closed

Host keys auto generator outside config dir #124

sergejostir opened this issue May 28, 2020 · 13 comments
Labels
enhancement New feature or request

Comments

@sergejostir
Copy link

sftpgo/sftpd/server.go

Lines 518 to 536 in a08dd85

if len(c.HostKeys) == 0 {
defaultKeys := []string{defaultPrivateRSAKeyName, defaultPrivateECDSAKeyName}
for _, k := range defaultKeys {
autoFile := filepath.Join(configDir, k)
if _, err := os.Stat(autoFile); os.IsNotExist(err) {
logger.Info(logSender, "", "No host keys configured and %#v does not exist; creating new key for server", autoFile)
logger.InfoToConsole("No host keys configured and %#v does not exist; creating new key for server", autoFile)
if k == defaultPrivateRSAKeyName {
err = utils.GenerateRSAKeys(autoFile)
} else {
err = utils.GenerateECDSAKeys(autoFile)
}
if err != nil {
return err
}
}
c.HostKeys = append(c.HostKeys, k)
}
}

Looking at this, it seems that you can utilize auto generation of the host keys, only if host_keys setting is empty. Otherwise you have to provide your own.

Would it be possible to add another setting, so we can specify a directory where our host keys should be regardless of our config dir, and if they don't exist they are automatically created?

@drakkan
Copy link
Owner

drakkan commented May 28, 2020

sftpgo/sftpd/server.go

Lines 518 to 536 in a08dd85

if len(c.HostKeys) == 0 {
defaultKeys := []string{defaultPrivateRSAKeyName, defaultPrivateECDSAKeyName}
for _, k := range defaultKeys {
autoFile := filepath.Join(configDir, k)
if _, err := os.Stat(autoFile); os.IsNotExist(err) {
logger.Info(logSender, "", "No host keys configured and %#v does not exist; creating new key for server", autoFile)
logger.InfoToConsole("No host keys configured and %#v does not exist; creating new key for server", autoFile)
if k == defaultPrivateRSAKeyName {
err = utils.GenerateRSAKeys(autoFile)
} else {
err = utils.GenerateECDSAKeys(autoFile)
}
if err != nil {
return err
}
}
c.HostKeys = append(c.HostKeys, k)
}
}

Looking at this, it seems that you can utilize auto generation of the host keys, only if host_keys setting is empty. Otherwise you have to provide your own.

Correct. If you use the default configuration then an RSA/2048 and a ECDSA/P-256 key will be automatically generated inside the config dir (this is a sane default very similar to OpenSSH), otherwise if you specify a relative or absolute path the keys must be there.

Would it be possible to add another setting, so we can specify a directory where our host keys should be regardless of our config dir, and if they don't exist they are automatically created?

If you specify a path for your keys you probably want a specific key size/algorithm (different from the default ones), I don't want to expose all these details or try to deduce them from the key name. If you specify only one key it must be RSA (2048? 4096?...) or EDSA (P-256?,P-384?,P-512?...,) and if you specify 3 keys?

Eventually I could add a sub command to generate keys but could this be better than ssh-keygen?

@sergejostir
Copy link
Author

I'm not asking for any advanced thing, a simple string setting to specify a path where keys should be.
Basically just this
autoFile := filepath.Join(configDir, k) -> if host_keys_path (or something alike) not null, then use it, otherwise use configDir.

Don't need to touch anything else. If you need more advanced configuration you have host_keys setting already.

@drakkan
Copy link
Owner

drakkan commented Jun 8, 2020

Hi,

If you want your keys auto created inside inside another directory you can use an hack such as this one, from the config directory create these symlinks to missing files:

ln -s /tmp/id_rsa . 
ln -s /tmp/id_rsa.pub .
ln -s /tmp/id_ecdsa .
ln -s /tmp/id_ecdsa.pub .

and start sftpgo, the keys will be created in /tmp.

I'm sorry but we cannot add a new feature only because it is simple to do, we must try to keep the configuration options as low as possibile and I don't see a valid rationale here, you can:

  1. simply start sftpgo and the host keys will be autogenerated with some sane defaults
  2. provide your own host keys, generated with the options you like, specifying any location you like
  3. use some hacks to autogenerate keys in other locations
  4. write your own setup script that, for example, starts sftpgo, waits for keys autogeneration, copies the keys somewhere and modify sftpgo configuration to use the new location

Anyway if you can send a well written pull request with full test cases coverage I'll evaluate and eventually merge it, but I'm not going to add this feature myself, sorry

@drakkan drakkan closed this as completed Jun 8, 2020
@drakkan drakkan added the wontfix This will not be worked on label Jun 8, 2020
@sergejostir
Copy link
Author

Thanks for your input.

The use case is for when you use SFTPGo in docker container (where you want to maintain maintain minimal image) and you want to have host keys preserved (so you bind that folder to host FS), but you use environment variables to handle your config (however there's requirement to explicitly specify values to be overriden; so you want to keep this config file in container's FS, but host keys on host FS - that is impossible currently, if you also want keys to be autogenerated by SFTPGo).

Since this won't be possible, my question is, why is this the case:

Please note that, to override configuration options with environment variables, a configuration file containing the options to override is required. You can, for example, deploy the default configuration file and then override the options you need to customize using environment variables.

Why can't we override values with env vars without also maintaining our own config file?

@drakkan
Copy link
Owner

drakkan commented Jun 8, 2020

Thanks for your input.

The use case is for when you use SFTPGo in docker container (where you want to maintain maintain minimal image) and you want to have host keys preserved (so you bind that folder to host FS), but you use environment variables to handle your config (however there's requirement to explicitly specify values to be overriden; so you want to keep this config file in container's FS, but host keys on host FS - that is impossible currently, if you also want keys to be autogenerated by SFTPGo).

I'm not a Docker expert so I could miss something here. Can you please explain why you cannot have a config directory on the host (as documented inside the docs)?

Since this won't be possible, my question is, why is this the case:

Please note that, to override configuration options with environment variables, a configuration file containing the options to override is required. You can, for example, deploy the default configuration file and then override the options you need to customize using environment variables.

Why can't we override values with env vars without also maintaining our own config file?

this is a viper issue spf13/viper#761

@sergejostir
Copy link
Author

I'm not a Docker expert so I could miss something here. Can you please explain why you cannot have a config directory on the host (as documented inside the docs)?

I set my configuration using ENV vars, but considering that we also have to have a physical config file (because of viper's issue), I also have to put that file somewhere. However, that file is not supposed to be changed (it only includes default values), since configuration is set with ENV vars.
Let's say I have this default config file at in "/etc/sftpgo" (and I set SFTPGO_CONFIG_DIR to "/etc/sftpgo").

With docker you can bind files or folders from host FS into container FS. If I bind container's "/etc/sftpgo" to my host FS, then my default config file is inaccessible since that path will now point to my binding on host FS. If I don't bind it, then host keys will be created in container's "/etc/sftpgo" and lost every time container is recreated.

One way to overcome this is, so put my default config somewhere else in container's FS, and then copy it to "/etc/sftpgo" on every start, which will then also expose it to host's FS, even though that file is completely irrelevant and it gives the host operator an illusion that it is there to be edited, but it will have no effect.
The other way is to install ssh-keygen in container and then script the same thing that is already included in sftpgo, to create host keys in desired folder (for example /etc/sftpgo/keys and use host_keys setting).

With the additional config key, that would decouple host keys location from config location, all it would take is to set config dir to "/etc/sftpgo" and host keys dir to "/etc/sftpgo/keys" (while binding "/etc/sftpgo/keys" to host FS).

@drakkan drakkan reopened this Jun 8, 2020
@drakkan drakkan added enhancement New feature or request and removed wontfix This will not be worked on labels Jun 8, 2020
@drakkan drakkan closed this as completed in cd38097 Jun 8, 2020
@jovandeginste
Copy link
Contributor

Another way to do this, is to list the keys in your config...?

@sergejostir
Copy link
Author

@jovandeginste, if you did that, you would need to manually generate those keys.

@jovandeginste
Copy link
Contributor

Ah I see. But you reuse your keys across restarts of the same service, right? (So, generate once, and then reuse them)

@sergejostir
Copy link
Author

Yes, but that was problematic, if you use sftpgo in docker container and at the same time want to manage config with env vars. You can read the full explanation at #124 (comment)

@jovandeginste
Copy link
Contributor

You can also override the location of the config file. If you combine the SFTPGO_CONFIG_FILE and SFTPGO_CONFIG_DIR, you can probably find a way to make it work (maybe override a few more files, like log file).

I have a similar use case as yours, but I "solved" it by fetching the keys from our Vault when the container starts (we use Nomad as a scheduler). This means I can inject the private keys anywhere on the container's file system (but I need to generate them beforehand and store them in Vault).

That being said, I agree your suggestion is a good one. The other solutions are just workarounds...

@drakkan
Copy link
Owner

drakkan commented Jun 9, 2020

Yes, but that was problematic, if you use sftpgo in docker container and at the same time want to manage config with env vars. You can read the full explanation at #124 (comment)

Hi, this feature was implemented, I did minimal testing (basically only the included test case) please let me know if it works as expected, thanks

@sergejostir
Copy link
Author

I can confirm that it is working as expected. Thank your very much for this addition!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants