Re: [PATCH] issue #4375: provide --password-fd option

El 17/09/17 a las 02:08, Stefan Sperling escribió:
> On Sun, Sep 17, 2017 at 01:06:14AM -0700, William Orr wrote:
>> Hey,
>>
>> This is my first patch to subversion, so please bear with me.
>>
>> This looks to address a very commonly requested feature: providing an
>> alternative for automated tools to provide a password to svn via piping
>> it in over an fd (similar to gnupg).
>
> Hi William,
>
> This looks good to me in principle and I support the idea.
>
> Will the implementation of read_pass_from_fd() compile and work on Win32?
> We usually defer such operations to APR to avoid portability concerns.
> However, APR does not seem to offer an API which wraps fdopen().
> Whenever we bypass APR and use system APIs directly we need to support
> at least Unix-like systems and Win32.
>
> We cannot change the parameter list of svn_cmdline_create_auth_baton2().
> The function is already part of a release, so changing it breaks our
> ABI compatibility promise. Instead, we can add a new function called
> svn_cmdline_create_auth_baton3() which has the additional parameter.
> The svn_cmdline_create_auth_baton2() interface can be implemented
> as a wrapper around our new version of this function.
>
> Given that this is feature intends to support non-interactive usage,
> I wonder if it should depend on the --non-interactive option as well?
> And maybe we could then reduce the new option to --password-from-stdin?
>
> Some tools already use stdin for a different purpose, though.
> E.g. 'svnrdump load' reads a dump file from stdin. But if we also extended
> such tools with an option to read their normal input from a file, we could
> make this idea work. "svnadmin load" already supports the -F (--file)
> option for this purpose. Scripts would have to pass the right combination
> of options: --non-interactive --password-from-stdin -F /tmp/my-dump-file
> If --password-from-stdin is used without --non-interactive and without
> -F then the program should error out and complain.
>
> This idea also solves the portability question, since svn_stream_for_stdin2()
> or apr_file_open_flags_stdin() could be used to read a password from stdin.
> And we wouldn't need a new revision of svn_cmdline_create_auth_baton2()
> either because the client could pass the password as a string, as it
> does for the --password option.
>
>> One outstanding concern that I couldn't find addressed is clearing out
>> memory that once contained passwords (like with memset_s or
>> explicit_bzero). If I missed a technique for doing this that exists in
>> svn already, please let me know so I can update the diff.
>
> I don't think we have an API for that either, unfortunately. However,
> the same portability concerns apply. In any case, it would be great to
> have such an API available in APR.
>
> Regards,
> Stefan
>
>> Tested on Fedora 25 x86_64 and OpenBSD 6.1 x86_64.
>>
>> Please CC me; I'm not on this list.
>>
>> [[[
>> Introduce global opt --password-fd to allow applications to provide a
>> password over an already-opened file descriptor.
>>
>> * subversion/include/svn_cmdline.h
>> (svn_cmdline_create_auth_baton2): Add `auth_password_fd` argument
>> * subversion/include/svn_error_codes.h
>> (SVN_ERR_IO_PIPE_READ_ERROR): Undeprecate, as now used
>> * subversion/libsvn_subr/cmdline.c
>> (read_pass_from_fd): Add static function to get password from a file
>> descriptor
>> (svn_cmdline_create_auth_baton2): Add `auth_password_fd` arg and
>> trigger read of fd if this arg is not -1
>> * subversion/libsvn_subr/deprecated.c:
>> (svn_cmdline_create_auth_baton): Add default val of -1 when calling
>> `svn_cmdline_create_auth_baton2`
>> * subversion/svn/svn.c
>> (svn_cl__longopt_t): Add `opt_auth_password_fd` longopt
>> (svn_cl__global_options): Add `opt_auth_password_fd` to global options
>> (sub_main): Process global option `opt_auth_password_fd` and pass it
>> to `svn_cmdline_create_auth_baton2`
>> * subversion/svnmucc/svnmucc.c
>> (sub_main): Process global option `opt_auth_password_fd` and pass it
>> to `svn_cmdline_create_auth_baton2`
>> * subversion/svnrdump/svnrdump.c
>> (svn_svnrdump__longopt_t): add `opt_auth_password_fd`
>> (svnrdump__options): add help message for `--password-fd`
>> (init_client_context): Pass `auth_password_fd` to
>> `svn_cmdline_create_auth_baton2`
>> (sub_main): Process global option `opt_auth_password_fd` and pass it
>> to `init_client_context`
>> * subversion/svnsync/svnsync.c
>> (svnsync__opt): Add `svnsync_opt_source_password_fd` and
>> `svnsync_opt_sync_password_fd`
>> (svnsync_options): Add help messages for `--source-password-fd` and
>> `--sync-password-fd`
>> (opt_baton_t): Add `source_password_fd` and `sync_password_fd`
>> (sub_main): Process global option `--source-password-fd` and
>> `--sync-password-fd` and pass it to `svn_cmdline_create_auth_baton2`
>> invocations
>> * subversion/tests/cmdline/atomic-ra-revprop-change.c
>> (construct_auth_baton): Pass -1 as the `auth_password_fd`
>> * subversion/tests/cmdline/getopt_tests_data/svn_help_log_switch_stdout
>> (): Add new `--password-fd` option to expected output
>> * subversion/tests/libsvn_ra/ra-test.c
>> (check_tunnel_callback_test): Pass -1 as the `auth_password_fd`
>> (tunnel_callback_test): Pass -1 as the `auth_password_fd`
>> (tunnel_run_checkout): Pass -1 as the `auth_password_fd`
>> * subversion/tests/svn_test_main.c
>> (svn_test__init_auth_baton): Pass -1 as the `auth_password_fd`
>> * tools/client-side/svn-mergeinfo-normalizer/mergeinfo-normalizer.h
>> (svn_min__opt_state_t): Add `auth_password_fd`
>> * tools/client-side/svn-mergeinfo-normalizer/svn-mergeinfo-normalizer.c
>> (svn_min__longopt_t) Add `opt_auth_password_fd`
>> (sub_main) Process global option `--password-fd` and pass it to
>> `svn_cmdline_create_auth_baton2` invocations
>> * tools/client-side/svnconflict/svnconflict.c
>> (svnconflict_opt_state_t): Add `auth_password_fd`
>> (svnconflict_options): Add `--password-fd` documentation
>> (svnconflict_global_options): Add `opt_auth_password_fd`
>> (sub_main): Process global option `--password-fd` and pass it to
>> `svn_cmdline_create_auth_baton2` invocations
>> * tools/dev/svnmover/svnmover.c
>> (sub_main): Process global option `--password-fd` and pass it to
>> `svn_cmdline_create_auth_baton2` invocations
>> ]]]
>

Hey,

Thanks for the feedback. I've finally gotten the chance to update the
patch with the suggestions. Lemme know if this addresses your concerns,
and I can write up the changelog for it.

Thanks,
William Orr