Closed bradjones1 closed 6 years ago
Okay I actually had the time to look into this. I will try to do a proper comment on each change in the same order Github presents them.
Also, could you explain why we would need to set an expire date several thousand years into the future? Sure I could consider adding an expire date env variable as a feature. But under no circumstances should this be hard coded. It should be a variable, based on current date + x and it should be user configurable if needed.
fi
, after you removed mine). Another issue I have is that you don't use tabs for indentation, which makes it incompatible with my coding style.Then you got me thinking, would it not be safer and more flexible to split up the check and adding of $SFTP_CHROOT/$user and $SFTP_CHROOT/$user/.ssh. Just in case someone customizes the configuration and/or stores data externally? Simply put, make as few assumptions as possible.
ChrootDirectory
is a bit of a special one. I agree it would seem more logical, but unfortunately thins will break in certain setups with your change. I know this because I used to test every kind of setup combination. My memory does not allow for a great explanation, but I will try.The problem is if you set the default value at build time like this is your data cannot be copied to an external data storage by the script. The README value is still the default, but it needs to check what kind of storage solution you are using before it actually sets it in the config file. This therefor has to be done at runtime.
I also feel I need to repeat that I cannot accept merges into master. If you would rebase against beta it would be no issue. In the future projects I will probably have a "stable" tag that is reused instead of using master as a stable branch.
A few quick changes to make the behaviour match the spec outlined in the README. Also primes a .ssh directory for users to add authorized keys.