[pacman-dev] [PATCH 1/5] pacman-key: keyring management tool

[Denis A. Altoé Falqueto]

On Monday 26 July 2010 18:46:27 Dan McGee wrote:
> > +# gettext initialization
> > +export TEXTDOMAIN='pacman'
> > +export TEXTDOMAINDIR='@localedir@'
>
> You don't end up actually using gettext anywhere. A script like this
> actually should.

Yes, totally forgot. Done.

> > +
> > +# Based on apt-key, from Debian
> > +# Author: Denis A. Altoé Falqueto <denisfalqueto at gmail dot com>
>
> These quickly become outdated- I'm fine with the copyright, but if you
> want this you also need to fix every single problem that comes up with
> the script. git does a perfectly fine job at telling you who the
> original author is.

I'm fine with handing off the copyright to Pacman Developers too, if
you don't mind. i didn't do it at first because i'm not a pacman
developer, so didn't to put me in a position I'm not. I'm not applying
for it, this is just an occasional help :)

> > +PACMAN_SHARE_DIR="/usr/share/pacman"
>
> This needs to be autoconf-ed. See doc/Makefile.am and pkgdatadir and
> use that like @sysconfdir@ in makepkg.sh.in.

Done. Corrected to @pkgdatadir@, which is in Makefile.am, in source root.

> > +
> > +# Default parameters for the command gpg. Some more will be added when
> > needed +GPG="gpg"
> > +GPG_NOKEYRING="${GPG} --ignore-time-conflict --no-options
> > --no-default-keyring" +SIG_EXT=".sig"
>
> Would we ever change this? I'd rather have .sig hardcoded just like we
> killed other things that will never really change because they are
> convention outside of just this package.

Yes, I was going to discuss in that other reply. In fact, the
variables are there not because they can change. I just don't feel
comfortable to use strings literals that mean something to the script.
If I mistype them in one place and nobody sees it, it is a fool bug
that slips off. That way, bash will make sure that the right values
are on the right places.

> > +usage() {
> > +       echo "pacman-key - Pacman's keyring management utility"
> > +       echo "Usage: $(basename $0) [options] command [arguments]"
> > +       echo
> > +       echo "Manage pacman's list of trusted keys"
> > +       echo
> > +       echo "Options must be placed before commands. The abailable
> > options are:" +       echo " --config    - set an alternative
> > configuraton file to use. " +       echo "               Default is
> > @sysconfdir@/pacman.conf" +       echo " --gpgdir    - set an alternativ
> > home directory for gnupg. " +       echo "               Default is
> > @sysconfdir@/pacman.d/gnupg" +       echo
> > +       echo "The available commands are:"
> > +       echo "  pacman-key add <file> ...                  - add the key
> > contained in <file> ('-' for stdin)" +       echo "  pacman-key del
> > <keyid> ...                 - remove the key <keyid>" +       echo "
> >  pacman-key export <keyid> ...              - output the key <keyid>" +
> >       echo "  pacman-key exportall                       - output all
> > trusted keys" +       echo "  pacman-key receive <keyserver> <keyid> ...
> > - fetch the keyids from the specified keyserver URL" +       echo "
> >  pacman-key trust <keyid> ...               - set the truslevel of the
> > given key" +       echo "  pacman-key updatedb                        -
> > update the trustdb of pacman" +       echo "  pacman-key reload
> >                  - reloads the keys from the keyring package" +
> > echo "  pacman-key list                            - list keys" +
> > echo "  pacman-key finger <keyid> ...              - list fingerprints"
> > +       echo "  pacman-key adv <params>                    - pass
> > advanced options to gpg" +       echo "  pacman-key help
> >            - displays this message" +       echo "  pacman-key version
> >                       - displays the current version" +}
>
> I forgot we did this in makepkg; I don't really like it there. This
> stuff seems like it belongs in a manpage with a shorter usage() such
> as the one in repo-add. Anyone else have thoughts?
>
> Either way, for a essential tool, we will need a manpage.

The man page will be done for sure, I just wanted to share my work soon :)

About the usage message, I don't think it is too long. It just gives a
quick explanation. But I can shorten it, stripping the details of each
command.

> > +
> > +prepare_homedir() {
> > +       if [[ ! -d ${PACMAN_KEYRING_DIR} ]] ; then
> > +               mkdir -p "${PACMAN_KEYRING_DIR}"
> > +               [[ ! -f "${PACMAN_KEYRING_DIR}/secring.gpg" ]] && touch
> > "${PACMAN_KEYRING_DIR}/secring.gpg" +               [[ ! -f
> > "${PACMAN_KEYRING_DIR}/pubring.gpg" ]] && touch
> > "${PACMAN_KEYRING_DIR}/pubring.gpg"
>
> If the dir didn't exist, why this -f check?

Yes, you're right. Corrected.

> > +               chmod 700 "${PACMAN_KEYRING_DIR}"
> > +               chmod 600 "${PACMAN_KEYRING_DIR}"/*
>
> Why the glob? Can't we just explicitly list the two files previously
> touched? Minor nitpick but seems more sensible to me.

Done too.

> > +       fi
> > +}
> > +
> > +add_key() {
> > +       ${GPG_PACMAN} --quiet --batch --import "$1"
> > +}
> > +
> > +remove_key() {
> > +       ${GPG_PACMAN} --quiet --batch --delete-key --yes "$1"
> > +}
> > +
> > +update_trustdb() {
> > +       ${GPG_PACMAN} --batch --check-trustdb
> > +}
> > +
> > +list_sigs() {
> > +       ${GPG_PACMAN} --batch --list-sigs
> > +}
> > +
> > +list_fingerprints() {
> > +       ${GPG_PACMAN} --batch --fingerprint $*
> > +}
> > +
> > +export_key() {
> > +       ${GPG_PACMAN} --armor --export "$1"
> > +}
> > +
> > +export_all() {
> > +       ${GPG_PACMAN} --armor --export
> > +}
>
> If these are all one-liners, it would seem to make more sense to
> inline them if they are only called from one place. Ignore me if they
> are not.

I did that way because of homogeneity. I moved them for the calling
places. just update_trustdb can be called from two places, so it
stayed.

> > +
> > +trust_key() {
> > +       # Verify if the key exists in pacman's keyring
> > +       if ${GPG_PACMAN} --list-key "$1" > /dev/null 2>&1 ; then
> > +               ${GPG_PACMAN} --edit-key "$1"
> > +       else
> > +               echo >&2 "The key identified by $1 doesn't exist"
>
> I'd rather find a way to reuse msg/msg2/warning/error from repo-add
> then do this. We might want to move those into a sourced common
> utilities file.

Right. When the decision about the file is made, I'll change it.

> > +       if [[ -f ${REMOVED_KEYS} ]] && ! ${GPG_PACMAN} --quiet --verify
> > ${REMOVED_KEYS}${SIG_EXT} ; then
>
> You quoted paths elsewhere but then didn't quote here or many of the
> following places.

Yes, I corrected the quotes in the other places. If there's still some
missing, please let me know.

> > +if [[ ! -r "$CONFIG" ]] ; then
> > +       echo >&2 "It is not possible to read $CONFIG."
> > +       exit 1
> > +fi
> > +# Read GPGDIR from $CONFIG
> > +GPGDIR=$(grep "^ *GPGDir *=" "$CONFIG" | cut -d= -f2)
> > +if [[ "$GPGDIR" ]] ; then
> > +       PACMAN_KEYRING_DIR="$GPGDIR"
> > +fi
>
> Hmm, this is kinda ugly but not sure what to suggest just yet.

Agreed :) But pacman.conf is a kind of ini file, so the correct
parsing would give too much trouble. Looking now, it doesn't work if
there is a tab before GPGDir... I'll fix it tomorow.

> > +       --help)
> > +               usage
> > +               ;;
> > +       --version)
> > +               echo "pacman-key v${PACMAN_KEY_VERSION}"
> > +               echo " This program can be freely distributed under the
> > GPL v2" +               ;;
>
> Please use a similar version() function to makepkg/repo-add.
>
> You also have help/version above, and --help/--version here.

Done.

> > +       *)
> > +               usage
> > +               exit 1
> > +               ;;
> > +esac
> > --
> > 1.7.1.1
>
> General thoughts:
> 1. We don't use the non-dashed option type commands in any other
> utility. I'm not necessarily against it but thought it was worth
> pointing out.

Agreed. I already changed it to short and long options. I just got
influenced by apt-key, because they always use non-dashed commands, as
in apt-get install, etc.

> 2. A lot of times our style in shell scripts has tended towards
> omitting braces, e.g. ${foobar} would be just $foobar if possible.

I think I've seen some patches in pacman-dev changing variables that
way (${} insttead of just $). But I can change it too, no big deal.

> 3. This was a static code analysis, I need to actually run this and
> give it a shot.
> 4. Don't get discouraged at my review here, I'm just trying to make
> sure when we introduce something new we don't have to spend the next
> few iterations making it a solid core piece of code. I'd rather get it
> right the first time.

Don't worry about that. We both want the same goal.

I'll try to send the new patch in reply to this email, as soon as I
learn how to do it with git send-email, but i'll leave that for
tomorrow. 3 am here and I'm barely able to write this message :)

-- 
A: Because it obfuscates the reading.
Q: Why is top posting so bad?

-------------------------------------------
Denis A. Altoe Falqueto
-------------------------------------------