[pacman-dev] Unusable pacman-key help by default

older
[pacman-dev] Python 3 bindings for...

Dan McGee

28 Mar 2011 28 Mar '11

5:27 p.m.

This is not cool at all, patches welcome from anyone. :) dmcgee@clifden ~/projects/pacman (master) $ ./scripts/pacman-key --help mkdir: cannot create directory `/etc/pacman.d/gnupg': Permission denied

Show replies by date

Ray Kohler

28 Mar 28 Mar

5:34 p.m.

New subject: [pacman-dev] [PATCH] Only try to create keyring directory when root

If we try and fail to create it, the -e flag to bash will make --help and --version break. Signed-off-by: Ray Kohler --- scripts/pacman-key.sh.in | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in index 89e52fc..057b3bb 100644 --- a/scripts/pacman-key.sh.in +++ b/scripts/pacman-key.sh.in @@ -249,7 +249,7 @@ GPG_PACMAN="gpg --homedir ${PACMAN_KEYRING_DIR} --no-permission-warning" # Try to create $PACMAN_KEYRING_DIR if non-existent # Check for simple existence rather than for a directory as someone may want # to use a symlink here -[[ -e ${PACMAN_KEYRING_DIR} ]] || mkdir -p -m 755 "${PACMAN_KEYRING_DIR}" +(( EUID != 0 )) || [[ -e ${PACMAN_KEYRING_DIR} ]] || mkdir -p -m 755 "${PACMAN_KEYRING_DIR}" # Parse and execute command command="$1" -- 1.7.4.2

Ray Kohler

29 Mar 29 Mar

10:21 p.m.

New subject: [pacman-dev] [PATCH] Only try to create keyring directory when root

On Mon, Mar 28, 2011 at 1:34 PM, Ray Kohler wrote:

...

If we try and fail to create it, the -e flag to bash will make --help and --version break.

Signed-off-by: Ray Kohler --- scripts/pacman-key.sh.in | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in index 89e52fc..057b3bb 100644 --- a/scripts/pacman-key.sh.in +++ b/scripts/pacman-key.sh.in @@ -249,7 +249,7 @@ GPG_PACMAN="gpg --homedir ${PACMAN_KEYRING_DIR} --no-permission-warning" # Try to create $PACMAN_KEYRING_DIR if non-existent # Check for simple existence rather than for a directory as someone may want # to use a symlink here -[[ -e ${PACMAN_KEYRING_DIR} ]] || mkdir -p -m 755 "${PACMAN_KEYRING_DIR}" +(( EUID != 0 )) || [[ -e ${PACMAN_KEYRING_DIR} ]] || mkdir -p -m 755 "${PACMAN_KEYRING_DIR}"

# Parse and execute command command="$1" -- 1.7.4.2

Dan, was this not an acceptable means of dealing with the broken "pacman-key -h"? I see you workshopping someone else's much more detailed patch for the same issue. I sent mine in immediately on seeing you report the bug - I don't want it to be said that I don't fix bugs I introduce. In any case, if you don't want it for whatever reason, let me know so I can drop it from my tree.

Dan McGee

10:25 p.m.

New subject: [pacman-dev] [PATCH] Only try to create keyring directory when root

On Tue, Mar 29, 2011 at 5:21 PM, Ray Kohler wrote:

...

On Mon, Mar 28, 2011 at 1:34 PM, Ray Kohler wrote:

...
If we try and fail to create it, the -e flag to bash will make --help and --version break.

Signed-off-by: Ray Kohler --- scripts/pacman-key.sh.in | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in index 89e52fc..057b3bb 100644 --- a/scripts/pacman-key.sh.in +++ b/scripts/pacman-key.sh.in @@ -249,7 +249,7 @@ GPG_PACMAN="gpg --homedir ${PACMAN_KEYRING_DIR} --no-permission-warning" # Try to create $PACMAN_KEYRING_DIR if non-existent # Check for simple existence rather than for a directory as someone may want # to use a symlink here -[[ -e ${PACMAN_KEYRING_DIR} ]] || mkdir -p -m 755 "${PACMAN_KEYRING_DIR}" +(( EUID != 0 )) || [[ -e ${PACMAN_KEYRING_DIR} ]] || mkdir -p -m 755 "${PACMAN_KEYRING_DIR}"

# Parse and execute command command="$1" -- 1.7.4.2

Dan, was this not an acceptable means of dealing with the broken "pacman-key -h"? I see you workshopping someone else's much more detailed patch for the same issue. I sent mine in immediately on seeing you report the bug - I don't want it to be said that I don't fix bugs I introduce.

In any case, if you don't want it for whatever reason, let me know so I can drop it from my tree.

Sorry- I forgot to get back to you, but I thought you would pick up on it from the other conversation. This patch makes --help not break, but doesn't do it in a way I like. I have a bad taste in my mouth for all UID-specific workarounds in most pacman code due to this monstrosity: http://projects.archlinux.org/pacman.git/tree/src/pacman/util.c#n89 -Dan

Rémy Oudompheng

11:07 p.m.

New subject: [pacman-dev] [PATCH] Only try to create keyring directory when root

On 2011/3/30 Dan McGee wrote:

...

Sorry- I forgot to get back to you, but I thought you would pick up on it from the other conversation. This patch makes --help not break, but doesn't do it in a way I like. I have a bad taste in my mouth for all UID-specific workarounds in most pacman code due to this monstrosity: http://projects.archlinux.org/pacman.git/tree/src/pacman/util.c#n89

That's makes me remember a day when I wanted to use pacman to maintain packages in my HOME on a foreign system. Is it desirable to get rid of that? That's actually very strange that pacman wants to check such a thing whereas libalpm doesn't. Proper write permission checking in libalpm seems to be sufficient (with good error catching in pacman). -- Rémy.

Dan McGee

11:16 p.m.

New subject: [pacman-dev] [PATCH] Only try to create keyring directory when root

On Tue, Mar 29, 2011 at 6:07 PM, Rémy Oudompheng
 wrote:
> On 2011/3/30 Dan McGee  wrote:
>> Sorry- I forgot to get back to you, but I thought you would pick up on
>> it from the other conversation. This patch makes --help not break, but
>> doesn't do it in a way I like. I have a bad taste in my mouth for all
>> UID-specific workarounds in most pacman code due to this monstrosity:
>> http://projects.archlinux.org/pacman.git/tree/src/pacman/util.c#n89
>
> That's makes me remember a day when I wanted to use pacman to maintain
> packages in my HOME on a foreign system. Is it desirable to get rid of
> that? That's actually very strange that pacman wants to check such a
> thing whereas libalpm doesn't.
You haven't been here long, have you? :) pacman and libalpm have a lot
of gotchas like this, as the frontend/backend split was done a tad
oddly way back when. libalpm would eat s**t pretty fast without this
safeguard though, as far as I know.

> Proper write permission checking in libalpm seems to be sufficient
> (with good error catching in pacman).
That's the quick assumption, then you start to peel the layers from
the onion like we've all tried in the past because this problem seems
simple. You forgot:
* ldconfig invocation
* scriptlets and chrooting, even if root is "/" (see commit 5d30c5c0b76e)
* extraction permissions- chown. chmod, etc.
* database locking (commit f4ecc908eccc3a for example)

-Dan

Ivan c00kiemon5ter Kanak

28 Mar 28 Mar

11:05 p.m.

On 28 March 2011 20:27, Dan McGee wrote:

...

This is not cool at all, patches welcome from anyone. :)

dmcgee@clifden ~/projects/pacman (master) $ ./scripts/pacman-key --help mkdir: cannot create directory `/etc/pacman.d/gnupg': Permission denied

Hi all, I'm pretty new here, so I'm not sure how I should send this. I've read the submitting-patches page but there are lots of things to experiment with. So I thought I'd just send it as it is now and look more into how to submit patches tomorrow and maybe ask for some help on irc. This is actually the output of 'git show'. also two comments: a. On line 227 we define a default location for the PACMAN_KEYRING_DIR . On lines 228-234 we read the command line options and set the PACMAN_KEYRING_DIR to the one specified by the appropriate switch. On lines 244-246 we read the configuration file and look for GPGDir variable which sets again the PACMAN_KEYRING_DIR. I find this behaviour a bit strange. I would expect the command line options to overide the configuration file settings. Is that a bug or a wanted behavior ? b. I like that we use bash, and not restrict ourselves to sh. Is that valid for devtools too ? Are we restricted to some bash version ? I've seen some of the devtools scripts and I think bash would make some things easier and cleaner. --- commit 4004b8e927705cab0e0d4fefcf7d04cf7630a2e7 Author: Ivan Kanakarakis Date: Tue Mar 29 01:47:07 2011 +0300 This fixes the issue raised by Dan McGee, the wrong processing of -h, --help switches. It ignores any other switch given if any of those two are present. It also skips the root check. Why would one need to be root to see the help message ? Signed-off-by: Ivan Kanakarakis diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in index 89e52fc..490dc39 100644 --- a/scripts/pacman-key.sh.in +++ b/scripts/pacman-key.sh.in @@ -211,6 +211,14 @@ if ! type gettext &>/dev/null; then } fi +opts=($@) +for opt in ${opts[@]}; do + if [[ $opt == "--help" || $opt == "-h" ]]; then + usage + exit 0 + fi +done + if [[ $1 != "--version" && $1 != "-V" && $1 != "--help" && $1 != "-h" && $1 != "" ]]; then if type -p gpg >/dev/null 2>&1 = 1; then error "$(gettext "gnupg does not seem to be installed.")" @@ -316,8 +324,6 @@ case "${command}" in ${GPG_PACMAN} "$@" || ret=$? exit $ret ;; - -h|--help) - usage; exit 0 ;; -V|--version) version; exit 0 ;; *)

Dan McGee

29 Mar 29 Mar

5:08 p.m.

On Mon, Mar 28, 2011 at 6:05 PM, Ivan c00kiemon5ter Kanak wrote:

...

On 28 March 2011 20:27, Dan McGee wrote:

...
This is not cool at all, patches welcome from anyone. :)

dmcgee@clifden ~/projects/pacman (master) $ ./scripts/pacman-key --help mkdir: cannot create directory `/etc/pacman.d/gnupg': Permission denied

Hi all, I'm pretty new here, so I'm not sure how I should send this. I've read the submitting-patches page but there are lots of things to experiment with. So I thought I'd just send it as it is now and look more into how to submit patches tomorrow and maybe ask for some help on irc. This is actually the output of 'git show'. git format-patch and git send-email are preferred (required?)- this should be in that document.

...

also two comments: a. On line 227 we define a default location for the PACMAN_KEYRING_DIR . On lines 228-234 we read the command line options and set the PACMAN_KEYRING_DIR to the one specified by the appropriate switch. On lines 244-246 we read the configuration file and look for GPGDir variable which sets again the PACMAN_KEYRING_DIR. I find this behaviour a bit strange. I would expect the command line options to overide the configuration file settings. Is that a bug or a wanted behavior ? Command line should always override config file, so this is a bug. default < config file < command line.

...

b. I like that we use bash, and not restrict ourselves to sh. Is that valid for devtools too ? Devtools isn't managed on this list, but as most of those scripts have #!/bin/bash, draw your own conclusions there. Are we restricted to some bash version ? I've seen some of the devtools scripts and I think bash would make some things easier and cleaner. Here we try to stay 3.2+ compatible; devtools are Arch-specific so

This check also seems totally bogus: if [[ ! -r "${CONFIG}" ]]; then Why should we care? We have a default anyway if the config file isn't there. probably no such restriction.

...

---

commit 4004b8e927705cab0e0d4fefcf7d04cf7630a2e7 Author: Ivan Kanakarakis Date: Tue Mar 29 01:47:07 2011 +0300

This fixes the issue raised by Dan McGee, the wrong processing of -h, --help switches. It ignores any other switch given if any of those two are present. It also skips the root check. Why would one need to be root to see the help message ?

Signed-off-by: Ivan Kanakarakis

diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in index 89e52fc..490dc39 100644 --- a/scripts/pacman-key.sh.in +++ b/scripts/pacman-key.sh.in @@ -211,6 +211,14 @@ if ! type gettext &>/dev/null; then } fi

+opts=($@) +for opt in ${opts[@]}; do + if [[ $opt == "--help" || $opt == "-h" ]]; then + usage + exit 0 + fi +done

I hate this special casing. You also omitted -V/--version. And now repo-add and this script do it in two completely different ways. One case statement to parse all opts in both of these scripts should be used if possible; delaying any necessary actions until the command has been determined. If each of the add/update/etc. ops called a "setup_gpg" method first or something that did the config file parsing and ensuring the GPG directory exists, that would be best.

...

+ if [[ $1 != "--version" && $1 != "-V" && $1 != "--help" && $1 != "-h" && $1 != "" ]]; then if type -p gpg >/dev/null 2>&1 = 1; then error "$(gettext "gnupg does not seem to be installed.")" @@ -316,8 +324,6 @@ case "${command}" in ${GPG_PACMAN} "$@" || ret=$? exit $ret ;; - -h|--help) - usage; exit 0 ;; -V|--version) version; exit 0 ;; *)

Ivan c00kiemon5ter Kanak

5:22 p.m.

...

On Mon, Mar 28, 2011 at 6:05 PM, Ivan c00kiemon5ter Kanak wrote:

...
On 28 March 2011 20:27, Dan McGee wrote:

...
This is not cool at all, patches welcome from anyone. :)

dmcgee@clifden ~/projects/pacman (master) $ ./scripts/pacman-key --help mkdir: cannot create directory `/etc/pacman.d/gnupg': Permission denied

Hi all, I'm pretty new here, so I'm not sure how I should send this. I've read the submitting-patches page but there are lots of things to experiment with. So I thought I'd just send it as it is now and look more into how to submit patches tomorrow and maybe ask for some help on irc. This is actually the output of 'git show'. git format-patch and git send-email are preferred (required?)- this should be in that document.

They are ;) it was actually 2am for me, so I thought I just send this and

On 29 March 2011 20:08, Dan McGee wrote: try those today.

...

...
also two comments: a. On line 227 we define a default location for the PACMAN_KEYRING_DIR . On lines 228-234 we read the command line options and set the PACMAN_KEYRING_DIR to the one specified by the appropriate switch. On lines 244-246 we read the configuration file and look for GPGDir variable which sets again the PACMAN_KEYRING_DIR. I find this behaviour a bit strange. I would expect the command line options to overide the configuration file settings. Is that a bug or a wanted behavior ? Command line should always override config file, so this is a bug. default < config file < command line.

right. this is easy to fix, just rearrange the check order.

...

This check also seems totally bogus: if [[ ! -r "${CONFIG}" ]]; then Why should we care? We have a default anyway if the config file isn't there.

...
b. I like that we use bash, and not restrict ourselves to sh. Is that valid for devtools too ? Devtools isn't managed on this list, but as most of those scripts have #!/bin/bash, draw your own conclusions there. Are we restricted to some bash version ? I've seen some of the devtools scripts and I think bash would make some things easier and cleaner. Here we try to stay 3.2+ compatible; devtools are Arch-specific so probably no such restriction.

good to know, thanks :)

...

...
---

commit 4004b8e927705cab0e0d4fefcf7d04cf7630a2e7 Author: Ivan Kanakarakis Date: Tue Mar 29 01:47:07 2011 +0300

This fixes the issue raised by Dan McGee, the wrong processing of -h, --help switches. It ignores any other switch given if any of those two are present. It also skips the root check. Why would one need to be root to see the help message ?

Signed-off-by: Ivan Kanakarakis

diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in index 89e52fc..490dc39 100644 --- a/scripts/pacman-key.sh.in +++ b/scripts/pacman-key.sh.in @@ -211,6 +211,14 @@ if ! type gettext &>/dev/null; then } fi

+opts=($@) +for opt in ${opts[@]}; do + if [[ $opt == "--help" || $opt == "-h" ]]; then + usage + exit 0 + fi +done

I hate this special casing. You also omitted -V/--version. And now repo-add and this script do it in two completely different ways.

One case statement to parse all opts in both of these scripts should be used if possible; delaying any necessary actions until the command has been determined. If each of the add/update/etc. ops called a "setup_gpg" method first or something that did the config file parsing and ensuring the GPG directory exists, that would be best.

Alright, I thought a special casing wouldnt be the best solution here. I'll look into repo-add script too and try to do as you suggested.

...

...
+ if [[ $1 != "--version" && $1 != "-V" && $1 != "--help" && $1 != "-h" && $1 != "" ]]; then if type -p gpg >/dev/null 2>&1 = 1; then error "$(gettext "gnupg does not seem to be installed.")" @@ -316,8 +324,6 @@ case "${command}" in ${GPG_PACMAN} "$@" || ret=$? exit $ret ;; - -h|--help) - usage; exit 0 ;; -V|--version) version; exit 0 ;; *)

4788

Age (days ago)

4789

Last active (days ago)

List overview

Download

8 comments

4 participants

participants (4)

Dan McGee
Ivan c00kiemon5ter Kanak
Ray Kohler
Rémy Oudompheng