Cannot desactivate FTP on DMS !

[Joël Grand-Guillaume @ camptocamp]

Hi !

Using the last version of OpenERP we are not able to desactivate the FTP service running by default on 8021 !

I think you can answer me it's a wishlist, but seriously... Can we accept this for an ERP ?! Can we ?

I suggest here just an option on server conf file to allow OpenERP stop his ftp services... It's too dangerous.

My test case :

Standard OpenERP, with document and document_sftp installed.

Regards,

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

Little comment more, if we can't desactivate it, we should at least :

1. Be able to specify the host through config (like config.get('ftp_host', 'localhost'))

2. The IP detection don't seems to work (function detect_ip_addr)

3. Of course, specifying the host into option should overwrite the ip-detection which is not the case

Ragards,

[Xavier (Open ERP) (xmo-deactivatedaccount)]

last version = 5.0.7 or trunk?

(in any case, not really a fix, but I think by default the ftp service only listens on localhost, which should only be accessible from the server machine shouldn't it?)
(and in trunk, document_ftp has been spun off from document itself, so you can install document without having the ftp server)

[xrg (xrg)]

On Monday 01 February 2010, you wrote:
> Public bug reported:

> Using the last version of OpenERP we are not able to desactivate the FTP
> service running by default on 8021 !
>

I had always made a serious effort to disable it in my tree.

Now, in 5.2 it will hopefully be configurable.

[Olivier Dony (Odoo) (odo-openerp)]

On 01/02/10 16:13, Xavier (Open ERP) wrote:
> (in any case, not really a fix, but I think by default the ftp service only listens on localhost, which should only be accessible from the server machine shouldn't it?)

I beg to differ, based on my experience I think you cannot reliably
guess on which interface (loopback or ethernet link) the server will
listen by default.
Might be worth checking the python module for this, but this may prove
to be partially system-dependent...

However you can determine it easily by watching the server log, since
the FTP service advertises the IP when it starts.

> (and in trunk, document_ftp has been spun off from document itself, so you can install document without having the ftp server)

And if you're on stable, no need to remind you that running a
correctly-configured firewall is always a good practice, especially on a
production system ;-)

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

Hi,

I'm using the last 5.0.7, and I agree with guys, I can put a firewall and/or patch the code to have the FTP server listening only on localhost !

But, that's not the point I think...

Why should I patch the server in order to have it listening on localhost ?

Why should I patch the code to desactivate the FTP service (using sftp for example)

Have a look on the code and tell me if you think it's good enough for a stable, professional and powerful ERP !?

I think it's not, and it'll may be take a few hours to have something good...

Regards,

Joël

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

After 5 minutes work and tests, I suggest a patch for at least :

- Serving on localhost by default (Why the hell this is not like this !!!)
- Allowing to add the parameter in the config file

The patch is attached.

Even suggesting this solution, I would really appreciate that we can desactivate this service... But I will understand that this is a new feature in a way.

I just want to say that in my own opinion, this little patch is really important as the FTP server is listening from anywhere by default (not just localhost). I don't think (or at least hope) that you let this like this on your Odoo offer don't you !?

So why you don't fix it for everybody ?

Thanks in advance to consider this fix,

Regards,

[Greg (greg-apieum)]

Hi,
I've linked two branches to fix it.
To desactivate FTP you've just to set the parameter ftp_server_host to none.
Same on sftp with sftp_server_host=none.

Maybe this solution isn't good enough ?

Has someone a suggest to improve it ?

Thanks.

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

Hi !

Thanks for your work !

Did you include my fix as well (serve localhost by default + add the parameter in the config file) ?

Deactivating it by putting hostname too none is good for me if it's clearly explained.

Regards,

[Greg (greg-apieum)]

Hi,
Sorry, there were errors on last push.

SFTP was serving to all adresses if it was not set in config,
and I've defined 'ftp_server_address' and not 'ftp_server_host' as config parameter. ^^

I push new commits but keep "detect_ip_addr() from tools.misc"
to set the host to serve if ftp_server_host or sftp_server_host are not set in config file.
Here the code:
<code>
from tools.misc import detect_ip_addr
if detect_ip_addr:
    HOST = config.get('ftp_server_host', detect_ip_addr())
else:
    HOST = config.get('ftp_server_host', '127.0.0.1')
</code>

I hope that's good for you, but take care maybe I don't use the same revision as your patch.

I'm a newbie in python and new on the community, feel free to tell me if there is something wrong.
Regards

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

Hi,

Thanks for your work. It sounds good to me. I suggest to integrate your branch in the trunk official branch AND apply my patch to the current stable version.

I mean, it's important to have this in both version, it's too dangerous to let this in stable.

Thanks in advance for the OpenERP team to do it !

Regards,

[Xavier (Open ERP) (xmo-deactivatedaccount)]

Sorry about the delay, I kind-of forgot to recheck this bug.

Greg > I checked your branch, it looks good as far as the fix goes but there are issues with a bunch of reformatting mixed with the fix, could it be possible to correct that and resubmit?

Joël > I'll check with Stéphane whether I should merge Greg's branch in both -stable and -trunk (once it's fixed) or just apply a basic patch on -stable

[Xavier (Open ERP) (xmo-deactivatedaccount)]

* but it will only be for 5.0.8 (damn you tab key)

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

Hi Xavier,

First, thanks for review ! This is ok for me to release for 5.0.8 as long as the delay is 1st april :) !

Regards,

Joël

[Xavier (Open ERP) (xmo-deactivatedaccount)]

> This is ok for me to release for 5.0.8 as long as the delay is 1st april :) !

The delay was for my response to the thread (I left 10 days pass by between your last comment and mine, which isn't very good). As far as I know, 5.0.8 is still planned for April 1st.

[Greg (greg-apieum)]

Hi,
I've just push the extra-trunk branch that fix this bug for SFTP server, but as you had to do a revert for trunk due to others changes, I won't suggest a merge, just give you a diff.

My branch allow to set sftp authentication by password or by key (rev 4333), if you want it, I'll suggest a merge.

Is it good for you ?

[Olivier Dony (Odoo) (odo-openerp)]

Summary so far:

1. For stable, I have applied a modified version of Joël's patch (rev 2701 <email address hidden>)
@Joël: as you guessed, we can't put the default as 127.0.0.1 in 5.0 because it could potentially break many local installations where they rely on the fact that 0.0.0.0 is used when the detection fails. But at least now you can override the default in the configuration in all case.
This bug was in fact indeed a wishlist but given that auto-detection is unreliable let's consider it a valid bug in stable with a workaround for bad auto-detection.

2. For trunk it's already merged by Xavier, and I'm all ok for the more restrictive default of 127.0.0.1.

3. For document_sftp, as it's in extra-addons, any member of ~openerp-commiter can do it. => Joël ?
Greg's patch looks okay. And his proposal of allowing both key and password auth for sftp sounds good too (but haven't looked at that code in his branch).

Speaking of code, Greg and Xavier I have a question about what you merged in trunk (rev 3058), specifically this test at document_ftp/ftpserver/__init__.py:62:

   if HOST.lower() == 'none':

Hrm ... what exactly are you trying to achieve?
Do you plan to allow setting the host to the string None to disable the ftp? Or is this a (very wrong) way of testing for None?
In any case this doesn't look right, if only because 'none' could very well be a valid DNS alias for a local IP (people have strange nicknames for their computers sometimes)

Thanks everyone for your time and contribution!

[Olivier Dony (Odoo) (odo-openerp)]

Oh, and I also changed the severity, please keep in mind that you should not use the severity to reflect your personal wishlists ;-)

We have tried during the community days (merge prop workshop) to write explicit rules for deciding about the severity of a bug. We will put these in the new contribution guidelines (see preliminary version in Report/Classification at http://piratepad.net/communitydays-merges)

Based on these guidelines I would say the auto-detection problem is a Medium/Low bug, and the rest is Wishlist, hence I set it as Medium ;-)

[Greg (greg-apieum)]

Hi,
Thanks Olivier for this summary.

>Speaking of code, Greg and Xavier I have a question about what you merged in trunk (rev 3058), specifically this test at >document_ftp/ftpserver/__init__.py:62:
>
> if HOST.lower() == 'none':
>
>Hrm ... what exactly are you trying to achieve?

If you set ftp_server_host to "none" (HOST var) in the config file, FTP Server don't start.

> Cannot desactivate FTP on DMS !
I agree that the solution isn't the best one, but now you can desactivate FTP server. ;-)

>In any case this doesn't look right, if only because 'none' could very well be a valid DNS alias for a local IP (people have >strange nicknames for their computers sometimes)

I've a database with more than 2 millions hosts names (60% from US, 20% from asia, 20% from europa) and never found an host called "none".
Even if someone choose this strange name, he can set the IP adress directly.

Finally, I needed to desactivate FTP on mine servers, so I've done this job few months ago (from 5.0.1 server version) and when I saw Xavier patch looks like mine (I had exactly the same code), I just decided to push what I use even if I know that's not what we can expect for a professionnal ERP.

Sorry I found this solution (setting host to none) simple and functional, but I'm weird about your proposals. :-)
why not an other settings like disable_ftp_server ? - if you think that really make a difference -

have a nice day.

PS:Thanks for guidelines (I'm not concerned but it's interesting).

[Olivier Dony (Odoo) (odo-openerp)]

On 21/04/10 09:38, Greg wrote:
> I've a database with more than 2 millions hosts names (60% from US, 20% from asia, 20% from europa) and never found an host called "none".
> Even if someone choose this strange name, he can set the IP adress directly.

Yes, but if possible I'd like making the parameters to OpenERP as easy
to use as possible, and this mechanism seems a bit complicated to
understand for a newcomer (and how is the user supposed to guess that
'none' is not a valid ftp_serve_host for an obscure reason.)

> Sorry I found this solution (setting host to none) simple and functional, but I'm weird about your proposals. :-)
> why not an other settings like disable_ftp_server ? - if you think that really make a difference -

Yes I would prefer a separate parameter, unless we change the whole
configuration to support None *and* False values for all parameters,
which appropriate semantics, in which case it will become obvious to
everyone that using 'None' as a hostname is not allowed, as for any
other config option, unless you want to disable it. (and then lowercase
'none' would not make sense)

But the first question to ask is: now that in trunk you can install
document_ftp separately from document, why would you need to disable the
FTP server?
If you don't need it, don't install it, or uninstall it.

What do you think?

[Greg (greg-apieum)]

I use SFTP server only.

[Greg (greg-apieum)]

oups sorry,

SFTP server depends on module document ( with FTP server ).

>now that in trunk you can install document_ftp separately from document

Sounds Good, but I have not tried.
I use 5.03, 5.06 and 5.07 versions of server and do not play to make upgrade every days (for free) with the risk to loose customers datas.

As I said, I saw the opened bug on launchpad and just push the work I had already done, but why opening a critical bug if we don't require module document to have SFTP ? it's just a whish.

There is few case (I know) where you need to desactivate FTP server temporarely.
One is when an enterprise wish to serve documents only when its offices are opened, another is when it close a fiscal year and have to give all documents to an external service. (Cases I've already seen)
You can effectivly uninstall module but it's easier and more secure to be able to desactivate it with config.

If you approve the change to parameter disable_ftp_server
I can take the action.
Do you need a test on differents values like : true, 1, on (case insensible) ?

[Olivier Dony (Odoo) (odo-openerp)]

On 22/04/10 07:54, Greg wrote:
> I use 5.03, 5.06 and 5.07 versions of server and do not play to
> make upgrade every days (for free) with the risk to loose
> customers datas.

Sure, and we only accept bugfixes in stable, features/enhancements are
for trunk, for this very reason.

> As I said, I saw the opened bug on launchpad and just push the work I
> had already done, but why opening a critical bug if we don't require
> module document to have SFTP ? it's just a whish.

Exactly, the critical status was put by the original reporter. That's
why I changed it to medium, even then only because the IP detection did
not work correctly so in some cases you had no way to make the FTP
server accessible from the network (which is a bug).

The rest is indeed wishlist.

> You can effectivly uninstall module but it's easier and more secure
> to be able to desactivate it with config.

That's debatable, but ok, I would still vote for a clean option to
disable it temporarily (if it's not temporary, just uninstall it)

> If you approve the change to parameter disable_ftp_server
> I can take the action.
> Do you need a test on differents values like : true, 1, on (case insensible) ?

Sure, in that case please submit a merge proposal on
openobject-addons/trunk, and introduce an 'ftp_server_disabled' option.
For the test please use regular Python boolean testing, so something
like the following to support 1/True/true/on/whatever:
    if HOST and not config.get('ftp_server_disabled'):
       # start the server....

Thanks a lot!