[pacman-dev] [PATCH v2] pacman-key: ignores keys already lsigned/deleted

[Allan McRae]

On 5/11/19 5:36 am, Matthew Sexton wrote:
> Added two new functions, lsigned_already() and revoked_already()
> that check whether a key has been locally signed or revoked
> respectively during --populate. If the key is already signed
> or revoked, it is quietly ignored.
> 

It looks as though the implementation is fine at first glance. I also
see this was discussed on the IRC channel with Dave and Eli, so I'm
focusing on stylistic elements here.

The function naming is more about what you are going to do with its
output, rather than what the function does.  I suggest:

lsigned_already() -> key_is_lsigned()
revoked_already() -> key_is_revoked()

Also, you added the functions in alphabetically.  But that is the
collection of functions directly related to pacman-key options.  Please
move them up to where all the helper functions are at the top.  Under
check_keyids_exist() seems a logical place.

Finally, the code uses tabs, not spaces for indents.  You seem to have
some mixture of the two.


> Suggested-by: Eli Schwartz <eschwartz at archlinux.org>
> Signed-off-by: Matthew Sexton <wsdmatty at gmail.com>
> ---
> v2. (ACTUAL v2.) Previous email was erroneous, I attached the wrong
> patch. Corrected some inconsistencies. Gave proper attribution to Eli,
> 
>  scripts/pacman-key.sh.in | 43 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in
> index 3627a805..25bcb3de 100644
> --- a/scripts/pacman-key.sh.in
> +++ b/scripts/pacman-key.sh.in
> @@ -247,7 +247,7 @@ check_keyring() {
>  		fi
>  	fi
>  
> -	if (( LSIGNKEY )); then
> +	if (( LSIGNKEY || POPULATE )); then
>  		if [[ $(secret_keys_available) -lt 1 ]]; then
>  			error "$(gettext "There is no secret key available to sign with.")"
>  			msg "$(gettext "Use '%s' to generate a default secret key.")" "pacman-key --init"
> @@ -337,13 +337,18 @@ populate_keyring() {
>  		local key_count=0
>  		msg "$(gettext "Disabling revoked keys in keyring...")"
>  		for key_id in "${!revoked_ids[@]}"; do
> +            if revoked_already "$key_id" ; then
> +                continue
> +            fi
>  			if (( VERBOSE )); then
>  				msg2 "$(gettext "Disabling key %s...")" "${key_id}"
>  			fi
>  			printf 'disable\nquit\n' | LANG=C "${GPG_PACMAN[@]}" --command-fd 0 --quiet --batch --edit-key "${key_id}" 2>/dev/null
>  			key_count=$((key_count+1))
>  		done
> -		msg2 "$(gettext "Disabled %s keys.")" "${key_count}"
> +		if (( key_count )); then
> +			msg2 "$(gettext "Disabled %s keys.")" "${key_count}"
> +		fi
>  	fi
>  }
>  
> @@ -447,6 +452,22 @@ list_sigs() {
>  		exit 1
>  	fi
>  }
> +lsigned_already() {
> +	# Determines whether a key has already been signed locally by getting the
> +	# local pacman secret key and comparing it against signatures on the key
> +	# returns 0 if key is signed, 1 if it is unsigned
> +	secret_key=$("${GPG_PACMAN[@]}" --with-colons --list-secret-key | head -n1 | awk -F : '{print $5}')
> +    while IFS=: read -r _ valid _ _ signkey _; do
> +            if [[ "$valid" != "!" ]]; then

We don't quote the left hand side.

> +                continue
> +            fi
> +            if [[ "$signkey" = "$secret_key" ]]; then
> +                return 0
> +            fi
> +	done < <("${GPG_PACMAN[@]}" --with-colons --check-signatures "$1")
> +	return 1
> +
> +}
>  
>  lsign_keys() {
>  	check_keyids_exist
> @@ -454,6 +475,7 @@ lsign_keys() {
>  	local ret=0
>  	local key_count=0
>  	for key_id in "$@"; do
> +		if lsigned_already "$key_id" ; then	continue; fi

Put this over multiple lines.

>  		if (( VERBOSE )); then
>  			msg2 "$(gettext "Locally signing key %s...")" "${key_id}"
>  		fi
> @@ -469,7 +491,9 @@ lsign_keys() {
>  	if (( ret )); then
>  		exit 1
>  	fi
> -	msg2 "$(gettext "Locally signed %s keys.")" "${key_count}"
> +	if (( key_count )); then
> +		msg2 "$(gettext "Locally signed %s keys.")" "${key_count}"
> +	fi
>  }
>  
>  receive_keys() {
> @@ -511,6 +535,19 @@ refresh_keys() {
>  	fi
>  }
>  
> +revoked_already() {
> +
> +    while IFS=: read -r type _ _ _ _ _ _ _ _ _ _ flags _; do
> +            if [[ "$type" != "pub" ]]; then
> +                continue
> +            fi
> +            if [[ "$flags" = *"D"* ]]; then

That quoting on the RHS looked weird to me, but I think is fine...

> +                return 0
> +            fi
> +	done < <("${GPG_PACMAN[@]}" --with-colons --list-key "$1")
> +	return 1
> +}
> +
>  verify_sig() {
>  	local ret=0 sig=$1 file=$2
>  	if [[  -z $file && -f ${sig%.*} ]]; then
>