Bug 3974 - smbclient tar creating option arguments are confused with password provided in command line
Summary: smbclient tar creating option arguments are confused with password provided i...
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.0
Classification: Unclassified
Component: smbclient (show other bugs)
Version: 3.0.23c
Hardware: All All
: P3 normal
Target Milestone: none
Assignee: Simo Sorce
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-07-28 06:47 UTC by Tomasz Ostrowski
Modified: 2007-03-28 09:13 UTC (History)
0 users

See Also:


Attachments
try to fix clash between tar option parsing and reading service/password from the command line (2.44 KB, patch)
2007-03-26 15:11 UTC, Simo Sorce
no flags Details
samba3_smbclient_tar_opt.patch (3.45 KB, patch)
2007-03-27 08:13 UTC, Simo Sorce
no flags Details
samba3_smbclient_tar_opt.patch (3) (3.07 KB, patch)
2007-03-27 10:11 UTC, Simo Sorce
no flags Details
samba3_smbclient_tar_opt.patch (4) (3.92 KB, patch)
2007-03-27 16:46 UTC, Simo Sorce
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tomasz Ostrowski 2006-07-28 06:47:08 UTC
Tar creating option arguments of smbclient are confused with password provided in command line, for example:
    smbclient //server/share -U user -Tqc filename.tar
Instead of prompting for password will try to connect using password "filename.tar" and also create file "filename.tar".

This is caused by trying to hide options parsed by function tar_parseargs() from popt using poptGetOptArg(pc) several times (line 3506 in source/client/client.c and line 3643 in source/client/smbctool.c). This can not work - popt does not support multiple arguments for options - and this calls are ignored.

This is also why a proposed patch for bug 2051 breaks tar file creation.
Comment 1 Simo Sorce 2007-03-26 15:11:06 UTC
Created attachment 2346 [details]
try to fix clash between tar option parsing and reading service/password from the command line

Can you try this patch?

This should solve the problem with the brain damaged way tar options are parsed.
Comment 2 Tomasz Ostrowski 2007-03-27 07:38:14 UTC
(In reply to comment #1)

> Can you try this patch?

This patch breaks basic functionality, like:
  smbclient //omega/instale
which does show "usage" and does not connect to a server:
Usage: [-?] [-?EgV] [-?EgV] [-?EgVNkP] [-?|--help] [--usage] [-R|--name-resolve NAME-RESOLVE-ORDER]
        [-M|--message HOST] [-I|--ip-address IP] [-E|--stderr] [-L|--list HOST]
        [-t|--terminal CODE] [-m|--max-protocol LEVEL] [-T|--tar <c|x>IXFqgbNan]
        [-D|--directory DIR] [-c|--command STRING] [-b|--send-buffer BYTES]
        [-p|--port PORT] [-g|--grepable] [-d|--debuglevel DEBUGLEVEL]
        [-s|--configfile CONFIGFILE] [-l|--log-basename LOGFILEBASE]
        [-V|--version] [-O|--socket-options SOCKETOPTIONS]
        [-n|--netbiosname NETBIOSNAME] [-W|--workgroup WORKGROUP]
        [-i|--scope SCOPE] [-U|--user USERNAME] [-N|--no-pass] [-k|--kerberos]
        [-A|--authentication-file FILE] [-S|--signing on|off|required]
        [-P|--machine-pass] service <password>

"-T" option can have any number of arguments which just mean which files/directories are to be archived, like:
   smbclient //server/share -T filename1 filename2 filename3
As this file names can start with "-" you can not tell them apart from options. The only useful "-T" is the last option - I think anything after "-T" should be treated as an option for "-T".
Comment 3 Simo Sorce 2007-03-27 07:41:47 UTC
> "-T" option can have any number of arguments which just mean which
> files/directories are to be archived, like:
>    smbclient //server/share -T filename1 filename2 filename3
> As this file names can start with "-" you can not tell them apart from options.
> The only useful "-T" is the last option - I think anything after "-T" should be
> treated as an option for "-T".

This is exactly what I am trying to do, now I just need to manage the case when no option at all is passed.

Comment 4 Tomasz Ostrowski 2007-03-27 07:47:57 UTC
I've tried several combinations and adding some options does not help - every time I get usage help screen:
smbclient //server/share -U username
smbclient //server/share -U username -d 1
smbclient //server/share -U username -d 1 -N
smbclient //server/share '' -U username
smbclient //server/share ''

I think this is not this one special case.
Comment 5 Simo Sorce 2007-03-27 08:13:05 UTC
Created attachment 2347 [details]
samba3_smbclient_tar_opt.patch

This one is uglier but make things work in case no options are provided, or options are passed before service password are specified.

This is the only form that actually gives a bad error:

$ ./bin/smbclient -Tc /tmp/backup.tar backup
Connection to  failed

but I thing this is better than the current smbclient behavior.
Comment 6 Simo Sorce 2007-03-27 08:15:52 UTC
(In reply to comment #4)
> I've tried several combinations and adding some options does not help - every
> time I get usage help screen:
> smbclient //server/share -U username
> smbclient //server/share -U username -d 1
> smbclient //server/share -U username -d 1 -N
> smbclient //server/share '' -U username
> smbclient //server/share ''
> 
> I think this is not this one special case.
> 

They worked for me even with the first patch, what system are you building these on?
I fear a different behavior of different popt implementations :-(
Comment 7 Tomasz Ostrowski 2007-03-27 09:24:55 UTC
My system is up-to-date Fedora Core Linux 5 with popt-1.10.2-15.2. I've just made a new, clear compile using first version of patch and just (./configure && make) and it is still showing usage on "smbclient //server/share".

The second patch version works better but I haven't tested it extensively yet. I'll test it more tomorrow.
Comment 8 Simo Sorce 2007-03-27 10:11:41 UTC
Created attachment 2348 [details]
samba3_smbclient_tar_opt.patch (3)

ok this patch is much more clear and should match 99% to current expected and defined smbclient behavior.

The only difference is that it forces the -T option to be the last one for sanity.
I think we can't do better then this and keep things sane.
Comment 9 Simo Sorce 2007-03-27 16:46:51 UTC
Created attachment 2349 [details]
samba3_smbclient_tar_opt.patch (4)

another more refined patch that is able to parse options after -T has been parsed as well.

the only problem with this is that the tar option parsing routines pick up also the following options as filenames to back up.

But that is another bug I will eventually fix later.
Comment 10 Tomasz Ostrowski 2007-03-28 05:55:02 UTC
(In reply to comment #9)
> Created an attachment (id=2349) [edit]
> samba3_smbclient_tar_opt.patch (4)

I've done some testing and it is much better than before. It is now possible to make smbclient do what it is supposed to.

There are 2 minor related problems, maybe worth looking at if we're here:

1. Bug 2051: smbclient -N doesn't work as documented (should be easy to correct)

2. "-T" accepts its options with or without space, but with a space it treats them both as a file to write to and as options, for example
  smbclient //server/share -A .auth.file -T cqX filename.tar excluded_file
will write to file "cqX" with no diagnostics and excluding "filename.tar" "excluded_file". I think this should be rejected for avoiding confusion.

But these are minor and can be avoided.

Thank you very much for your work here.
Comment 11 Simo Sorce 2007-03-28 09:13:50 UTC
The -N flag really means no password, changing this for just smbclient is not something that can be done easily, I will just update the docuemntation to reflect the fact that if you use -N then an anonymous connection will be attempted and any password will be silently discarded.

As said the -T option is parsed separately and I will try to address it as a separate bug, it has many things I don't like, not only it uses it's options as filename, but in a command line like this:
smbclient //srv/share -T /tmp/t.tar blah -d 5 -N
it takes up and try to use "-d" "5" and "-N" as files/dirs to back up as well :(

Thanks fro feedback, I am going to commit this patch and work on other issues later.