[pacman-dev] [PATCH 1/5] pacman-key: keyring management tool
Allan McRae
allan at archlinux.org
Wed Aug 4 09:17:35 EDT 2010
On 28/07/10 13:50, Denis A. Altoé Falqueto wrote:
> The script pacman-key will manage pacman's keyring. It imports, exports,
> fetches from keyservers, helps in the process of trusting and updates
> the trust database.
>
> Signed-off-by: Denis A. Altoé Falqueto<denisfalqueto at gmail.com>
Hi Denis,
I think it would be good for us to focus on getting this onto the gpg
branch and then move onto the other patches. I do not think this
requires massive changes to be ready.
> ---
> scripts/.gitignore | 1 +
> scripts/Makefile.am | 3 +
> scripts/pacman-key.sh.in | 280 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 284 insertions(+), 0 deletions(-)
> create mode 100644 scripts/pacman-key.sh.in
>
> diff --git a/scripts/.gitignore b/scripts/.gitignore
> index eafc493..1c662de 100644
> --- a/scripts/.gitignore
> +++ b/scripts/.gitignore
> @@ -4,3 +4,4 @@ rankmirrors
> repo-add
> repo-remove
> pkgdelta
> +pacman-key
> diff --git a/scripts/Makefile.am b/scripts/Makefile.am
> index 31e8fb5..c81f703 100644
> --- a/scripts/Makefile.am
> +++ b/scripts/Makefile.am
> @@ -7,6 +7,7 @@ bin_SCRIPTS = \
>
> OURSCRIPTS = \
> makepkg \
> + pacman-key \
> pacman-optimize \
> pkgdelta \
> rankmirrors \
> @@ -14,6 +15,7 @@ OURSCRIPTS = \
>
> EXTRA_DIST = \
> makepkg.sh.in \
> + pacman-key.sh.in \
> pacman-optimize.sh.in \
> pkgdelta.sh.in \
> rankmirrors.sh.in \
> @@ -60,6 +62,7 @@ $(OURSCRIPTS): Makefile
> @mv $@.tmp $@
>
> makepkg: $(srcdir)/makepkg.sh.in
> +pacman-key: ${srcdir}/pacman-key.sh.in
> pacman-optimize: $(srcdir)/pacman-optimize.sh.in
> pkgdelta: $(srcdir)/pkgdelta.sh.in
> rankmirrors: $(srcdir)/rankmirrors.sh.in
> diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in
> new file mode 100644
> index 0000000..05745a9
> --- /dev/null
> +++ b/scripts/pacman-key.sh.in
> @@ -0,0 +1,280 @@
> +#!/bin/bash -e
> +#
> +# pacman-key - manages pacman's keyring
> +# @configure_input@
> +#
> +# Copyright (c) 2010 - Pacman Development Team<pacman-dev at archlinux.org>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see<http://www.gnu.org/licenses/>.
> +#
> +
> +# gettext initialization
> +export TEXTDOMAIN='pacman'
> +export TEXTDOMAINDIR='@localedir@'
> +
> +# Based on apt-key, from Debian
Move that comment up, either right below the copyright of below the
@condigure_input@? It seems out of place there.
> +myver="@PACKAGE_VERSION@"
> +
> +# According to apt-key, gpg doesn't like to be called without a secret keyring.
Has that been verified? If so, remove the "According to apt-key". But
as far as I can tell, gpg just dislikes no keyrings being added. See
the man page and the --no-default-keyring option.
> +# We will not really need one, because pacman will not sign packages, just verify
> +# their integrities.
And why exactly is that whole comment here? I can not figure out how it
is relevant to the following line.
> +PACMAN_SHARE_DIR="@pkgdatadir@"
> +
> +# Default parameters for the command gpg. Some more will be added when needed
> +GPG="gpg"
Do we need this? The makepkg changes assume "gpg" is the program name
for signing. If it is going to be set by a variable, I would prefer to
autoconf it and have it default to gpg with a configure switch to change
it. But I really do not think it is necessary at all.
> +GPG_NOKEYRING="${GPG} --ignore-time-conflict --no-options --no-default-keyring"
This is used only twice in the same function. I would prefer it defined
there - see comment in relevant place below.
> +SIG_EXT=".sig"
Get rid of this variable. Its value can be considered standard.
> +
> +# Read-only keyring with keys to be added to the keyring
> +ADDED_KEYS="${PACMAN_SHARE_DIR}/addedkeys.gpg"
That seems a weird place to put these files to me. I think that
@sysconfdir@/pacman.d/gnupg/ would be a better place. Maybe that
directory could be configured...
> +# Read-only keyring with keys removed from the keyring. They need to be removed before
> +# the keys from the added keyring be really imported
..from the added keyring are imported?
Anyway, that amount of detail is unneeded. Maybe just have "Read-only
keyrings provided by distribution" as the comment for both these files.
> +REMOVED_KEYS="${PACMAN_SHARE_DIR}/removedkeys.gpg"
> +
> +usage() {
> + printf "pacman-key (pacman) %s\n" ${myver}
> + echo
> + printf $(gettext "Usage: %s [options] command [arguments]") $(basename $0)
> + echo
> + echo $(gettext "Manage pacman's list of trusted keys")
> + echo
> + echo $(gettext "Options must be placed before commands. The abailable options are:")
typo - available
> + echo $(gettext " --config - set an alternative configuraton file to use. ")
typo - configuration
> + echo $(gettext " Default is @sysconfdir@/pacman.conf")
> + echo $(gettext " --gpgdir - set an alternativ home directory for gnupg. ")
typo - alternative
> + echo $(gettext " Default is @sysconfdir@/pacman.d/gnupg")
Default is set in @sysconfdir@/pacman.conf.
> + echo
> + echo $(gettext "The available commands are:")
> + echo $(gettext " pacman-key -a | --add<file> ... - add the key contained ")
> + echo $(gettext " in<file> ('-' for stdin)")
I could not find reference the behaviour on passing "-" to --import in
the gpg man page. Worth double checking.
> + echo $(gettext " pacman-key -d | --del<keyid> ... - remove the key<keyid>")
> + echo $(gettext " pacman-key -e | --export<keyid> ... - output the key<keyid>")
> + echo $(gettext " pacman-key -x | --exportall - output all trusted keys")
> + echo $(gettext " pacman-key -r | --receive<keyserver> <keyid> ... - fetch the keyids from")
> + echo $(gettext " the specified keyserver URL")
> + echo $(gettext " pacman-key -t | --trust<keyid> ... - set the truslevel of the given key")
typo - trust level
> + echo $(gettext " pacman-key -u | --updatedb - update the trustdb of pacman")
> + echo $(gettext " pacman-key --reload - reloads the keys from the keyring package")
> + echo $(gettext " pacman-key -l | --list - list keys")
> + echo $(gettext " pacman-key -f | --finger<keyid> ... - list fingerprints")
"fingerprints" is not clear. i.e. is it just the fingerprint of <keyid>
of does omitting that print them all? Also, see below.
> + echo $(gettext " pacman-key --adv<params> - pass advanced options to gpg")
This is a bit vague. It sounds like I would be passing additional
options to gpg than what the program already is passing rather than the
manually telling gpg what to do to the pacman keyring.
Maybe "manually manage the pacman keyring"
> + echo $(gettext " pacman-key -h | --help - displays this message")
> + echo $(gettext " pacman-key -v | --version - displays the current version")
> +}
> +
> +version() {
> + printf "pacman-key (pacman) %s\n" "${myver}"
> + printf "$(gettext "\
> +Copyright (c) 2010 Pacman Development Team<pacman-dev at archlinux.org>.\n\
> +This is free software; see the source for copying conditions.\n\
> +There is NO WARRANTY, to the extent permitted by law.\n")"
> +}
> +
> +prepare_homedir() {
> + if [[ ! -d "${PACMAN_KEYRING_DIR}" ]] ; then
> + mkdir -p "${PACMAN_KEYRING_DIR}"
> + touch "${PACMAN_KEYRING_DIR}/secring.gpg"
> + touch "${PACMAN_KEYRING_DIR}/pubring.gpg"
> + chmod 700 "${PACMAN_KEYRING_DIR}"
> + chmod 600 "${PACMAN_KEYRING_DIR}"/{sec,pub}ring.gpg
We should just use:
install -dm700 ${PACMAN_KEYRING_DIR}
to create the directory with the right permissions.
And should those files actually be part of the pacman package and so
guaranteed to be present.
> + fi
> +}
> +
> +update_trustdb() {
> + ${GPG_PACMAN} --batch --check-trustdb
Should we be using --update-trustdb?
> +}
> +
> +reload_keyring() {
> + # Verify the signature of removed keys file
> + if [[ -f "${REMOVED_KEYS}" ]]&& ! ${GPG_PACMAN} --quiet --verify "${REMOVED_KEYS}${SIG_EXT}" ; then
> + echo>&2 $(gettext "The signature of file ${REMOVED_KEYS} is not valid.")
$(gettext "The signature of file %s is not valid.") "${REMOVED_KEYS}"
> + exit 1
> + fi
> +
> + # Verify the signature of the added keys file
> + if [[ -f "${ADDED_KEYS}" ]]&& ! ${GPG_PACMAN} --quiet --verify "${ADDED_KEYS}${SIG_EXT}" ; then
> + echo>&2 $(gettext "The signature of file ${ADDED_KEYS} is not valid.")
$(gettext "The signature of file %s is not valid.") "${ADDED_KEYS}"
> + exit 1
> + fi
> +
> + # Remove the keys from REMOVED_KEYS keyring
> + [[ -r "${REMOVED_KEYS}" ]]&& cat "${REMOVED_KEYS}" | while read key ; do
> + ${GPG_PACMAN} --quiet --batch --yes --delete-keys "${key}"
--delete-key
> + done
> +
> + # Add keys from the current set of keys from pacman-keyring package. The web of trust will
> + # be updated automatically.
> + if [[ -r "${ADDED_KEYS}" ]] ; then
I'd define something like:
local GPG_COMMAND="${GPG} --ignore-time-conflict --no-options
--no-default-keyring --keyring "${ADDED_KEYS}"
here rather than the GPG_NOKEYRING variable at the start as it is used
no-where else.
> + add_keys=$(${GPG_NOKEYRING} --keyring "${ADDED_KEYS}" --with-colons --list-keys | grep ^pub | cut -d: -f5)
local add_keys
> + for key in ${add_keys}; do
> + ${GPG_NOKEYRING} --quiet --batch --keyring "${ADDED_KEYS}" --export "${key}" | ${GPG_PACMAN} --import
> + done
> + fi
> +
> + # Update trustdb, just to be sure
> + update_trustdb
> +}
> +
> +receive() {
> + keyserver="$1"
local keyserver
> + shift
> + ${GPG_PACMAN} --keyserver "${keyserver}" $*
--recv-keys is needed?
> +}
> +
> +# PROGRAM START
> +
> +if ! type gettext&>/dev/null; then
> + gettext() {
> + echo "$@"
> + }
> +fi
> +
> +if [[ "$command" != "version"&& "$command" != "help" ]]&& ! which "${GPG}">/dev/null 2>&1; then
"! type -p gpg" instead of which. Also, quotes around $command are
unnecessary and they should be compared to "--version" and "--help".
Also... $command is not defined yet! You would be wanting $1.
> + echo>&2 $(gettext "Warning: gnupg does not seem to be installed.")
> + echo>&2 $(gettext "Warning: pacman-key requires gnupg for most operations.")
It is not a warning if you are exiting... so change that to "Error:".
Also, the second output line does not need "Warning/Error:" at the start.
> + exit 1
> +fi
> +
> +# Parse global options
> +CONFIG="@sysconfdir@/pacman.conf"
> +PACMAN_KEYRING_DIR="@sysconfdir@/pacman.d/gnupg"
> +while [[ $1 =~ ^--(config|gpgdir)$ ]] ; do
> + case "$1" in
> + --config) shift; CONFIG="$1" ;;
> + --gpgdir) shift; PACMAN_KEYRING_DIR="$1" ;;
> + esac
> + shift
> +done
> +
> +if [[ ! -r "${CONFIG}" ]] ; then
> + echo>&2 $(gettext "It is not possible to read ${CONFIG}.")
$(gettext "It is not possible to read %s.") "${CONFIG}"
> + exit 1
> +fi
> +
> +# Read GPGDIR from $CONFIG.
> +# The pattern is: any spaces or tabs, GPGDir, any spaces or tabs, equal sign
> +# and the rest of the line. The string is splitted after the first occurrence of =
> +GPGDIR=$(cat ${CONFIG} | awk '/^(\t| )*GPGDir(\t| )*=.*/ { print substr($0,index($0, "=")+1) }')
cat a file to awk it... yuck. Also, we do not use awk anywhere else,
so grep followed by a bash substitution to remove "*=" from the start
may be better.
> +if [[ "${GPGDIR}" ]] ; then
-z $GPGDIR?
> + PACMAN_KEYRING_DIR="${GPGDIR}"
> +fi
> +GPG_PACMAN="${GPG} --homedir ${PACMAN_KEYRING_DIR}"
> +
> +prepare_homedir
> +
> +# Parse and execute command
> +command="$1"
> +if [[ -z "${command}" ]]; then
> + usage
> + exit 1
> +fi
> +shift
> +
> +case "${command}" in
> + -a|--add)
> + if (( $# == 0 )) ; then
> + echo>&2 $(gettext "You need to specify at least one key identifier")
Error: You need...
(and throughout.)
> + usage
> + exit 1
> + fi
> + while (( $#> 0 )) ; do
> + ${GPG_PACMAN} --quiet --batch --import "$1"
> + shift
> + done
> + ;;
> + -d|--del)
> + if (( $# == 0 )) ; then
> + echo>&2 $(gettext "You need to specify at least one key identifier")
> + usage
> + exit 1
> + fi
> + while (( $#> 0 )) ; do
> + ${GPG_PACMAN} --quiet --batch --delete-key --yes "$1"
> + shift
> + done
> + ;;
> + -u|--updatedb)
> + update_trustdb
I am not sure this single line function needs to not be inline (even
though it is called once elsewhere).
> + ;;
> + --reload)
> + reload_keyring
> + ;;
> + -l|-list)
> + ${GPG_PACMAN} --batch --list-sigs
> + ;;
> + -f|--finger)
> + if (( $# == 0 )) ; then
> + echo>&2 $(gettext "You need to specify at least one key identifier")
> + usage
> + exit 1
> + fi
It appears --fingerprint works fine without an argument...
> + ${GPG_PACMAN} --batch --fingerprint $*
> + ;;
> + -e|--export)
> + if (( $# == 0 )) ; then
> + echo>&2 $(gettext "You need to specify at least one key identifier")
> + usage
> + exit 1
> + fi
> + while (( $#> 0 )) ; do
> + ${GPG_PACMAN} --armor --export "$1"
> + shift
> + done
> + ;;
> + -x|--exportall)
> + ${GPG_PACMAN} --armor --export
> + ;;
This can be combined with --export. No need for a separate option.
if (( $# == 0 ));
... --export
else
while (( $# > 0 )); do...
> + -r|--receive)
> + if (( $#< 2 )) ; then
> + echo>&2 $(gettext "You need to specify the keyserver and at least one key identifier")
> + usage
> + exit 1
> + fi
> + receive $*
The receive function is only 3 lines so could be inlined.
> + ;;
> + -t|--trust)
> + if (( $# == 0 )) ; then
> + echo>&2 $(gettext "You need to specify at least one key identifier")
> + usage
> + exit 1
> + fi
> + while (( $#> 0 )) ; do
> + # Verify if the key exists in pacman's keyring
> + if ${GPG_PACMAN} --list-key "$1"> /dev/null 2>&1 ; then
--list-keys
> + ${GPG_PACMAN} --edit-key "$1"
> + else
> + echo>&2 $(gettext "The key identified by $1 doesn't exist")
$(gettext "The key identified by %s does not exist") $1
> + exit 1
> + fi
> + shift
> + done
> + ;;
> + --adv)
> + echo $(gettext "Executing: ${GPG_PACMAN} $*")
$(gettext "Executing: %s") "${GPG_PACMAN} $*"
(I think... check formatting works)
> + ${GPG_PACMAN} $* || ret=$?
> + exit $ret
> + ;;
> + --help)
> + usage
> + ;;
> + --version)
> + version
> + exit 0
> + ;;
> + *)
> + usage
> + exit 1
> + ;;
> +esac
Add blank line at the end.
General thoughts:
1) I see that I have made _A LOT_ of comments here. But as I said at
the top, I feel this is basically ready to go so please do not get
discouraged. Blame Dan. He has turned me into a pedantic bastard! :)
2) I think there should also be a check that this is being run as root
when not using --version or --help. See what makepkg does to check for
root.
3) We will really need a man-page for this. The usage() output is a bit
brief to stand alone.
4) I still have not actually run this... this is entirely static code
review.
Allan
More information about the pacman-dev
mailing list