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

[Denis A. Altoé Falqueto]

On Wed, Aug 4, 2010 at 10:17 AM, Allan McRae <allan at archlinux.org> wrote:
> 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.

Hi.

Sorry for the delay again. Time is so short lately... It took way
longer than I would like. But here I am again. I'll answer only the
things that I would like to discuss further. The other points were
implemented as advised by you.

>> +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.

Yes, I believe the best place is pacman package. I removed the
function, so we need to make sure the PKGBUILD for pacman creates the
proper files and directory.

>> +update_trustdb() {
>> +       ${GPG_PACMAN} --batch --check-trustdb
>
> Should we be using --update-trustdb?

>From gpg's man page:

"The processing is identical to that of --update-trustdb but it skips
keys with a not yet defined "ownertrust"."

I'm not sure what is the best option. update-trustdb may ask the user
what is the ownertrust value for the keys gpg can't compute with the
web of trust. check-trustdb ignores those keys. According to the man
page, none of them are necessary. The values are computed correctly
when importing keys. Should we drop that option? It would be still
accessible through --adv command, if someone really needs it.

>> +# 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.

I see. I googled about grep and tabs and some links showed that grep
couldn't accept correctly the \t character, but I found now that the
class [:blank:] is what we really need: spaces and tabs. And grep
accepts it. I'll change the script.

>> +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
> (...)
> 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.

The man page will be done, no doubt about that. But not right now, as
don't have all the time I would like to have. If any of the
collaborators want to help, this is the time (hint for all those who
mailed me telling they would be glad to help :D)

> 4) I still have not actually run this... this is entirely static code
> review.

I've tried all options and they are working alright.

I'll send the new version of script in the next email, as a reply to
this message.

Thanks for the patience.

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

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