Request for review: keychain

[Chip Turner]

On 7/13/05, Alexander Dalloz <alex at dalloz.de> wrote:
> Am Mi, den 13.07.2005 schrieb Alexander Dalloz um 16:48:
> > Am Di, den 12.07.2005 sch
> 
> > I kindly request to review following bah script:
> >
> > http://www.uni-x.org/keychain.sh.txt
> 
> Gna! Found a very stupid already my own :(
> 
> line 4:  which keychain &>/dev/null || exit 1
> is of course a bad idea as it will prevent users from logging in when
> "keychain" isn't found. As I would like to let the profile script only
> run when keychain is existing on the host, my suggestion is
> 
> which keychain &>/dev/null && (
>     ... )
> 
> where the ( ) surround the whole script.
> 
> Corrected script:
> 
> http://www.uni-x.org/keychain.sh.2.txt

Overall a good start!  But, this script is a bit complicated.  Some comments:

1) The differentiation between rsa keys, dsa keys, and gpg keys is a
bit unnecessary since they all end up on the command line anyway.

2) You can just use 'local FOO=""' instead of FOO="" and then worrying
about unsetting FOO.

3) Along with 2, you can have a 'set -u' that will require all vars
declared, thus ensuring you don't end up with FOOs that need localing
or unsetting that you didn't realize

4) It would be nicer if a user just doing 'touch ~/.keychainrc' would
activate it.  Creating empty configs for users who don't know anything
about keychain (maybe the admin just installed it for herself!) is
confusing, especially since it isn't done silently.  Basically I'd
personally rather it be 'if keychainrc exists, set some defaults that
would load all the keys, then source the user's file letting them
override our defaults' and not 'require multiple settings in user's
config to have the system work properly'

5) creating files in a user's homedir silently is naughty, esp for
software they've never ever run.

6) The 'which keychain' is unnecessary since your script is part of
the keychain rpm -- one goes with the other :)

7) chmod'ing to 640 is bad if a user has a umask of 077, etc.

8) General scripting rule of thumb: when you nest four if loops deep,
something else is probably wrong.

Chip

-- 
Chip Turner                   chip.turner at gmail.com