postgresql icon indicating copy to clipboard operation
postgresql copied to clipboard

Become root when installing dependencies

Open Ichimonji10 opened this issue 6 years ago • 15 comments

Ichimonji10 avatar Aug 03 '18 21:08 Ichimonji10

as far as I know this whole role is run as root first then changes to the pgsql user when needed then returns as root so this is not needed.

aoyawale avatar Aug 04 '18 17:08 aoyawale

The entire ANXS.postgresql role is supposed to be run as root? If so, isn't that a design bug? Or do you mean something else?

FWIW, this PR was created because I ran ANXS.postgresql against a remote host, where the user used for SSH connections was non-root and had sudo privileges.

Ichimonji10 avatar Aug 06 '18 13:08 Ichimonji10

I think being able to run as non-root would be an improvement. It would be good if in places where the playbook needs root, it would specifically becomes it there. Not having to specify to run the entire role as root is one less requirement on users. This is backwards compatible too.

I didn't see it specified in the docs that it needs root or not. Either way, that might also be helpful too.

bmbouter avatar Aug 06 '18 18:08 bmbouter

yes, it does. Your doing a lot of system level changes. This means either ssh in as root or as another user that can escalate privileges. So if you look at the tasks, you're installing packages, changing systemd, init files etc. Adding become root/sudo in every tasks is a waste of time and unnecessary. The parts that are not run as root/sudo are the postgres parts where it becomes pgsql to handle pgsql tasks since you can't be root/sudo on postgres. This is also how ansible works.

aoyawale avatar Aug 07 '18 13:08 aoyawale

you don't need to specify anywhere that you need root. You should know what you need by reading the ansible documents and decide if you need to escalate privileges or not. So if you are running this from ansible core or awx, you can easily escalate your privileges when you run the playbook. There is no need to do it on every tasks. When it comes to this PR this is not needed.

aoyawale avatar Aug 07 '18 13:08 aoyawale

@jlozadad I hear you; all of your environments work that way so I can see how this isn't useful to you. I'm a user of this role, and I see value in it.

In terms of documenting the root requirement, I agree the Ansible docs do guide the role author to make a choice for the users if it should be run as root. What I'm confused about is how users could know what choice you made without it being documented.

The PR is posted so if you accept it that would be cool. If you don't accept it that is your choice. Thanks for the role either way. It's really helpful.

bmbouter avatar Aug 07 '18 14:08 bmbouter

Adding become root/sudo in every tasks is a waste of time and unnecessary.

This role already specifies become in a number of places. If "adding become root/sudo in every tasks is a waste of time and unnecessary," it would be good to drop these directives. Doing so would let the code base be self-consistent, with the possible side-effects of improving user-friendliness and squashing some latent bugs.

$ git grep become:
tasks/configure.yml:  become: yes
tasks/configure.yml:  become: yes
tasks/configure.yml:  become: yes
tasks/configure.yml:  become: yes
tasks/configure.yml:  become: yes
tasks/configure.yml:  become: true
tasks/databases.yml:  become: yes
tasks/databases.yml:  become: yes
tasks/databases.yml:  become: yes
tasks/databases.yml:  become: yes
tasks/databases.yml:  become: yes
tasks/databases.yml:  become: yes
tasks/databases.yml:  become: yes
tasks/databases.yml:  become: yes
tasks/databases.yml:  become: yes
tasks/install_dnf.yml:    become: yes
tasks/install_yum.yml:    become: yes
tasks/schemas.yml:  become: yes
tasks/users.yml:  become: yes
tasks/users_privileges.yml:  become: yes
tests/docker/site.yml:  become: false
tests/docker/site.yml:  become: false
tests/playbook.yml:  become: yes

That said, I disagree with the notion that explicitly escalating privileges on specific tasks is a harmful design pattern. There are two benefits to escalating privileges only when needed:

  • The role better follows the principle of least privilege (PoLP).
  • The role is more user-friendly. Users who default to setting become: true won't be affected. And users who leave become at its default of false (and follow the PoLP) will be able to use this role without stumbling over this roadblock.

Ichimonji10 avatar Aug 07 '18 14:08 Ichimonji10

Hi @Ichimonji10 , I notice this has stalled... and I'm wondering if there's a fault, or this is just making things safer?

I've always run the role as a standard user which has "sudo" privs, and it seems to work OK.

gclough avatar Mar 08 '19 21:03 gclough

My goal in filing this issue is to make this role safer. Better to fix an issue before it bites someone than afterwards.

I don't recall whether I was able to run this role as a user who merely had sudo privileges. It's been quite some time since I've had to use this role, as I no longer work on a project which uses this.

Ichimonji10 avatar Mar 11 '19 23:03 Ichimonji10

This seems like a fine change to me I agree with the sentiments put forward by @Ichimonji10 and @bmbouter

nchudleigh avatar Mar 14 '19 20:03 nchudleigh

The whole role is meant to be run as non root, that is why there are "become" statemens in other tasks.

In my view, the lack of thereof (hence this pull request) is a bug. Most likely this has not been addressed before as other users that need "yum" probably ran the whole role as root.

In my view, we should merge this into master.

maglub avatar Apr 18 '19 20:04 maglub

Hi @Ichimonji10 , I notice this has stalled... and I'm wondering if there's a fault, or this is just making things safer?

I've always run the role as a standard user which has "sudo" privs, and it seems to work OK.

Did you run it on Fedora or CentOs? If you, as I, are on Ubuntu or Debian, you would not be affected by the tasks in this pull request.

maglub avatar Apr 18 '19 20:04 maglub

Did you run it on Fedora or CentOs? If you, as I, are on Ubuntu or Debian, you would not be affected by the tasks in this pull request.

At the time I wrote this pull request, I would have tested it against Fedora and/or RHEL 7.

Ichimonji10 avatar Apr 19 '19 17:04 Ichimonji10

Sorry Ichimonji10, my question was actually directed to @gclough. I suspect that he has not ran into your elevated rights issue that you are correcting with this pull request, as he might not have been running on Fedora and/or RHEL.

maglub avatar Apr 21 '19 10:04 maglub

Is this related to issue #434

gclough avatar May 20 '19 15:05 gclough