optional skip of postgres user check

[rvalyi]

Hello,

with the new Docker rage, it's now not always an issue to connect to postgres with the postgres user. Official PG image provide a postgres user and that's not a problem if the PG server is containerized.

So I would like to optionally be able to skip the PG user check here:
https://github.com/anybox/anybox.recipe.odoo/blob/master/anybox/recipe/odoo/runtime/session.py#L165

(same goes for odoo codebase but let's say it's easier to patch)

Would it be acceptable that if the SKIP_PG_USER environment variable is set we skip this test? If yes I'll do a PR now...

[gracinet]

Hi Raphaël,

no problem in principle, but don't you think the check should be removed
altogether ? It is not a very thorough one anyway, being just based on
the name (although it's harder to bypass than it looks because Odoo
shadows the PGUSER environment variable by refusing db_user not to be set).

My feeling is that one should not make assumptions on the user context.
Same goes for running as root (even though it's still a bad idea in a
Docker container, it might be acceptable in other cases, who am I to know).

Just being curious, this means you are installing inside a PG image and
not linking to it, right ?

On 10/12/2015 08:39 PM, Raphaël Valyi wrote:

Hello,

with the new Docker rage, it's now not always an issue to connect to
postgres with the postgres user. Official PG image provide a postgres
user and that's not a problem if the PG server is containerized.

So I would like to optionally be able to skip the PG user check here:
https://github.com/anybox/anybox.recipe.odoo/blob/master/anybox/recipe/odoo/runtime/session.py#L165

(same goes for odoo codebase but let's say it's easier to patch)

Would it be acceptable that if the SKIP_PG_USER environment variable
is set we skip this test? If yes I'll do a PR now...

—
Reply to this email directly or view it on GitHub
#63.

Georges Racinet
Anybox SAS, http://anybox.fr
Téléphone: +33 6 51 32 07 27
GPG: 0x33AB0A35, sur serveurs publics

[rvalyi]

Hello @gracinet
so if you look the official postgres images they have postgres as their only default user:
https://github.com/docker-library/postgres/blob/master/docker-entrypoint.sh

Yes this is the admin user, but if you use it to try to attack the PG server you will only attack your own PG server container, so in this usage context this is acceptable I think.

It's the context where I do link to a postgres container, the default pg user is still called postgres here...

As for the Odoo unix user, I agree, root should never be allowed, not even inside Docker. I'm only talking about the PG user here.

You say "make no assumption about the user context". Well if the user could alter the process env var of Odoo, certainly he could attack many well known 12 factors apps as well. I don't see anything special in Odoo that would expose it more to such attack than other apps. If Odoo was 12 factors compliant, if the user could craft the env variables he could change the admin password and download any database. So I don't see it as a special risk.

IMHO these checks are more some basic hint for the noob developer trying to install Odoo without paying attention to basic security.

So what I propose is an opt-in to skip this check.

But do you prefer that opt-in to be passed via the config file? Is it possible to pass any arbitrary param? personally prefer the ENV var, but that's up to you.

Thoughts?

[gracinet]

Hi Rapahël, don't worry, I'm approving what you suggest. I'm just
saying we could even go farther and remove these checks completetly, as
they make litlle sense to me. If people want to run Odoo as root, well
that's their right isn't it ?

« IMHO these checks are more some basic hint for the noob developer
trying to install Odoo without paying attention to basic security. » ->
I agree, although a newbie would really need already to make special
efforts to be in that situation.

Anyway, curious what the others would say, and I'm off for tonight
(almost 3AM here)
Cheers,

On 10/13/2015 02:42 AM, Raphaël Valyi wrote:

Hello @gracinet https://github.com/gracinet
so f you look the official postgres images they have postgres as their
only default user:
https://github.com/docker-library/postgres/blob/master/docker-entrypoint.sh

Yes this is the admin user, but if you use it to try to attack the PG
server you will only attack your own PG server container, so in this
usage context this is acceptable I think.

It's the context where I do link to a postgres container, the default
pg user is still called postgres here...

As for the Odoo unix user, I agree, root should never be allowed, not
even inside Docker. I'm only talking about the PG user here.

Yes but, in one of those hypervisors running only one process ?

You say "make no assumption about the user context". Well if the user
could alter the process env var of Odoo, certainly he could attack
many well known 12 factors apps as well. I don't see anything special
in Odoo that would expose it more to such attack than other apps. If
Odoo was 12 factors compliant, if the user could craft the env
variables he could change the admin password and download any
database. So I don't see it as a special risk.

IMHO these checks are more some basic hint for the noob developer
trying to install Odoo without paying attention to basic security.

So what I propose is an opt-in to skip this check.

But do you prefer that opt-in to be passed via the config file? Is it
possible to pass any arbitrary param?

Yes it is

I personally prefer the ENV var, but that's up to you.

no wonder, in Docker context that's handy.

Thoughts?

—
Reply to this email directly or view it on GitHub
#63 (comment).

Georges Racinet
Anybox SAS, http://anybox.fr
Téléphone: +33 6 51 32 07 27
GPG: 0x33AB0A35, sur serveurs publics

[rvalyi]

Here it is: #64

Well, I think it's acceptable to keep the test by default and skip it only as an opt-in the developer should be aware about. It's not documented, but if the dev tries the postgres user, it will break at the check and he will find out the feature, so I propose no leaky abstraction here.

[sbidoul]

No objection... although I personally prefer deploying the good old way (deb+virtualenv) with SaltStack in an LXC container, so I'm not affected by this issue.

[rvalyi]

so I made the change I explained previously and documented.