You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@subversion.apache.org by CoolCold <co...@gmail.com> on 2021/02/22 17:40:14 UTC
using svn cli with --non-interactive (in scripts) securely, without
exposing password
Good day!
(please CC me, I'm not subscribed to the list)
A bit of context:
I was using subversion to store my serves' configs versioned for
almost a decade, with bash wrapping around it. Simplified, it had repo
per server name, wrapper called by cron to checkout, rsync over,
commit changes back, sending email on diffs (
https://github.com/coolcold/svnbackup ). Had no issue with it, when
password store was enabled. It's runned under root user and saved
credentials are not exposed to non-admin users on the system.
Issue: with recent changes hitting packages in distributions (
https://marc.info/?l=subversion-commits&m=154101482302608&w=2 ), that
seems to be not possible anymore.
I did adjust my script to use command line switch --password, but this
makes it visible for anyone who does simple commands like ps aux.
I've tried to look around for possible support of environment
variables / password file support, but couldn't find any except some
old proposals like
http://subversion.1072662.n5.nabble.com/Feature-proposal-SVN-USERNAME-and-SVN-PASSWORD-environment-variables-td180031.html
Rebuilding subversion from source is not an option for many reasons.
Seeking for your help on this, what is the proper way of doing this
with recent versions?
Thanks in advance.
Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password
Posted by CoolCold <co...@gmail.com>.
Hello!
On Tue, Feb 23, 2021 at 2:37 AM Nathan Hartman <ha...@gmail.com> wrote:
>
> On Mon, Feb 22, 2021 at 1:17 PM CoolCold <co...@gmail.com> wrote:
> >
> > Good day!
> > (please CC me, I'm not subscribed to the list)
> >
> > A bit of context:
> > I was using subversion to store my serves' configs versioned for
> > almost a decade, with bash wrapping around it. Simplified, it had repo
> > per server name, wrapper called by cron to checkout, rsync over,
> > commit changes back, sending email on diffs (
> > https://github.com/coolcold/svnbackup ). Had no issue with it, when
> > password store was enabled. It's runned under root user and saved
> > credentials are not exposed to non-admin users on the system.
> >
> > Issue: with recent changes hitting packages in distributions (
> > https://marc.info/?l=subversion-commits&m=154101482302608&w=2 ), that
> > seems to be not possible anymore.
> > I did adjust my script to use command line switch --password, but this
> > makes it visible for anyone who does simple commands like ps aux.
> > I've tried to look around for possible support of environment
> > variables / password file support, but couldn't find any except some
> > old proposals like
> > http://subversion.1072662.n5.nabble.com/Feature-proposal-SVN-USERNAME-and-SVN-PASSWORD-environment-variables-td180031.html
> >
> > Rebuilding subversion from source is not an option for many reasons.
> >
> > Seeking for your help on this, what is the proper way of doing this
> > with recent versions?
> > Thanks in advance.
>
> Hello,
>
> Recent versions (1.12.x and newer [1]) by default don't _save_
> passwords to disk in plaintext (unless configured to do so at
> build-time).
>
> However, Subversion will _use_ the password, if it is already stored
> on disk.
>
> So, as a workaround, you could use some out-of-band method to save the
> password to disk either by using an older SVN client or by generating
> a file with the right bits in it:
That's good news for me, will poke around.
>
> In a recent discussion on our dev mailing list, there is an example
> shell script (for zsh) that saves a password file. See [2] and note
> that there were a few corrections to the script so be sure to use the
> latest version in that mail list thread.
>
> [1] https://subversion.apache.org/docs/release-notes/1.12.html#client-server-improvements
Thanks for the links with discussion and proposed solution, really helpful.
Also it expresses the same feeling and reaction I have, but stated in
much better English.
>
> [2] https://lists.apache.org/thread.html/r0eef40236aeddd1db18bc7882454dd3b18bcd721d8fd8c9e21aca52a%40%3Cdev.subversion.apache.org%3E
>
> I hope the above is helpful; feel free to ask as many questions as you
> need to, or propose improvements to the above-mentioned script or
> Subversion itself. We have gotten quite a few questions about this and
> it has been frustrating for anyone who uses svn as part of cron jobs
> in non-X environments, where the available encrypted password stores,
> Kwallet and Gnome-keyring, aren't much help, and GPG-Agent doesn't
> persist the passwords indefinitely.
> We would be really grateful if
> someone could propose a solution that works well in these scenarios
> while alleviating people's concerns about storing passwords on disk in
> plaintext.
As I see it, at the end of the day, cleartext password / token /
ssh-key would be saved anyway, if you need to have it to work in an
automated way.
Most convenient for me would be having:
a) --pasword-file=... command option
b) SVN_PASSWORD environment variable
both of them should not be hard to implement and both provide access
to current and/or root user, compared to current implementation, when
running "ps aux" to reveal --password=... param executed by any user
(this param could be at least googled fast and majority of people
won't go deep into crafting simple auth file themselves).
Live examples of cleartext passwords/tokens are numerous - SAMBA mount
passwords, AWS access tokens, deployment ssh keys and so on, which are
stored in cleartext anyway - I'd say industry made it's choice
already.
For advanced feature, it could be having something like
SVN_ASK_PASS_PROGRAM helper support, it can be sophisticated enough to
connect to some URL and get password from there, without storing it on
any local disk, but that's much harder to implement and setup by end
user. If such helpers would be bundled with subversion (like email
notification scripts in Perl, for example), there is a non-zero chance
someone will use it.
>
> Cheers,
> Nathan
Thanks again for your reply, Nathan!
--
Best regards,
[COOLCOLD-RIPN]
Re: A more permanent home for the add-a-password-to-a-cached-username script? (was: Re: using svn cli with --non-interactive (in scripts) securely, without exposing password)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Sahlberg wrote on Wed, 03 Mar 2021 07:32 +00:00:
> This brings up the question of translation. Futatuki translated the
> text to Japanese last time - or can someone else volonteer? There is
> also a Chinese translation, but it has not been updated for a long time
> - is there someone who would be interested in updating the text?
First, we shouldn't ask translators to translate moving targets. The
text in staging/ may still change before it's published.
As to translating the text once it's stable: we don't have a process
for this. I suppose we could just email the relevant translators and
advise them that faq.en.html has been updated and they may want to
update their translations, just like we might ask translators to update
subversion/po/ before a release — but we should keep in mind that some
translators haven't been active in years, and may or may not be
interested in contributing further.
Both translations have text near the top identifying the English
revision that was translated. There's some room for improvement there:
1. The Chinese translation needs to get the +840074 bump (per ^/subversion/README).
2. Both translations state the revision number that was translated, but
leave the reader to do some footwork in order to determine whether
the translation is up-to-date. They could, say, link to the ViewVC
log view of the English FAQ.
3. Linking to the log view assumes some understanding of Subversion, and
in context, that assumption may not be desirable. So, the English
FAQ could state a "Last modified" date, and the translations could
indicate the date of the English version that was translated.
Cheers,
Daniel
Re: A more permanent home for the add-a-password-to-a-cached-username
script? (was: Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password)
Posted by Daniel Sahlberg <da...@gmail.com>.
Den tis 2 mars 2021 kl 13:58 skrev Nathan Hartman <hartman.nathan@gmail.com
>:
> On Mon, Mar 1, 2021 at 2:25 AM Daniel Sahlberg <
> daniel.l.sahlberg@gmail.com> wrote:
>
>>
>> I think we should put it in the staging website for a final review? This
>> should go under General questions?
>>
>
>
> Yes, please commit it to staging for now. There is already a FAQ entry
> with ID plaintext-passwords; this is the update/replacement of that
> question. Keeping the ID the same means that any URLs out there will still
> work, and direct visitors to the updated answer.
>
Found some time this morning: r1887129. (I also fixed a few HTML validation
errors in r1887130).
This brings up the question of translation. Futatuki translated the text to
Japanese last time - or can someone else volonteer? There is also a Chinese
translation, but it has not been updated for a long time - is there someone
who would be interested in updating the text?
Kind regards,
Daniel Sahlberg
Re: A more permanent home for the add-a-password-to-a-cached-username
script? (was: Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password)
Posted by Nathan Hartman <ha...@gmail.com>.
On Mon, Mar 1, 2021 at 2:25 AM Daniel Sahlberg <da...@gmail.com>
wrote:
>
> I think we should put it in the staging website for a final review? This
> should go under General questions?
>
Yes, please commit it to staging for now. There is already a FAQ entry with
ID plaintext-passwords; this is the update/replacement of that question.
Keeping the ID the same means that any URLs out there will still work, and
direct visitors to the updated answer.
Thanks!
Nathan
Re: A more permanent home for the add-a-password-to-a-cached-username
script? (was: Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password)
Posted by Daniel Sahlberg <da...@gmail.com>.
Den mån 1 mars 2021 kl 02:47 skrev Nathan Hartman <hartman.nathan@gmail.com
>:
> On Sun, Feb 28, 2021 at 10:51 AM Daniel Sahlberg
> <da...@gmail.com> wrote:
> > When researching, I discovered that reading plain text passwords that
> are "grandfathered in" works the same way on Windows as on Unix. If the
> password is invalid it is switched to passtype==wincrypt when updated.
>
> Thanks for documenting that!
>
> > I have taken Nathan's suggestion and rearranged it slightly, trying to
> incorporate Danielsh' feedback. I have made notes as HTML comments: <!--
> ... --> - these should be removed from the final commit.
>
> Thanks also for improving the FAQ text. This is a big improvement over
> the text I proposed earlier. I only have a few minor nits to pick,
> mostly on things I wrote... I'll respond inline below, and below that
> for convenience I'll give the text with my proposed minor changes
> applied...
>
I think your suggestions make sense.
Only one small thing:
> > <p>On UNIX/Linux, Subversion supports up to four credential caches:</p>
> >
> > <ul>
> > <li>GNOME Keyring</li>
> > <li>KWallet</li>
> > <li>GPG-Agent</li>
> > <li>Plaintext cache in ~/.subversion</li>
>
> I think we should write "Plaintext cache in ~/.subversion/auth/".
> This is a preexisting issue from the original FAQ and I meant to
> change it before, but I forgot.
>
Maybe even ~/.subversion/auth/svn.simple/? Disclaimer: I don't fully
understand what the other directories contain but from a quick look at the
code it didn't seem to be passwords. Is svn.ssl.client-passphrase used to
store passphrases for SSL client certificates (in plaintext??) - then maybe
this should be considered but we don't discuss client certificates (this
was one of Danielsh's comments).
Here's the full text with my above suggestions applied... I didn't
> remove the HTML comments, and the two other things remaining to be
> done is the todo about GPG-Agent and to add the correct link to the
> Python script, if we've figured out where that should be.
>
> [[[
> <div class="h3" id="plaintext-passwords">
> <h3>How does Subversion cache credentials (plain text and encrypted)?
> <!-- The other FAQ entries are questions and there are discussions of
> encrypted password stores in addition to the plain text store -->
> <a class="sectionlink" href="#plaintext-passwords"
> title="Link to this section">¶</a>
> </h3>
>
> <p>To avoid having to type a password for each server operation, Subversion
> can cache credentials.</p>
>
> <p>Passwords may have been cached unencrypted by older versions of
> Subversion
> ("grandfathered in") and Subversion always supports reading these. Whether
> and
> how Subversion caches new credentials depends on several factors,
> including the
> access method, operating system, compile-time options, and settings in the
> client's run-time config file.</p>
> <!-- Moved the first sentence (grandfathered passwords) here from the Unix
> section. Added 'access methods' to somewhat cater for Danielsh's
> comment
> regarding SSH password prompts / keys / client certs. I'm leaving the
> question open if we should mention what is not covered in the answer.
> -->
>
> <!-- Removed the <p/> and <ul/> about what we try to answer -->
>
> <p>To show the credentials in your cache, use <tt>svn auth</tt>.
> Credentials
> are never removed automatically but may be removed manually using
> <tt>svn auth --remove</tt>.</p>
> <!-- Added info on how to review the cache and remove credentials. This
> should
> address part of Danielsh's #6 -->
>
> <h4>Windows</h4>
>
> <p>On Windows, Subversion uses standard Windows APIs to encrypt the data,
> so
> only the user can decrypt the cached password. <i>(Since Subversion
> 1.2.)</i></p>
>
> <h4>macOS (formerly Mac OS X)</h4>
>
> <p>On macOS, Subversion uses the system Keychain facility to encrypt/store
> the user's svn password. <i>(Since Subversion 1.4.)</i></p>
>
> <h4>UNIX/Linux</h4>
>
> <p>On UNIX/Linux, Subversion supports up to four credential caches:</p>
>
> <ul>
> <li>GNOME Keyring</li>
> <li>KWallet</li>
> <li>GPG-Agent</li>
> <li>Plaintext cache in ~/.subversion/auth/.
> </ul>
>
> <p>To determine which credential caches your Subversion client supports,
> run
> the <tt>svn --version</tt> command and look for "The following
> authentication
> credential caches are available" toward the end of its output.</p>
>
> <p>GNOME Keyring and KWallet both facilitate storing passwords on disk
> encrypted. For Subversion to support these programs (since Subversion 1.6),
> they need to be available at compile-time and at run-time.</p>
> <!-- Removing the sentence about fallback to plaintext - it is discussed
> after
> GPG-Agent -->
> <p class="todo">TODO: Discuss GPG-Agent.</p>
>
> <p>Depending on a compile-time option (--enable-plaintext-password-storage)
> and runtime configurations (see below) Subversion <i>may</i> fallback to
> storing
> passwords in the Plaintext cache.</p>
> <!-- From GNOME Keyring/KWallet merged with some of Danielsh's
> comments (1...6) -->
>
> <p>The default value of --enable-plaintext-password-storage was changed
> from
> True to False in Subversion 1.12, thus disabling the Plaintext cache unless
> explicitly enabled.</p>
> <!-- Danielsh's #2 -->
>
> <p>The directory which contains cached Plaintext passwords (usually
> <tt>~/.subversion/auth/</tt>) has permissions of 700, meaning only the user
> (and root) can read them.</p>
>
> <h4>"Subversion was compiled with support for Plaintext password cache but
> I
> want to prevent writing passwords to the Plaintext cache."</h4>
>
> <p>The following options are available in your run-time config file
> (per user ~/.subversion/config and ~/.subversion/servers,
> systemwide /etc/subversion/config and /etc/subversion/servers):</p>
>
> <ul>
> <li>To allow encrypted stores like GNOME Keyring and KWallet, but not the
> Plaintext cache, set <tt>store-plaintext-passwords = no</tt>.</li>
> <li>To allow caching server certs but not passwords (encrypted or not), set
> <tt>store-passwords = no</tt>.</li>
> <li>To disable storing any kind of credentials (encrypted or not) set
> <tt>store-auth-creds = no</tt>.</li>
> </ul>
>
> <!-- The <ul/> should cover most of Danielsh's #2. -->
>
> <h4>"I want to use the Plaintext cache but it wasn't enabled at compile
> time."</h4>
>
> <!-- Removed If your Subversion client was not build ... <p/> since it
> is already covered in the header -->
>
> <!-- I've not added anything about rebuilding. I think most users will
> prefer the comfort of package managers (or other binary distributions
> - eg TSVN). Those who are comfortable building their own have already
> figured out from the description earlier. But I'm not opposed it -->
>
> <p>In response to various questions and requests, the Subversion developers
> have written a Python script that can store a plain-text password to the
> cache. If you understand the security implications, have ruled out other
> alternatives, and still want to cache your password in plain-text on disk,
> you
> may find the script here:</p>
>
> <p class="todo">TODO: Link to the script.</p>
>
> <h4>Additional Information</h4>
>
> <p>More information on password caching is in Chapter 6 of the <a
> href="http://svnbook.red-bean.com/en/1.7/index.html">Subversion book</a>,
> under <a href="
> http://svnbook.red-bean.com/en/1.7/svn.serverconfig.netmodel.html#svn.serverconfig.netmodel.credcache
> "
> >"Client Credentials Caching".</a></p>
>
> </div>
> ]]]
>
> By the way, I realize that we've been having this discussion on users@
> and it probably should have been on dev@, but since we're almost done
> (I think), we may as well finish it here for continuity. :-)
>
And the discussion about the script was moved to dev@... Let's stay here.
I think we should put it in the staging website for a final review? This
should go under General questions? I have to go to my day-job now but I'll
do it in the evening unless someone is quicker than me.
Kind regards
Daniel Sahlberg
Re: A more permanent home for the add-a-password-to-a-cached-username
script? (was: Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password)
Posted by Nathan Hartman <ha...@gmail.com>.
On Sun, Feb 28, 2021 at 10:51 AM Daniel Sahlberg
<da...@gmail.com> wrote:
> When researching, I discovered that reading plain text passwords that are "grandfathered in" works the same way on Windows as on Unix. If the password is invalid it is switched to passtype==wincrypt when updated.
Thanks for documenting that!
> I have taken Nathan's suggestion and rearranged it slightly, trying to incorporate Danielsh' feedback. I have made notes as HTML comments: <!-- ... --> - these should be removed from the final commit.
Thanks also for improving the FAQ text. This is a big improvement over
the text I proposed earlier. I only have a few minor nits to pick,
mostly on things I wrote... I'll respond inline below, and below that
for convenience I'll give the text with my proposed minor changes
applied...
> [[[
> <div class="h3" id="plaintext-passwords">
> <h3>How does Subversion cache credentials (plain text and encrypted)?
> <!-- The other FAQ entries are questions and there are discussions of
> encrypted password stores in addition to the plain text store -->
> <a class="sectionlink" href="#plaintext-passwords"
> title="Link to this section">¶</a>
> </h3>
>
> <p>To avoid having to type a password for each server operation, Subversion
> can cache credentials.</p>
>
> <p>Passwords may have been cached unencrypted by older versions of Subversion
> ("grandfathered in") and Subversion always supports reading these. Whether and
> how Subversion caches new credentials depends on several factors, including the
> access method, operating system, compile-time options, and settings in the
> client's run-time config file.</p>
> <!-- Moved the first sentence (grandfathered passwords) here from the Unix
> section. Added 'access methods' to somewhat cater for Danielsh's comment
> regarding SSH password prompts / keys / client certs. I'm leaving the
> question open if we should mention what is not covered in the answer. -->
>
> <!-- Removed the <p/> and <ul/> about what we try to answer -->
>
> <p>To show the credentials in your cache, use <tt>svn auth</tt>. Credentials
> are never removed automatically but Credentials may be removed manually using
In the above sentence, should the last mention of "Credentials" be
removed?
> <tt>svn auth --remove</tt>.</p>
> <!-- Added info on how to review the cache and remove credentials. This should
> address part of Danielsh's #6 -->
>
> <h4>Windows</h4>
>
> <p>On Windows, Subversion uses standard Windows APIs to encrypt the data, so
> only the user can decrypt the cached password. <i>(Since Subversion
> 1.2.)</i></p>
>
> <h4>macOS (formerly Mac OS X)</h4>
>
> <p>On macOS, Subversion uses the system Keychain facility to encrypt/store
> the user's svn password. <i>(Since Subversion 1.4.)</i></p>
>
> <h4>UNIX/Linux</h4>
>
> <p>On UNIX/Linux, Subversion supports up to four credential caches:</p>
>
> <ul>
> <li>GNOME Keyring</li>
> <li>KWallet</li>
> <li>GPG-Agent</li>
> <li>Plaintext cache in ~/.subversion</li>
I think we should write "Plaintext cache in ~/.subversion/auth/".
This is a preexisting issue from the original FAQ and I meant to
change it before, but I forgot.
> </ul>
>
> <p>To determine which credential caches your Subversion client supports, run
> the <tt>svn --version</tt> command and look for "The following authentication
> credential caches are available" toward the end of its output.</p>
>
> <p>GNOME Keyring and KWallet both facilitate storing passwords on disk
> encrypted. For Subversion to support these programs (since Subversion 1.6),
> they need to be available at compile-time and at run-time.</p>
> <!-- Removing the sentence about fallback to plaintext - it is discussed after
> GPG-Agent -->
> <p class="todo">TODO: Discuss GPG-Agent.</p>
>
> <p>Depending on a compile-time option (--enable-plaintext-password-storage)
> and runtime configurations (see below) Subversion <i>may</i> fallback to storing
> passwords in the Plaintext cache.</p>
> <!-- From GNOME Keyring/KWallet merged with some of Danielsh's comments (1...6) -->
>
> <p>The default value of --enable-plaintext-password-storage was changed from
> True to False in Subversion 1.12, thus disabling the Plaintext cache unless
> explicitly enabled.</p>
> <!-- Danielsh's #2 -->
>
> <p>The directory which contains cached Plaintext passwords (usually
> <tt>~/.subversion/auth/</tt>) has permissions of 700, meaning only the user
> (and root) can read them.</p>
>
> <h4>"Subversion was compiled with support for Plaintext password cache but I
> want to prevent writing passwords to the Plaintext cache!"</h4>
I originally put the exclamation points in these titles, but now I
think it would be better to replace them with periods.
> <p>The following options are available in your run-time config file
> (per user ~/.subversion/config and ~/.subversion/servers,
> systemwide /etc/subversion/config and /etc/subversion/servers):</p>
>
> <ul>
> <li>To allow encrypted stores like GNOME Keyring and KWallet, but not the
> Plaintext cache, set <tt>store-plaintext-passwords = no</tt>.</li>
> <li>To allow caching server certs but not passwords (encrypted or not), set
> <tt>store-passwords = no</tt>.</li>
> <li>To disable storing any kind of credentials (encrypted or not) set
> <tt>store-auth-creds = no</tt>.</li>
> </ul>
>
> <!-- The <ul/> should cover most of Danielsh's #2. -->
>
> <h4>"I want to use the Plaintext cache but it wasn't enabled at compile
> time!"</h4>
Same comment here about my exclamation point.
> <!-- Removed If your Subversion client was not build ... <p/> since it
> is already covered in the header -->
>
> <!-- I've not added anything about rebuilding. I think most users will
> prefer the comfort of package managers (or other binary distributions
> - eg TSVN). Those who are comfortable building their own have already
> figured out from the description earlier. But I'm not opposed it -->
>
> <p>In response to various questions and requests, the Subversion developers
> have written a Python script that can store a plain-text password to the
> cache. If you understand the security implications, have ruled out other
> alternatives, and still want to cache your password in plain-text on disk, you
> may find the script here:</p>
>
> <p class="todo">TODO: Link to the script.</p>
>
> <h4>Additional Information</h4>
>
> <p>More information on password caching is in Chapter 6 of the <a
> href="http://svnbook.red-bean.com/en/1.7/index.html">Subversion book</a>,
> under <a href="http://svnbook.red-bean.com/en/1.7/svn.serverconfig.netmodel.html#svn.serverconfig.netmodel.credcache"
> >"Client Credentials Caching".</a></p>
>
> </div>
> ]]]
Here's the full text with my above suggestions applied... I didn't
remove the HTML comments, and the two other things remaining to be
done is the todo about GPG-Agent and to add the correct link to the
Python script, if we've figured out where that should be.
[[[
<div class="h3" id="plaintext-passwords">
<h3>How does Subversion cache credentials (plain text and encrypted)?
<!-- The other FAQ entries are questions and there are discussions of
encrypted password stores in addition to the plain text store -->
<a class="sectionlink" href="#plaintext-passwords"
title="Link to this section">¶</a>
</h3>
<p>To avoid having to type a password for each server operation, Subversion
can cache credentials.</p>
<p>Passwords may have been cached unencrypted by older versions of Subversion
("grandfathered in") and Subversion always supports reading these. Whether and
how Subversion caches new credentials depends on several factors, including the
access method, operating system, compile-time options, and settings in the
client's run-time config file.</p>
<!-- Moved the first sentence (grandfathered passwords) here from the Unix
section. Added 'access methods' to somewhat cater for Danielsh's comment
regarding SSH password prompts / keys / client certs. I'm leaving the
question open if we should mention what is not covered in the answer. -->
<!-- Removed the <p/> and <ul/> about what we try to answer -->
<p>To show the credentials in your cache, use <tt>svn auth</tt>. Credentials
are never removed automatically but may be removed manually using
<tt>svn auth --remove</tt>.</p>
<!-- Added info on how to review the cache and remove credentials. This should
address part of Danielsh's #6 -->
<h4>Windows</h4>
<p>On Windows, Subversion uses standard Windows APIs to encrypt the data, so
only the user can decrypt the cached password. <i>(Since Subversion
1.2.)</i></p>
<h4>macOS (formerly Mac OS X)</h4>
<p>On macOS, Subversion uses the system Keychain facility to encrypt/store
the user's svn password. <i>(Since Subversion 1.4.)</i></p>
<h4>UNIX/Linux</h4>
<p>On UNIX/Linux, Subversion supports up to four credential caches:</p>
<ul>
<li>GNOME Keyring</li>
<li>KWallet</li>
<li>GPG-Agent</li>
<li>Plaintext cache in ~/.subversion/auth/.
</ul>
<p>To determine which credential caches your Subversion client supports, run
the <tt>svn --version</tt> command and look for "The following authentication
credential caches are available" toward the end of its output.</p>
<p>GNOME Keyring and KWallet both facilitate storing passwords on disk
encrypted. For Subversion to support these programs (since Subversion 1.6),
they need to be available at compile-time and at run-time.</p>
<!-- Removing the sentence about fallback to plaintext - it is discussed after
GPG-Agent -->
<p class="todo">TODO: Discuss GPG-Agent.</p>
<p>Depending on a compile-time option (--enable-plaintext-password-storage)
and runtime configurations (see below) Subversion <i>may</i> fallback to storing
passwords in the Plaintext cache.</p>
<!-- From GNOME Keyring/KWallet merged with some of Danielsh's
comments (1...6) -->
<p>The default value of --enable-plaintext-password-storage was changed from
True to False in Subversion 1.12, thus disabling the Plaintext cache unless
explicitly enabled.</p>
<!-- Danielsh's #2 -->
<p>The directory which contains cached Plaintext passwords (usually
<tt>~/.subversion/auth/</tt>) has permissions of 700, meaning only the user
(and root) can read them.</p>
<h4>"Subversion was compiled with support for Plaintext password cache but I
want to prevent writing passwords to the Plaintext cache."</h4>
<p>The following options are available in your run-time config file
(per user ~/.subversion/config and ~/.subversion/servers,
systemwide /etc/subversion/config and /etc/subversion/servers):</p>
<ul>
<li>To allow encrypted stores like GNOME Keyring and KWallet, but not the
Plaintext cache, set <tt>store-plaintext-passwords = no</tt>.</li>
<li>To allow caching server certs but not passwords (encrypted or not), set
<tt>store-passwords = no</tt>.</li>
<li>To disable storing any kind of credentials (encrypted or not) set
<tt>store-auth-creds = no</tt>.</li>
</ul>
<!-- The <ul/> should cover most of Danielsh's #2. -->
<h4>"I want to use the Plaintext cache but it wasn't enabled at compile
time."</h4>
<!-- Removed If your Subversion client was not build ... <p/> since it
is already covered in the header -->
<!-- I've not added anything about rebuilding. I think most users will
prefer the comfort of package managers (or other binary distributions
- eg TSVN). Those who are comfortable building their own have already
figured out from the description earlier. But I'm not opposed it -->
<p>In response to various questions and requests, the Subversion developers
have written a Python script that can store a plain-text password to the
cache. If you understand the security implications, have ruled out other
alternatives, and still want to cache your password in plain-text on disk, you
may find the script here:</p>
<p class="todo">TODO: Link to the script.</p>
<h4>Additional Information</h4>
<p>More information on password caching is in Chapter 6 of the <a
href="http://svnbook.red-bean.com/en/1.7/index.html">Subversion book</a>,
under <a href="http://svnbook.red-bean.com/en/1.7/svn.serverconfig.netmodel.html#svn.serverconfig.netmodel.credcache"
>"Client Credentials Caching".</a></p>
</div>
]]]
By the way, I realize that we've been having this discussion on users@
and it probably should have been on dev@, but since we're almost done
(I think), we may as well finish it here for continuity. :-)
Cheers,
Nathan
Re: A more permanent home for the add-a-password-to-a-cached-username
script? (was: Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password)
Posted by Daniel Sahlberg <da...@gmail.com>.
Den fre 26 feb. 2021 kl 14:43 skrev Daniel Shahaf <d....@daniel.shahaf.name>:
> Nathan Hartman wrote on Thu, Feb 25, 2021 at 13:29:48 -0500:
> > May I propose to have just one FAQ entry that simultaneously would
> answer:
> > * "what alternatives to plaintext caching are there?"
> > * "plaintext caching is supported but I want to *prevent* it"
> > * "plaintext caching is not supported but I want to use it anyway"
>
> Of course you may. _Prima facie_ I'm not sure that would be a good
> idea — one question per FAQ entry is similar to "Do one thing and do it
> well", and answering two questions per FAQ entry might make that entry
> less usable as a URL to be passed to someone who has either of these
> questions — but I'll reserve judgement until I've read the proposal.
>
When researching, I discovered that reading plain text passwords that are
"grandfathered in" works the same way on Windows as on Unix. If the
password is invalid it is switched to passtype==wincrypt when updated.
I have taken Nathan's suggestion and rearranged it slightly, trying to
incorporate Danielsh' feedback. I have made notes as HTML comments: <!--
... --> - these should be removed from the final commit.
[[[
<div class="h3" id="plaintext-passwords">
<h3>How does Subversion cache credentials (plain text and encrypted)?
<!-- The other FAQ entries are questions and there are discussions of
encrypted password stores in addition to the plain text store -->
<a class="sectionlink" href="#plaintext-passwords"
title="Link to this section">¶</a>
</h3>
<p>To avoid having to type a password for each server operation, Subversion
can cache credentials.</p>
<p>Passwords may have been cached unencrypted by older versions of
Subversion
("grandfathered in") and Subversion always supports reading these. Whether
and
how Subversion caches new credentials depends on several factors, including
the
access method, operating system, compile-time options, and settings in the
client's run-time config file.</p>
<!-- Moved the first sentence (grandfathered passwords) here from the Unix
section. Added 'access methods' to somewhat cater for Danielsh's
comment
regarding SSH password prompts / keys / client certs. I'm leaving the
question open if we should mention what is not covered in the answer.
-->
<!-- Removed the <p/> and <ul/> about what we try to answer -->
<p>To show the credentials in your cache, use <tt>svn auth</tt>. Credentials
are never removed automatically but Credentials may be removed manually
using
<tt>svn auth --remove</tt>.</p>
<!-- Added info on how to review the cache and remove credentials. This
should
address part of Danielsh's #6 -->
<h4>Windows</h4>
<p>On Windows, Subversion uses standard Windows APIs to encrypt the data, so
only the user can decrypt the cached password. <i>(Since Subversion
1.2.)</i></p>
<h4>macOS (formerly Mac OS X)</h4>
<p>On macOS, Subversion uses the system Keychain facility to encrypt/store
the user's svn password. <i>(Since Subversion 1.4.)</i></p>
<h4>UNIX/Linux</h4>
<p>On UNIX/Linux, Subversion supports up to four credential caches:</p>
<ul>
<li>GNOME Keyring</li>
<li>KWallet</li>
<li>GPG-Agent</li>
<li>Plaintext cache in ~/.subversion</li>
</ul>
<p>To determine which credential caches your Subversion client supports, run
the <tt>svn --version</tt> command and look for "The following
authentication
credential caches are available" toward the end of its output.</p>
<p>GNOME Keyring and KWallet both facilitate storing passwords on disk
encrypted. For Subversion to support these programs (since Subversion 1.6),
they need to be available at compile-time and at run-time.</p>
<!-- Removing the sentence about fallback to plaintext - it is discussed
after
GPG-Agent -->
<p class="todo">TODO: Discuss GPG-Agent.</p>
<p>Depending on a compile-time option (--enable-plaintext-password-storage)
and runtime configurations (see below) Subversion <i>may</i> fallback to
storing
passwords in the Plaintext cache.</p>
<!-- From GNOME Keyring/KWallet merged with some of Danielsh's comments
(1...6) -->
<p>The default value of --enable-plaintext-password-storage was changed
from
True to False in Subversion 1.12, thus disabling the Plaintext cache unless
explicitly enabled.</p>
<!-- Danielsh's #2 -->
<p>The directory which contains cached Plaintext passwords (usually
<tt>~/.subversion/auth/</tt>) has permissions of 700, meaning only the user
(and root) can read them.</p>
<h4>"Subversion was compiled with support for Plaintext password cache but
I
want to prevent writing passwords to the Plaintext cache!"</h4>
<p>The following options are available in your run-time config file
(per user ~/.subversion/config and ~/.subversion/servers,
systemwide /etc/subversion/config and /etc/subversion/servers):</p>
<ul>
<li>To allow encrypted stores like GNOME Keyring and KWallet, but not the
Plaintext cache, set <tt>store-plaintext-passwords = no</tt>.</li>
<li>To allow caching server certs but not passwords (encrypted or not), set
<tt>store-passwords = no</tt>.</li>
<li>To disable storing any kind of credentials (encrypted or not) set
<tt>store-auth-creds = no</tt>.</li>
</ul>
<!-- The <ul/> should cover most of Danielsh's #2. -->
<h4>"I want to use the Plaintext cache but it wasn't enabled at compile
time!"</h4>
<!-- Removed If your Subversion client was not build ... <p/> since it
is already covered in the header -->
<!-- I've not added anything about rebuilding. I think most users will
prefer the comfort of package managers (or other binary distributions
- eg TSVN). Those who are comfortable building their own have already
figured out from the description earlier. But I'm not opposed it -->
<p>In response to various questions and requests, the Subversion developers
have written a Python script that can store a plain-text password to the
cache. If you understand the security implications, have ruled out other
alternatives, and still want to cache your password in plain-text on disk,
you
may find the script here:</p>
<p class="todo">TODO: Link to the script.</p>
<h4>Additional Information</h4>
<p>More information on password caching is in Chapter 6 of the <a
href="http://svnbook.red-bean.com/en/1.7/index.html">Subversion book</a>,
under <a href="
http://svnbook.red-bean.com/en/1.7/svn.serverconfig.netmodel.html#svn.serverconfig.netmodel.credcache
"
>"Client Credentials Caching".</a></p>
</div>
]]]
For those who prefer to rewiev the changes as a patch, I've attached it. It
should apply to Nathan's suggestioon (
https://mail-archives.apache.org/mod_mbox/subversion-users/202102.mbox/%3CCAJT2EHoiDPNnvxHPf8702p8WHKUBttowdfJ%3DyepPDPUT8hUzfw%40mail.gmail.com%3E
).
Kind regards,
Daniel Sahlberg
Re: A more permanent home for the
add-a-password-to-a-cached-username script? (was: Re: using svn cli with
--non-interactive (in scripts) securely, without exposing password)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Nathan Hartman wrote on Thu, Feb 25, 2021 at 13:29:48 -0500:
> May I propose to have just one FAQ entry that simultaneously would answer:
> * "what alternatives to plaintext caching are there?"
> * "plaintext caching is supported but I want to *prevent* it"
> * "plaintext caching is not supported but I want to use it anyway"
Of course you may. _Prima facie_ I'm not sure that would be a good
idea — one question per FAQ entry is similar to "Do one thing and do it
well", and answering two questions per FAQ entry might make that entry
less usable as a URL to be passed to someone who has either of these
questions — but I'll reserve judgement until I've read the proposal.
> <div class="h3" id="plaintext-passwords">
> <h3>Password caching in plain-text on disk
How about "Does Subversion cache passwords in plain text?" or
"Configuring plain text password caching"?
> <a class="sectionlink" href="#plaintext-passwords"
> title="Link to this section">¶</a>
> </h3>
>
> <p>To avoid having to type a password for each server operation, Subversion
> can cache credentials.</p>
>
> <p>Whether and how Subversion caches credentials depends on several factors,
> including the operating system, compile-time options, and settings in the
> client's run-time config file.</p>
>
Should this mention that ssh password prompts / keys aren't covered
here? And what about client certs?
> <p>On some operating systems and configurations, Subversion can cache
> passwords on disk in plain-text. Some users want this, while others want to
> disallow it. This FAQ entry summarizes how credential caching works and
> attempts to answer both of these questons:</p>
>
> <ul>
> <li>How to <b>prevent</b> caching passwords on disk in plain-text (with
> various alternatives provided), and</li>
> <li>How to cache passwords on disk in plain-text</li>
> </ul>
>
See https://m.xkcd.com/1343/.¹ How about dropping the <ul/>, deleting
the <p/> above it, and in the latter's stead mention, in the Unix <h4/>,
that plaintext password caching is configurable because it's not
everyone's cup of tea?
> <h4>UNIX/Linux</h4>
>
> <p>On UNIX/Linux, Subversion supports up to four credential caches:</p>
>
> <ul>
> <li>GNOME Keyring</li>
> <li>KWallet</li>
> <li>GPG-Agent</li>
> <li>Plaintext cache in ~/.subversion</li>
> </ul>
>
> <p>To determine which credential caches your Subversion client supports, run
> the <tt>svn --version</tt> command and look for "The following
> authentication
> credential caches are available" toward the end of its output.</p>
>
> <p>GNOME Keyring and KWallet both facilitate storing passwords on disk
> encrypted. For Subversion to support these programs (since Subversion 1.6),
> they need to be available at compile-time and at run-time. Otherwise,
> Subversion <i>may</i> fallback to storing passwords in the Plaintext cache,
> if support for that is built in; see below.</p>
Suggest s/if.*built in/if so configured/, since there are both run-time
and compile-time knobs. The term "built in" in particular is ambiguous
(could mean either "enabled at compile time" or "is available
always rather than as a module").
> <p class="todo">TODO: Discuss GPG-Agent.</p>
>
> <p>On UNIX/Linux, the Plaintext cache is always supported for
> <b>reading</b>,
> but support for <b>writing</b> new passwords to the cache depends on build
> time configuration. Since Subversion 1.12, the default is <b>not</b> to
> support writing new passwords to the Plaintext cache, unless specifically
> enabled at build time, but Subversion will continue to use any previously
> cached passwords that are "grandfathered in."</p>
This jumps back and forth between reading and writing. How about
separating several concerns here:
1. State that writing new passwords to the cache is configurable because
it's not everyone's cup of tea.
2. Describe the relevant compile- and run-time knobs, where they live,
and their defaults on various versions: --enable-plaintext-password-storage,
SVN_DISABLE_PLAINTEXT_PASSWORD_STORAGE, --no-auth-cache, store-passwords,
store-auth-creds.
3. State that writing is only done if all those planets align (if all
knobs have the right values).
4. State that reading is always done.
5. In the sections for "How to enable plaintext cachnig" and "How to
disable plaintext caching", build on top of this description.
6. Consider adding discussion of when cache entries are deleted
automatically (even if that's just "Never") and how to delete them
manually.
#5 would be a good reason to keep the answers to the two questions as
h4's under a single top-level entry.
> <p>The directory which contains the cached passwords (usually
> <tt>~/.subversion/auth/</tt>) has permissions of 700, meaning only the user
> (and root) can read them.</p>
>
> <h4>"I want to prevent writing passwords to the Plaintext cache!"</h4>
>
> <p>The following options are available in your run-time config file:</p>
>
> <ul>
> <li>To allow encrypted stores like GNOME Keyring and KWallet, but not the
> Plaintext cache, set <tt>store-plaintext-passwords = no</tt>.
> <li>To allow caching server certs but not passwords (encrypted or not), set
> <tt>store-passwords = no</tt>.</li>
> <li>To disable storing any kind of credentials (encrypted or not) set
> <tt>store-auth-creds = no</tt>.</li>
It's noting here that the settings can be set in both
~/.subversion/config and ~/.subversion/servers.
> </ul>
>
> <h4>"I want to use the Plaintext cache but it wasn't enabled at build
> time!"</h4>
>
> <p>If your Subversion client was not built to cache passwords in plain-text,
> note that although it will not <i>save</i> new passwords to the cache, it
> will <i>use</i> any passwords that are already stored there.</p>
>
Suggest to move this paragraph to the end of the <h4/> subsection, since
it's relevant but is not an answer to the question.
Suggest to mention here the option of rebuilding.
> <p>In response to various questions and requests, the Subversion developers
> have written a Python script that can store a plain-text password to the
> cache. If you understand the security implications, have ruled out other
> alternatives, and still want to cache your password in plain-text on disk,
> you
> may find the script here:</p>
>
> <p class="todo">TODO: Link to the script.</p>
>
> <h4>Additional Information</h4>
>
> <p>More information on password caching is in chapter 6 of the <a
Capitalize "Chapter"? (Preëxisting issue)
> href="http://svnbook.red-bean.com/en/1.7/index.html">Subversion book</a>,
> under <a href="
> http://svnbook.red-bean.com/en/1.7/svn.serverconfig.netmodel.html#svn.serverconfig.netmodel.credcache
> "
> >"Client Credentials Caching".</a></p>
>
> </div>
>
> ]]]
>
> Cheers,
> Nathan
Cheers,
Daniel
¹ Other references for "ease of use", in case they're useful to someone:
- search://monkey+lake+smartphone+photo/
- hwright's remark about user interfaces, years ago, in person: "[User
interfaces] should be like, 1. Profit!"
Re: A more permanent home for the add-a-password-to-a-cached-username
script? (was: Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password)
Posted by Nathan Hartman <ha...@gmail.com>.
On Thu, Feb 25, 2021 at 2:00 AM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> Daniel Sahlberg wrote on Wed, Feb 24, 2021 at 23:27:18 +0100:
> > Suggestion for new FAQ entry:
> > [[[
> > Ahhh! I just discovered that my Subversion client is NOT caching
> passwords
> > in plain-text on disk! AHHH!
>
> Having two entry titles that differ only by a "not" isn't a good idea.
>
> > Calm down, take a deep breath.
> >
> > This is the opposite of the previous question. After changing the compile
> > time default to not store passwords in plain-text there has been a number
> > of requests in the mailing lists to store the password.
>
> "A number of requests on the mailing lists" seems like too fine a level
> of abstraction. I think the context basically needs to be "The default
> is X but you may want Y; here's how to do that".
>
> > If you understand the security implications, you have ruled out other
>
> s/, you/, /
>
> > alternatives and still want to cache your password in plain-text on disk
>
> s/ and/, and/
>
> > you can use the script
> >
> https://svn.apache.org/repos/asf/subversion/trunk/contrib/client-side/store-plaintext-password-py
> > to store the password in the directory which contains the cached
> passwords
> > (usually ~/.subversion/auth/). The script can also be used to list any
> > existing passwords stored in plain-text.
>
> This should just point to `svn auth`, surely?
>
> Should this explicitly say to run the script with --help to get its
> usage message?
>
> > ]]]
> >
> > I'm also suggesting to change the existing FAQ entry (Ahhh! I just
> > discovered that my Subversion client is caching passwords in plain-text
> on
> > disk! AHHH!) to mention the changed compile time default since 1.12 to
> not
> > store plain-text passwords:
> >
> > [[[
> > s/Otherwise, the client will fall back/Otherwise, the client can fall
> back/
> >
> > Since svn 1.12 the compile time default has been to disable storing new
> > passwords in plain-text, but old passwords can still be used. Certain
> > distributions may also have selected to use the compile time option to
> > enable plain-text password storage.
>
> Spell out that "old" passwords means passwords already cached on disk
> ("grandfathered") — as opposed to, say, passwords that had been changed.
>
> The wording "Certain distributions may…" sounds like indirect
> finger-pointing. Why not s/speculation/fact/: a compile-time option to
> enable plaintext passwords storage is provided and may have been
> selected by whoever built the binaries you're using (the term "distro"
> doesn't capture VisualSVN and TortoiseSVN).
>
> > s/However .*/In case Subversion is compiled with support for storing
> > plain-text passwords, you can disable it in your run-time config file by
> > setting 'store-plaintext-passwords = no' (so that encrypted stores like
> > GNOME Keyring and KWallet will still be used - this is already done in at
> > least one distribution which has selected to enable the plain-text
> password
> > storage in svn 1.12). If you want to disable storing any kind of
> > credentials you may instead set 'store-auth-creds = no', or you can use
> the
> > more narrowly-defined 'store-passwords = no' (so that server certs are
> > still cached). More information on password cacheing is in chapter 6 of
> the
> > "Nightly Build" Subversion book, under "Client Credentials Caching"./
>
> Is the information only available in the nightly build? If not,
> s/"Nightly Build"//.
>
> > ]]]
> >
> > The "Since svn 1.12..." should probably go in the end of the first "On
> > UNIX/Linux" section, after "(Since svn 1.6.)"
>
> At this point, a «svn diff -x-U10» unidiff might be easier for everyone.
>
> By the way, how about changing "if you're really worried" in the
> preëxisting text. This phrasing crosses the line from discussing the
> reader's risk assessment to discussing their mental state.
>
May I propose to have just one FAQ entry that simultaneously would answer:
* "what alternatives to plaintext caching are there?"
* "plaintext caching is supported but I want to *prevent* it"
* "plaintext caching is not supported but I want to use it anyway"
I took the FAQ entry we have right now, expanded it, fixed a few
mistakes, updated a few out-of-date things, incorporated some of
dsahlberg's text, incorporated some of danielsh's feedback, added new
information, and now the following bears little resemblance
to any of the above...
But I've introduced some flaws, but I'm out of time for the day to fix
them. Please feel free to edit/reorganize/spindle/mutilate!!
Notes: INSTALL doesn't seem to document what is needed for Subversion
to support GPG-Agent on disk.
[[[
<div class="h3" id="plaintext-passwords">
<h3>Password caching in plain-text on disk
<a class="sectionlink" href="#plaintext-passwords"
title="Link to this section">¶</a>
</h3>
<p>To avoid having to type a password for each server operation, Subversion
can cache credentials.</p>
<p>Whether and how Subversion caches credentials depends on several factors,
including the operating system, compile-time options, and settings in the
client's run-time config file.</p>
<p>On some operating systems and configurations, Subversion can cache
passwords on disk in plain-text. Some users want this, while others want to
disallow it. This FAQ entry summarizes how credential caching works and
attempts to answer both of these questons:</p>
<ul>
<li>How to <b>prevent</b> caching passwords on disk in plain-text (with
various alternatives provided), and</li>
<li>How to cache passwords on disk in plain-text</li>
</ul>
<h4>Windows</h4>
<p>On Windows, Subversion uses standard Windows APIs to encrypt the data, so
only the user can decrypt the cached password. <i>(Since Subversion
1.2.)</i></p>
<h4>macOS (formerly Mac OS X)</h4>
<p>On macOS, Subversion uses the system Keychain facility to encrypt/store
the user's svn password. <i>(Since Subversion 1.4.)</i></p>
<h4>UNIX/Linux</h4>
<p>On UNIX/Linux, Subversion supports up to four credential caches:</p>
<ul>
<li>GNOME Keyring</li>
<li>KWallet</li>
<li>GPG-Agent</li>
<li>Plaintext cache in ~/.subversion</li>
</ul>
<p>To determine which credential caches your Subversion client supports, run
the <tt>svn --version</tt> command and look for "The following
authentication
credential caches are available" toward the end of its output.</p>
<p>GNOME Keyring and KWallet both facilitate storing passwords on disk
encrypted. For Subversion to support these programs (since Subversion 1.6),
they need to be available at compile-time and at run-time. Otherwise,
Subversion <i>may</i> fallback to storing passwords in the Plaintext cache,
if support for that is built in; see below.</p>
<p class="todo">TODO: Discuss GPG-Agent.</p>
<p>On UNIX/Linux, the Plaintext cache is always supported for
<b>reading</b>,
but support for <b>writing</b> new passwords to the cache depends on build
time configuration. Since Subversion 1.12, the default is <b>not</b> to
support writing new passwords to the Plaintext cache, unless specifically
enabled at build time, but Subversion will continue to use any previously
cached passwords that are "grandfathered in."</p>
<p>The directory which contains the cached passwords (usually
<tt>~/.subversion/auth/</tt>) has permissions of 700, meaning only the user
(and root) can read them.</p>
<h4>"I want to prevent writing passwords to the Plaintext cache!"</h4>
<p>The following options are available in your run-time config file:</p>
<ul>
<li>To allow encrypted stores like GNOME Keyring and KWallet, but not the
Plaintext cache, set <tt>store-plaintext-passwords = no</tt>.
<li>To allow caching server certs but not passwords (encrypted or not), set
<tt>store-passwords = no</tt>.</li>
<li>To disable storing any kind of credentials (encrypted or not) set
<tt>store-auth-creds = no</tt>.</li>
</ul>
<h4>"I want to use the Plaintext cache but it wasn't enabled at build
time!"</h4>
<p>If your Subversion client was not built to cache passwords in plain-text,
note that although it will not <i>save</i> new passwords to the cache, it
will <i>use</i> any passwords that are already stored there.</p>
<p>In response to various questions and requests, the Subversion developers
have written a Python script that can store a plain-text password to the
cache. If you understand the security implications, have ruled out other
alternatives, and still want to cache your password in plain-text on disk,
you
may find the script here:</p>
<p class="todo">TODO: Link to the script.</p>
<h4>Additional Information</h4>
<p>More information on password caching is in chapter 6 of the <a
href="http://svnbook.red-bean.com/en/1.7/index.html">Subversion book</a>,
under <a href="
http://svnbook.red-bean.com/en/1.7/svn.serverconfig.netmodel.html#svn.serverconfig.netmodel.credcache
"
>"Client Credentials Caching".</a></p>
</div>
]]]
Cheers,
Nathan
Re: A more permanent home for the
add-a-password-to-a-cached-username script? (was: Re: using svn cli with
--non-interactive (in scripts) securely, without exposing password)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Sahlberg wrote on Sun, Mar 07, 2021 at 19:34:15 +0100:
> I'm attaching v4 where I blatantly stole Danielsh's script to read the hash
> file
> (
> http://mail-archives.apache.org/mod_mbox/subversion-dev/202102.mbox/%3c20210226173525.GA24828@tarpaulin.shahaf.local2%3e
> )
> +++ contrib/client-side/store-plaintext-password.py (working copy)
> @@ -0,0 +1,184 @@
> +def _read_one_datum(fd, letter):
> + """\
> + Read a 'K <length>\\n<key>\\n' or 'V <length>\\n<value>\\n' block from
> + an svn_hash_write2()-format FD.
> +
> + LETTER identifies the first letter, as a bytes object.
> + """
> + assert letter in {b'K', b'V'}
> +
> + # Read the letter and the space
> + if fd.read(1) != letter or fd.read(1) != b' ':
> + raise ...
This line simply raises a TypeError that's unrelated to the
business-logic-level problem. Make it more user-friendly?
I know I wrote that line, but I meant it as a placeholder. (Compare
r1869139.)
> + if line[-1:] != b'\n':
Aside: the colon here isn't redundant, as it would be if «line» were
a str:
>>> type(b'foo'[-1])
<class 'int'>
>>> type(b'foo'[-1:])
<class 'bytes'>
>>> type('foo'[-1])
<class 'str'>
>>> type('foo'[-1:])
<class 'str'>
Cheers,
Daniel
Re: A more permanent home for the add-a-password-to-a-cached-username
script? (was: Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password)
Posted by Daniel Sahlberg <da...@gmail.com>.
Den fre 20 aug. 2021 kl 09:05 skrev Barry <ba...@barrys-emacs.org>:
>
>
> > On 8 Mar 2021, at 08:10, Daniel Sahlberg <da...@gmail.com>
> wrote:
> >
> >
> > Attaching version 5, with changes as below.
>
> I do not seem to see this in the subversion repo, I would find the code
> useful.
>
> Will it become part of contrib at some point?
>
Thanks for the reminder. It was a project during our winter holiday and I
lost steam when going back to work. As far as I can remember the script was
working fine, but danielsh had some remarks on coding style. My weekend is
busy but I will try to look at it next week. Of course, you are welcome to
pick up from where I left off and improve the script and post it here.
Kind regards,
Daniel
Re: A more permanent home for the add-a-password-to-a-cached-username script? (was: Re: using svn cli with --non-interactive (in scripts) securely, without exposing password)
Posted by Barry <ba...@barrys-emacs.org>.
> On 8 Mar 2021, at 08:10, Daniel Sahlberg <da...@gmail.com> wrote:
>
>
> Attaching version 5, with changes as below.
I do not seem to see this in the subversion repo, I would find the code useful.
Will it become part of contrib at some point?
Barry
Re: A more permanent home for the add-a-password-to-a-cached-username script? (was: Re: using svn cli with --non-interactive (in scripts) securely, without exposing password)
Posted by Daniel Sahlberg <da...@gmail.com>.
Coming back to a thread from long time ago, with the intention to commit
the script in tools/client-side so we can link this from the FAQ based on a
recent question on IRC [1], [2], [3]
[1] https://colabti.org/irclogger/irclogger_log/svn-dev?date=2022-07-01
[2] https://colabti.org/irclogger/irclogger_log/svn-dev?date=2022-07-04
[3] https://colabti.org/irclogger/irclogger_log/svn-dev?date=2022-07-07
More below...
Den tis 9 mars 2021 kl 21:09 skrev Daniel Shahaf <d....@daniel.shahaf.name>:
> Daniel Sahlberg wrote on Mon, Mar 08, 2021 at 09:15:21 +0100:
> > Attaching version 5, with changes as below.
> >
> > Den sön 7 mars 2021 kl 20:42 skrev Daniel Shahaf <d.s@daniel.shahaf.name
> >:
> >
> > > Daniel Sahlberg wrote on Sun, Mar 07, 2021 at 19:34:15 +0100:
> > > > + # Parse arguments
> > > > + import argparse
> > > > + parser = argparse.ArgumentParser(
> > > ⋮
> > > > + # The file name is the md5encoding of the realm
> > > > + import hashlib
> > > > + m = hashlib.md5()
> > >
> > > Hmm. Should all function-local imports be grouped at the top of the
> > > function? I don't remember the convention here.
>
[... long discussion about coding style with the conclusion that even
though PEP-8 says all import should be in the top of the file,
CPython's own source code contains function level imports. And it would be
more PEP-8-ish to have function level imports at the top of the function
...]
I've changed it this way and sorted them alphabetically.
> (Offtopic: When reviewing /trunk/doc, I had a look at
> > /trunk/doc/programmer/WritingChangeLogs.txt. Is it still relevant? It
> seems
> > very CVS-ish and mostly covered in HACKING).
>
> Everything up to the last two section headers is good advice for writing
> log messages. For those parts, they should be added to HACKING or
> referenced therefrom.
>
> The remaining two sections don't seem applicable to us at all, but as
> the file is "an essay by jimb", I don't think we can change it.
>
I was more thinking about removing it if it doesn't apply to the Subversion
project (anymore).
> +++ contrib/client-side/store-plaintext-password.py (working copy)
> > @@ -0,0 +1,189 @@
> > +def _read_one_datum(fd, letter):
> ⋮
> > + # Read the letter and the space
> > + if fd.read(1) != letter or fd.read(1) != b' ':
> > + raise ...
>
> (Raising ellipses is discussed elsethread.)
>
Now raising a ValueError(). I'm not experienced in Python but it looked
like the most suitable (standard) exception class.
>
> > +def outputHash(fd, hash):
> > + """\
> > + Write a dictionary to an FD in the same format as used by
>
> Say the formal parameter's name somewhere in here?
>
> Calling it "hash" could be confusing, because svn_hash_write2() and
> hashlib use that term in different senses, but I don't have a suggestion
> about this.
>
Renamed the parameter to dict and added the reference in the docstring.
> > +def writeHashFile(filename, hash):
> > + """\
> > + Open a temporary file and write the dictionary to the temp
> > + file in svn_hash_write2() format.
> > +
> > + If the file is successfully written it is renamed to <filename>.
>
> Please have all docstrings use the same markup for formal parameter
> names. outputHash() above uses the "capitalize the name" convention the
> C code uses for non-public-API function. I'll also use that for the
> example I write in the next paragraph.
>
> Describe the contract, not the implementation. For instance, "Write the
> dictionary HASH to a file named FILENAME in svn_hash_write2() format."
> would be a reasonable first sentence. The docstring could then promise
> a postcondition that if the file exists upon return, then it's not
> truncated, mention what happens if at entry FILENAME is a regular file
> or an unbroken symlink, etc..
>
Docstring rewritten.
>
> > + """
> > + tmpFilename = filename + '.tmp'
> > + try:
> > + with open(tmpFilename, 'xb') as fd:
> > + outputHash(fd, hash)
> > + os.rename(tmpFilename, filename)
> > + except FileExistsError:
> > + print('{}: File {!r} already exist. Is the script already
> running?\n'.format(argv[0], tmpFilename), file=sys.stderr)
>
> s/the/this/
>
> Wrap to 80 columns.
>
done
>
> > + except:
> > + os.remove(tmpFilename)
> > + raise
> > +
> > +def main():
> > + # Parse arguments
> > + import argparse
> > + parser = argparse.ArgumentParser(
> > + description=PARSERDESCR,
> > + formatter_class=argparse.RawDescriptionHelpFormatter)
> > + parser.add_argument('realm', help='Server authentication real')
> > + parser.add_argument('-u', '--user', help='Set username', nargs='?')
>
> Just delete nargs entirely, for reasons given upthread? Sorry for
> missing that in my reviews between then and now.
>
done
>
> > + args = parser.parse_args()
> > +
> > + # The file name is the md5encoding of the realm
> > + import hashlib
> > + m = hashlib.md5()
> > + m.update(args.realm.encode('utf-8'))
> > + authfileName =
> os.path.join(os.path.expanduser('~/.subversion/auth/svn.simple/'),
> m.hexdigest())
> > +
> > + # If new file, check that username was given as an argument before
> prompting for password
> > + existingFile = os.path.exists(authfileName)
> > + if not existingFile and args.user is None:
> > + print('{}: New file, username required (see
> -u)\n'.print(argv[0]), file=sys.stderr)
>
> The phrase "new file" in the comment isn't clear, probably because it
> isn't a full sentence.
>
> The phrase "New file" in the error message has the same problem, as well
> as being phrased in implementation terms rather than in user-facing
> terms.
>
Rephrased, and also added an interactive prompt for the username if missing.
>
> > + parser.print_help()
> > + return
>
> The exit code should be non-zero whenever an error was printed to
> stderr.
>
> Aren't you reimplementing one of
> https://docs.python.org/3/library/argparse.html#exiting-methods
> ?
>
Changed to parser.exit()
I took the courage to commit as r1902590 and we can work any changes from
there.
Kind regards,
Daniel
Re: A more permanent home for the
add-a-password-to-a-cached-username script? (was: Re: using svn cli with
--non-interactive (in scripts) securely, without exposing password)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
> > I don't think there's anything written down, but I don't think we
> > invented anything either. Python scripts are typically written to be
> > importable:
> > .
> > $ printf '%s\n' 'def foo():' ' return 42' > foo.py
> > $ python3 -c 'import foo; print(foo.foo())'
> > 42
> > $
>
> I see that many of the .py files in the folder cannot be imported either.
Past bugs are no excuse to write new ones :)
> Give the file name it has it cannot be imported, this is a mute point.
No, it's not. It still matters if the file is renamed (either by us
before committing it, or by a user down the road) or if it is imported
using the appropriate stdlib functions.
> >> +def outputHash(fd, hash):
> >> + """\
> >> + Write a dictionary to an FD in the same format as used by
> >
> > Say the formal parameter's name somewhere in here?
>
> Why have a back slash at all?
Presumably so as to make the docstring easier to read inside the source.
> I avoid them in python code because of the problems of \<SP>.
Even if there were a trailing space there, that wouldn't break anything.
Nobody's going to run a docstring extractor tool on this script (and if
they do, they'll discover the whitespace and remove it then).
I guess we could just remove the backslash, too. No strong opinion here.
>
Incidentally, if anyone wants to drop the zsh implementation I posted
upthread somewhere, feel free. I'm around to answer questions, review
docs, etc..
Cheers,
Daniel
Re: A more permanent home for the add-a-password-to-a-cached-username
script? (was: Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password)
Posted by Barry Scott <ba...@barrys-emacs.org>.
> On 9 Mar 2021, at 20:09, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
>
> Daniel Sahlberg wrote on Mon, Mar 08, 2021 at 09:15:21 +0100:
>> Attaching version 5, with changes as below.
There are lines with trailing white space that should be cleaned up.
>>
>> Den sön 7 mars 2021 kl 20:42 skrev Daniel Shahaf <d....@daniel.shahaf.name>:
>>
>>> Daniel Sahlberg wrote on Sun, Mar 07, 2021 at 19:34:15 +0100:
>>>> + # Parse arguments
>>>> + import argparse
>>>> + parser = argparse.ArgumentParser(
>>> ⋮
>>>> + # The file name is the md5encoding of the realm
>>>> + import hashlib
>>>> + m = hashlib.md5()
>>>
>>> Hmm. Should all function-local imports be grouped at the top of the
>>> function? I don't remember the convention here.
>>>
>>
>> Is there a Subversion specific Style Guide for Python? I didn't find
>> anything in HACKING or trunk/doc.
>
> I don't think there's anything written down, but I don't think we
> invented anything either. Python scripts are typically written to be
> importable:
> .
> $ printf '%s\n' 'def foo():' ' return 42' > foo.py
> $ python3 -c 'import foo; print(foo.foo())'
> 42
> $
Give the file name it has it cannot be imported, this is a mute point.
The hypens would need to be underscores.
I see that many of the .py files in the folder cannot be imported either.
> .
> Thus, putting the imports in main() rather than at the top level means
> they don't get executed when they aren't needed.
>
>> PEP 8 says:
>> [[[
>> Imports are always put at the top of the file, just after any module
>> comments and docstrings, and before module globals and constants.
>> ]]]
>>
>> If I understand this correctly, it prefers global imports only? If keeping
>> the imports in the function, then moving to the top is probably more
>> PEP-8-ish. I didn't change anything though.
>
> In CPython's own source code,
> .
> rm -rf Lib/test && grep -Rn ' import' Lib Tools | perl -lnE '/(\d+)/; print if $1 > 100'
> .
> has over 400 matches. Spot checking a few (shuf(1) is my friend ☺),
> they seem like true positives.
>
>> For reference, ordering should be
>> [[[
>> Imports should be grouped in the following order:
>> 1. standard library imports
>> 2. related third party imports
>> 3. local application/library specific imports
>> ]]]
>> No explicit mentioning of alphabetical sorting, but that seems like a good
>> idea.
>>
>> (Offtopic: When reviewing /trunk/doc, I had a look at
>> /trunk/doc/programmer/WritingChangeLogs.txt. Is it still relevant? It seems
>> very CVS-ish and mostly covered in HACKING).
>
> Everything up to the last two section headers is good advice for writing
> log messages. For those parts, they should be added to HACKING or
> referenced therefrom.
>
> The remaining two sections don't seem applicable to us at all, but as
> the file is "an essay by jimb", I don't think we can change it.
>
>> +++ contrib/client-side/store-plaintext-password.py (working copy)
>> @@ -0,0 +1,189 @@
>> +def _read_one_datum(fd, letter):
> ⋮
>> + # Read the letter and the space
>> + if fd.read(1) != letter or fd.read(1) != b' ':
>> + raise ...
>
> (Raising ellipses is discussed elsethread.)
>
>> +def outputHash(fd, hash):
>> + """\
>> + Write a dictionary to an FD in the same format as used by
>
> Say the formal parameter's name somewhere in here?
Why have a back slash at all?
I avoid them in python code because of the problems of \<SP>.
>
> Calling it "hash" could be confusing, because svn_hash_write2() and
> hashlib use that term in different senses, but I don't have a suggestion
> about this.
>
>> +def writeHashFile(filename, hash):
>> + """\
>> + Open a temporary file and write the dictionary to the temp
>> + file in svn_hash_write2() format.
>> +
>> + If the file is successfully written it is renamed to <filename>.
>
> Please have all docstrings use the same markup for formal parameter
> names. outputHash() above uses the "capitalize the name" convention the
> C code uses for non-public-API function. I'll also use that for the
> example I write in the next paragraph.
>
> Describe the contract, not the implementation. For instance, "Write the
> dictionary HASH to a file named FILENAME in svn_hash_write2() format."
> would be a reasonable first sentence. The docstring could then promise
> a postcondition that if the file exists upon return, then it's not
> truncated, mention what happens if at entry FILENAME is a regular file
> or an unbroken symlink, etc..
>
>> + """
>> + tmpFilename = filename + '.tmp'
>> + try:
>> + with open(tmpFilename, 'xb') as fd:
>> + outputHash(fd, hash)
>> + os.rename(tmpFilename, filename)
>> + except FileExistsError:
>> + print('{}: File {!r} already exist. Is the script already running?\n'.format(argv[0], tmpFilename), file=sys.stderr)
>
> s/the/this/
>
> Wrap to 80 columns.
>
>> + except:
>> + os.remove(tmpFilename)
>> + raise
>> +
>> +def main():
>> + # Parse arguments
>> + import argparse
>> + parser = argparse.ArgumentParser(
>> + description=PARSERDESCR,
>> + formatter_class=argparse.RawDescriptionHelpFormatter)
>> + parser.add_argument('realm', help='Server authentication real')
>> + parser.add_argument('-u', '--user', help='Set username', nargs='?')
>
> Just delete nargs entirely, for reasons given upthread? Sorry for
> missing that in my reviews between then and now.
>
>> + args = parser.parse_args()
>> +
>> + # The file name is the md5encoding of the realm
>> + import hashlib
>> + m = hashlib.md5()
>> + m.update(args.realm.encode('utf-8'))
>> + authfileName = os.path.join(os.path.expanduser('~/.subversion/auth/svn.simple/'), m.hexdigest())
>> +
>> + # If new file, check that username was given as an argument before prompting for password
>> + existingFile = os.path.exists(authfileName)
>> + if not existingFile and args.user is None:
>> + print('{}: New file, username required (see -u)\n'.print(argv[0]), file=sys.stderr)
>
> The phrase "new file" in the comment isn't clear, probably because it
> isn't a full sentence.
>
> The phrase "New file" in the error message has the same problem, as well
> as being phrased in implementation terms rather than in user-facing
> terms.
>
>> + parser.print_help()
>> + return
>
> The exit code should be non-zero whenever an error was printed to
> stderr.
>
> Aren't you reimplementing one of
> https://docs.python.org/3/library/argparse.html#exiting-methods
> ?
>
> Cheers,
>
> Daniel
>
Barry
Re: A more permanent home for the
add-a-password-to-a-cached-username script? (was: Re: using svn cli with
--non-interactive (in scripts) securely, without exposing password)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Sahlberg wrote on Mon, Mar 08, 2021 at 09:15:21 +0100:
> Attaching version 5, with changes as below.
>
> Den sön 7 mars 2021 kl 20:42 skrev Daniel Shahaf <d....@daniel.shahaf.name>:
>
> > Daniel Sahlberg wrote on Sun, Mar 07, 2021 at 19:34:15 +0100:
> > > + # Parse arguments
> > > + import argparse
> > > + parser = argparse.ArgumentParser(
> > ⋮
> > > + # The file name is the md5encoding of the realm
> > > + import hashlib
> > > + m = hashlib.md5()
> >
> > Hmm. Should all function-local imports be grouped at the top of the
> > function? I don't remember the convention here.
> >
>
> Is there a Subversion specific Style Guide for Python? I didn't find
> anything in HACKING or trunk/doc.
I don't think there's anything written down, but I don't think we
invented anything either. Python scripts are typically written to be
importable:
.
$ printf '%s\n' 'def foo():' ' return 42' > foo.py
$ python3 -c 'import foo; print(foo.foo())'
42
$
.
Thus, putting the imports in main() rather than at the top level means
they don't get executed when they aren't needed.
> PEP 8 says:
> [[[
> Imports are always put at the top of the file, just after any module
> comments and docstrings, and before module globals and constants.
> ]]]
>
> If I understand this correctly, it prefers global imports only? If keeping
> the imports in the function, then moving to the top is probably more
> PEP-8-ish. I didn't change anything though.
In CPython's own source code,
.
rm -rf Lib/test && grep -Rn ' import' Lib Tools | perl -lnE '/(\d+)/; print if $1 > 100'
.
has over 400 matches. Spot checking a few (shuf(1) is my friend ☺),
they seem like true positives.
> For reference, ordering should be
> [[[
> Imports should be grouped in the following order:
> 1. standard library imports
> 2. related third party imports
> 3. local application/library specific imports
> ]]]
> No explicit mentioning of alphabetical sorting, but that seems like a good
> idea.
>
> (Offtopic: When reviewing /trunk/doc, I had a look at
> /trunk/doc/programmer/WritingChangeLogs.txt. Is it still relevant? It seems
> very CVS-ish and mostly covered in HACKING).
Everything up to the last two section headers is good advice for writing
log messages. For those parts, they should be added to HACKING or
referenced therefrom.
The remaining two sections don't seem applicable to us at all, but as
the file is "an essay by jimb", I don't think we can change it.
> +++ contrib/client-side/store-plaintext-password.py (working copy)
> @@ -0,0 +1,189 @@
> +def _read_one_datum(fd, letter):
⋮
> + # Read the letter and the space
> + if fd.read(1) != letter or fd.read(1) != b' ':
> + raise ...
(Raising ellipses is discussed elsethread.)
> +def outputHash(fd, hash):
> + """\
> + Write a dictionary to an FD in the same format as used by
Say the formal parameter's name somewhere in here?
Calling it "hash" could be confusing, because svn_hash_write2() and
hashlib use that term in different senses, but I don't have a suggestion
about this.
> +def writeHashFile(filename, hash):
> + """\
> + Open a temporary file and write the dictionary to the temp
> + file in svn_hash_write2() format.
> +
> + If the file is successfully written it is renamed to <filename>.
Please have all docstrings use the same markup for formal parameter
names. outputHash() above uses the "capitalize the name" convention the
C code uses for non-public-API function. I'll also use that for the
example I write in the next paragraph.
Describe the contract, not the implementation. For instance, "Write the
dictionary HASH to a file named FILENAME in svn_hash_write2() format."
would be a reasonable first sentence. The docstring could then promise
a postcondition that if the file exists upon return, then it's not
truncated, mention what happens if at entry FILENAME is a regular file
or an unbroken symlink, etc..
> + """
> + tmpFilename = filename + '.tmp'
> + try:
> + with open(tmpFilename, 'xb') as fd:
> + outputHash(fd, hash)
> + os.rename(tmpFilename, filename)
> + except FileExistsError:
> + print('{}: File {!r} already exist. Is the script already running?\n'.format(argv[0], tmpFilename), file=sys.stderr)
s/the/this/
Wrap to 80 columns.
> + except:
> + os.remove(tmpFilename)
> + raise
> +
> +def main():
> + # Parse arguments
> + import argparse
> + parser = argparse.ArgumentParser(
> + description=PARSERDESCR,
> + formatter_class=argparse.RawDescriptionHelpFormatter)
> + parser.add_argument('realm', help='Server authentication real')
> + parser.add_argument('-u', '--user', help='Set username', nargs='?')
Just delete nargs entirely, for reasons given upthread? Sorry for
missing that in my reviews between then and now.
> + args = parser.parse_args()
> +
> + # The file name is the md5encoding of the realm
> + import hashlib
> + m = hashlib.md5()
> + m.update(args.realm.encode('utf-8'))
> + authfileName = os.path.join(os.path.expanduser('~/.subversion/auth/svn.simple/'), m.hexdigest())
> +
> + # If new file, check that username was given as an argument before prompting for password
> + existingFile = os.path.exists(authfileName)
> + if not existingFile and args.user is None:
> + print('{}: New file, username required (see -u)\n'.print(argv[0]), file=sys.stderr)
The phrase "new file" in the comment isn't clear, probably because it
isn't a full sentence.
The phrase "New file" in the error message has the same problem, as well
as being phrased in implementation terms rather than in user-facing
terms.
> + parser.print_help()
> + return
The exit code should be non-zero whenever an error was printed to
stderr.
Aren't you reimplementing one of
https://docs.python.org/3/library/argparse.html#exiting-methods
?
Cheers,
Daniel
Re: A more permanent home for the add-a-password-to-a-cached-username
script? (was: Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password)
Posted by Daniel Sahlberg <da...@gmail.com>.
Attaching version 5, with changes as below.
Den sön 7 mars 2021 kl 20:42 skrev Daniel Shahaf <d....@daniel.shahaf.name>:
> Daniel Sahlberg wrote on Sun, Mar 07, 2021 at 19:34:15 +0100:
> > Den fre 26 feb. 2021 kl 16:11 skrev Daniel Shahaf <
> d.s@daniel.shahaf.name>:
> > > Daniel Sahlberg wrote on Fri, Feb 26, 2021 at 13:49:25 +0100:
> > > > + with open(filename + '.tmp', 'x', encoding='utf-8') as file:
> > >
> > > See my previous mail about adding 'w' and ensuring CR isn't emitted on
> > > Windows.
> > >
> >
> > Python doesn't like wx combined:
> >
> > Traceback (most recent call last):
> > File "./store-plaintext-password.py", line 112, in writeHashFile
> > with open(tmpFilename, 'wxb') as fd:
> > ValueError: must have exactly one of create/read/write/append mode
>
> I stand corrected.
>
> > > > +parser.add_argument('realm', help='Server authentication real')
> > > > +parser.add_argument('-u', '--user', help='Set username', nargs='?')
> > > > +args = parser.parse_args()
> > > > +
> > > > +# The file name is the md5encoding of the realm
> > > > +m = hashlib.md5()
> > > > +m.update(args.realm.encode('utf-8'))
> > > > +authfileName =
> os.path.join(os.path.expanduser('~/.subversion/auth/svn.simple/'),
> .hexdigest())
> > > > +
> > > > +# If new file, check that username was given as an argument before
> prompting for password
> > > > +if not os.path.isfile(authfileName) and args.user is None:
> > >
> > > «not isfile()» could mean it's a directory or a broken symlink. How
> > > about:
> > > .
> > > if isfile():
> > > elif not exists():
> > > else:
> > > .
> > > ?
> > >
> >
> > I switched to exists() only. I'm not sure it makes sense to care for
> every
> > possible variation - if it is a directory then open() will raise an
> > exception and I think that is acceptable in this case.
>
> +1
>
> > To reduce the number of stat() I'm storing the result form exists() in a
> > variable and reusing late. As for race condition... well it's not really
> > that kind of script where I could bother.
>
> Let's just say that "should X be done" and "could Daniel be bothered to
> do X" are not synonymous.
>
Agree. Patches welcome!
> > > (The point below about stat() applies to this pseudocode too.)
> > >
> > > > + print('New file, username required (see -u)\n', file=sys.stderr)
> > >
> > > Error messages should start with the argv[0] of whoever generated them.
> > >
> >
> > Point taken, but in this particular case it is displayed by print_help().
>
> Yes, but that's printed later. If this script is invoked by another,
> that's then invoked by another that's invoked by some CI, etc.,
> _prefixing_ the script's name to the message tends to be helpful.
>
Ok. Added now.
> > > +# In an existing file, we add/replace password/username/passtype
> > > > +if os.path.isfile(authfileName):
> > >
> > > Calling stat() twice has a performance impact and looks like a race
> > > condition. Maybe in this case it doesn't matter, though…
> > >
> >
> > Using the result from the previous check. Race condition, agree..
> >
> > > > +# For a new file, set realmstring, username, password and passtype
> > > > +else:
> > > > + hash = {}
> > > > + hash['svn:realmstring'] = args.realm
> > > > + hash['username'] = args.user
> > > > + hash['passtype'] = 'simple'
> > > > + hash['password'] = password
> > >
> > > Style: Use a single, multiline initializer rather than multiple
> > > assignments?
> > >
> >
> > Like this?
> > hash = {
> > b'svn:realmstring': args.realm.encode('utf-8'),
> > b'username': args.user.encode('utf-8'),
> > b'passtype': 'simple'.encode('utf-8'),
> > b'password': password.encode('utf-8')
> > }
>
> Yeah, except add a comma on the fourth line to minimize future diffs.
>
(y)
For future reference, there's also a «dict(foo=42)» syntax which is
> sometimes convenient.
>
> > > Forward cribbing compatibility: «del password» once it's no longer
> needed?
> > >
> >
> > Sorry, didn't quite understand this. Do you ask for an option to remove a
> > stored password? Isn't svn auth --remove enough?
>
> No, I meant literally add the line of code «del password» once the
> variable «password» is no longer needed. That won't actually overwrite
> the value's bytes in memory, but it will prevent the variable from being
> accidentally used afterwards.
>
(y)
> > +++ contrib/client-side/store-plaintext-password.py (working copy)
> > @@ -0,0 +1,184 @@
> > +def outputHash(fd, hash):
> > + """\
> > + Write a dictionary to an FD in the same format as used by
> > + svn_hash_write2(), ie 'K <length>\\n<key>\\nV <length>\\n<value>\\n'
> > + terminated by "END\n".
>
> Escape the newline.
>
(y)
> Mention the data type of HASH's keys and values?
>
(y)
> > + """
> > + assert 'b' in fd.mode
> > +
> > + for key, val in hash.items():
> > + fd.write(b'K ' + bytes(str(len(key)), 'utf-8') + b'\n')
> > + fd.write(key + b'\n')
> > + fd.write(b'V ' + bytes(str(len(val)), 'utf-8') + b'\n')
> > + fd.write(val + b'\n')
> > + fd.write(TERMINATOR)
> > +
> > +def writeHashFile(filename, hash):
> > + """\
> > + Open a temporary file and write the dictionary to the temp
> > + file in svn_hash_write2() format.
> > +
> > + If the file is successfully written it is renamed to <filename>.
> > + """
> > + tmpFilename = filename + '.tmp'
> > + try:
> > + with open(tmpFilename, 'xb') as fd:
> > + outputHash(fd, hash)
> > + os.rename(tmpFilename, filename)
> > + except FileExistsError:
> > + print('{} File {} already exist. Is the script already
> running?\n'.format(argv[0], tmpFilename), file=sys.stderr)
>
> s/{}/{}:/ (first time)
>
(y)
> s/{}/{!r}/ (second time) - in this case this will probably just add
> quotes, but still. (tl;dr: This prints the repr() of the formatee
> rather than its str().)
>
(y)
> Is there any point in naming the exception and printing something for it
> (probably one of its attributes)? I don't see a failure mode in which
> that would make a difference, but absence of evidence isn't evidence of
> absence.
>
Probably, and moving the error handling to a more "top level" position.
However I'm out of time this morning.
> + except:
> > + os.remove(tmpFilename)
> > + raise
> > +
> > +def main():
> > + """Main function"""
>
> I'm not a fan of docstrings that simply repeat what may be inferred from
> the identifier. I don't think there's much this function's docstring
> _could_ say, though. I'd consider removing the docstring entirely or
> having it use the term "entry point" so as to make it less tautological.
>
(y)
> > + # Parse arguments
> > + import argparse
> > + parser = argparse.ArgumentParser(
> ⋮
> > + # The file name is the md5encoding of the realm
> > + import hashlib
> > + m = hashlib.md5()
>
> Hmm. Should all function-local imports be grouped at the top of the
> function? I don't remember the convention here.
>
Is there a Subversion specific Style Guide for Python? I didn't find
anything in HACKING or trunk/doc.
PEP 8 says:
[[[
Imports are always put at the top of the file, just after any module
comments and docstrings, and before module globals and constants.
]]]
If I understand this correctly, it prefers global imports only? If keeping
the imports in the function, then moving to the top is probably more
PEP-8-ish. I didn't change anything though.
For reference, ordering should be
[[[
Imports should be grouped in the following order:
1. standard library imports
2. related third party imports
3. local application/library specific imports
]]]
No explicit mentioning of alphabetical sorting, but that seems like a good
idea.
(Offtopic: When reviewing /trunk/doc, I had a look at
/trunk/doc/programmer/WritingChangeLogs.txt. Is it still relevant? It seems
very CVS-ish and mostly covered in HACKING).
Kind regards,
Daniel Sahlberg
Re: A more permanent home for the
add-a-password-to-a-cached-username script? (was: Re: using svn cli with
--non-interactive (in scripts) securely, without exposing password)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Sahlberg wrote on Sun, Mar 07, 2021 at 19:34:15 +0100:
> Den fre 26 feb. 2021 kl 16:11 skrev Daniel Shahaf <d....@daniel.shahaf.name>:
> > Daniel Sahlberg wrote on Fri, Feb 26, 2021 at 13:49:25 +0100:
> > > + with open(filename + '.tmp', 'x', encoding='utf-8') as file:
> >
> > See my previous mail about adding 'w' and ensuring CR isn't emitted on
> > Windows.
> >
>
> Python doesn't like wx combined:
>
> Traceback (most recent call last):
> File "./store-plaintext-password.py", line 112, in writeHashFile
> with open(tmpFilename, 'wxb') as fd:
> ValueError: must have exactly one of create/read/write/append mode
I stand corrected.
> > > +parser.add_argument('realm', help='Server authentication real')
> > > +parser.add_argument('-u', '--user', help='Set username', nargs='?')
> > > +args = parser.parse_args()
> > > +
> > > +# The file name is the md5encoding of the realm
> > > +m = hashlib.md5()
> > > +m.update(args.realm.encode('utf-8'))
> > > +authfileName = os.path.join(os.path.expanduser('~/.subversion/auth/svn.simple/'), .hexdigest())
> > > +
> > > +# If new file, check that username was given as an argument before prompting for password
> > > +if not os.path.isfile(authfileName) and args.user is None:
> >
> > «not isfile()» could mean it's a directory or a broken symlink. How
> > about:
> > .
> > if isfile():
> > elif not exists():
> > else:
> > .
> > ?
> >
>
> I switched to exists() only. I'm not sure it makes sense to care for every
> possible variation - if it is a directory then open() will raise an
> exception and I think that is acceptable in this case.
+1
> To reduce the number of stat() I'm storing the result form exists() in a
> variable and reusing late. As for race condition... well it's not really
> that kind of script where I could bother.
Let's just say that "should X be done" and "could Daniel be bothered to
do X" are not synonymous.
> > (The point below about stat() applies to this pseudocode too.)
> >
> > > + print('New file, username required (see -u)\n', file=sys.stderr)
> >
> > Error messages should start with the argv[0] of whoever generated them.
> >
>
> Point taken, but in this particular case it is displayed by print_help().
Yes, but that's printed later. If this script is invoked by another,
that's then invoked by another that's invoked by some CI, etc.,
_prefixing_ the script's name to the message tends to be helpful.
> > > +# In an existing file, we add/replace password/username/passtype
> > > +if os.path.isfile(authfileName):
> >
> > Calling stat() twice has a performance impact and looks like a race
> > condition. Maybe in this case it doesn't matter, though…
> >
>
> Using the result from the previous check. Race condition, agree..
>
> > > +# For a new file, set realmstring, username, password and passtype
> > > +else:
> > > + hash = {}
> > > + hash['svn:realmstring'] = args.realm
> > > + hash['username'] = args.user
> > > + hash['passtype'] = 'simple'
> > > + hash['password'] = password
> >
> > Style: Use a single, multiline initializer rather than multiple
> > assignments?
> >
>
> Like this?
> hash = {
> b'svn:realmstring': args.realm.encode('utf-8'),
> b'username': args.user.encode('utf-8'),
> b'passtype': 'simple'.encode('utf-8'),
> b'password': password.encode('utf-8')
> }
Yeah, except add a comma on the fourth line to minimize future diffs.
For future reference, there's also a «dict(foo=42)» syntax which is
sometimes convenient.
> > Forward cribbing compatibility: «del password» once it's no longer needed?
> >
>
> Sorry, didn't quite understand this. Do you ask for an option to remove a
> stored password? Isn't svn auth --remove enough?
No, I meant literally add the line of code «del password» once the
variable «password» is no longer needed. That won't actually overwrite
the value's bytes in memory, but it will prevent the variable from being
accidentally used afterwards.
> +++ contrib/client-side/store-plaintext-password.py (working copy)
> @@ -0,0 +1,184 @@
> +def outputHash(fd, hash):
> + """\
> + Write a dictionary to an FD in the same format as used by
> + svn_hash_write2(), ie 'K <length>\\n<key>\\nV <length>\\n<value>\\n'
> + terminated by "END\n".
Escape the newline.
Mention the data type of HASH's keys and values?
> + """
> + assert 'b' in fd.mode
> +
> + for key, val in hash.items():
> + fd.write(b'K ' + bytes(str(len(key)), 'utf-8') + b'\n')
> + fd.write(key + b'\n')
> + fd.write(b'V ' + bytes(str(len(val)), 'utf-8') + b'\n')
> + fd.write(val + b'\n')
> + fd.write(TERMINATOR)
> +
> +def writeHashFile(filename, hash):
> + """\
> + Open a temporary file and write the dictionary to the temp
> + file in svn_hash_write2() format.
> +
> + If the file is successfully written it is renamed to <filename>.
> + """
> + tmpFilename = filename + '.tmp'
> + try:
> + with open(tmpFilename, 'xb') as fd:
> + outputHash(fd, hash)
> + os.rename(tmpFilename, filename)
> + except FileExistsError:
> + print('{} File {} already exist. Is the script already running?\n'.format(argv[0], tmpFilename), file=sys.stderr)
s/{}/{}:/ (first time)
s/{}/{!r}/ (second time) - in this case this will probably just add
quotes, but still. (tl;dr: This prints the repr() of the formatee
rather than its str().)
Is there any point in naming the exception and printing something for it
(probably one of its attributes)? I don't see a failure mode in which
that would make a difference, but absence of evidence isn't evidence of
absence.
> + except:
> + os.remove(tmpFilename)
> + raise
> +
> +def main():
> + """Main function"""
I'm not a fan of docstrings that simply repeat what may be inferred from
the identifier. I don't think there's much this function's docstring
_could_ say, though. I'd consider removing the docstring entirely or
having it use the term "entry point" so as to make it less tautological.
> + # Parse arguments
> + import argparse
> + parser = argparse.ArgumentParser(
⋮
> + # The file name is the md5encoding of the realm
> + import hashlib
> + m = hashlib.md5()
Hmm. Should all function-local imports be grouped at the top of the
function? I don't remember the convention here.
Cheers,
Daniel
Re: A more permanent home for the
add-a-password-to-a-cached-username script? (was: Re: using svn cli with
--non-interactive (in scripts) securely, without exposing password)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Sahlberg wrote on Fri, Feb 26, 2021 at 13:49:25 +0100:
> +++ contrib/client-side/store-plaintext-password.py (working copy)
> @@ -0,0 +1,127 @@
> +def readHashFile(filename):
> + """Read a hashfile as written by svn_hash_write2() to a list of tuples (key, value)"""
> + hash = {}
> + with open(authfileName, 'r', encoding='utf-8') as file:
> + while True:
> + # Expecting K [length] or END
> + line = file.readline()
> + if not line:
> + raise Exception('Parse failed, expected K [length] or END, got EOF')
> + line = line.strip()
> + if line.strip() == 'END':
> + if file.readline():
> + raise Exception('Parse failed, unexpected data after END')
> + return hash
> + elif line[:1] != 'K':
> + raise Exception('Parse failed, expected K [length]')
> +
> + # Read keyname
> + key = file.readline()
> + if not line:
> + raise Exception('Parse failed, expected keyname')
> + key = key.strip()
> + if len(key.encode('utf-8')) != int(line[2:]):
> + raise Exception('Parse failed, invalid key length {} expected {}'.format(len(key.encode('utf-8')), line[2:]))
> +
> + # Read V [length]
> + line = file.readline()
> + if not line:
> + raise Exception('Parse failed, expected V [length], got EOF')
> + line = line.strip()
> + if line[:1] != 'V':
> + raise Exception('Parse failed, expected V [length]')
> +
> + # Read value
> + value = file.readline()
> + if not value:
> + raise Exception('Parse failed, expected value')
> + value = value.strip()
> + if len(value.encode('utf-8')) != int(line[2:]):
> + raise Exception('Parse failed, invalid value length {} expected {}'.format(len(value.encode('utf-8')), line[2:]))
> +
> + # Store
> + hash[key] = value
Couldn't resist. Attached is my take of this function. Preview:
[[[
% ./svn_hash_read.py ~/.subversion/auth/svn.simple/d3c8a345b14f6a1b42251aef8027ab57
{b'svn:realmstring': b'<https://svn.apache.org:443> ASF Committers',
b'username': b'danielsh'}
]]]
The script should support everything the serialized hash format allows,
including binary data in both keys and values (even though we generally
use «const char *» keys). I'm sure there are a few nits to pick, though.
The code is runnable as-is, despite the ellipses.
Cheers,
Daniel
[[[
#!/usr/bin/env python3
TERMINATOR = b"END\n"
def _read_one_datum(fd, letter):
"""\
Read a 'K <length>\\n<key>\\n' or 'V <length>\\n<value>\\n' block from
an svn_hash_write2()-format FD.
LETTER identifies the first letter, as a bytes object.
"""
assert letter in {b'K', b'V'}
# Read the letter and the space
if fd.read(1) != letter or fd.read(1) != b' ':
raise ...
# Read the length and the newline
line = fd.readline()
if line[-1:] != b'\n':
raise ...
expected_length = int(line[:-1])
# Read the datum and its newline
datum = fd.read(expected_length)
if len(datum) != expected_length:
raise ...
if fd.read(1) != b'\n':
raise ...
return datum
def svn_hash_read(fd):
"""\
Read an svn_hash_write2()-formatted dict from FD, terminated by "END".
Return a dict mapping bytes to bytes.
"""
assert 'b' in fd.mode
assert TERMINATOR[0] not in {b'K', b'V'}
ret = {}
while True:
if fd.peek(1)[0] == TERMINATOR[0]:
if fd.readline() != TERMINATOR:
raise ...
if fd.peek(1):
raise ...
return ret
key = _read_one_datum(fd, b'K')
value = _read_one_datum(fd, b'V')
ret[key] = value
def main():
import sys
parsed = svn_hash_read(open(sys.argv[1], 'rb'))
import pprint
pprint.pprint(parsed)
if __name__ == '__main__':
main()
]]]
Re: A more permanent home for the add-a-password-to-a-cached-username
script? (was: Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password)
Posted by Daniel Sahlberg <da...@gmail.com>.
I finally found some time to work on this, sorry for the delay!
I'm attaching v4 where I blatantly stole Danielsh's script to read the hash
file
(
http://mail-archives.apache.org/mod_mbox/subversion-dev/202102.mbox/%3c20210226173525.GA24828@tarpaulin.shahaf.local2%3e
)
and addressed the other comments below. I appreciate the "lead by example"
and I've tried to follow the best I could.
Den fre 26 feb. 2021 kl 16:11 skrev Daniel Shahaf <d....@daniel.shahaf.name>:
> Daniel Sahlberg wrote on Fri, Feb 26, 2021 at 13:49:25 +0100:
> > +++ contrib/client-side/store-plaintext-password.py (working copy)
> > @@ -0,0 +1,127 @@
> > +import hashlib
> > +import argparse
> > +import os
> > +import getpass
> > +import sys
>
> For bonus points, sort these. PEP 8 doesn't seem to mention this.
>
I've moved the imports to where they are being used (for most part)
> > +def readHashFile(filename):
>
[...] since the code to read the hash is based on Danielsh's script I will
not make any comments on this part of the rewiew, however I'm again
grateful for the feedback and I'll try to remember next time.
> > + # Store
> > + hash[key] = value
> > +
> > +def writeHashFile(filename, hash):
> > + """Write a list of tuples (key, value) to a hashfile of the same
> format as svn_hash_write2()"""
>
> Same issue with the docstring.
>
This is adjusted. And I've also considered your commment about using FDs as
arguments and split the write function in two:
- one to open the tmp file (and handle FileExistsError) and the renaming
afterwards
- one accepting the FD and the hash and writing the actual content
>
> > + with open(filename + '.tmp', 'x', encoding='utf-8') as file:
>
> See my previous mail about adding 'w' and ensuring CR isn't emitted on
> Windows.
>
Python doesn't like wx combined:
Traceback (most recent call last):
File "./store-plaintext-password.py", line 112, in writeHashFile
with open(tmpFilename, 'wxb') as fd:
ValueError: must have exactly one of create/read/write/append mode
The CR issue should be alright by using b mode, inspired by your code
reading in b mode
> +# Parse arguments
> > +parser = argparse.ArgumentParser(description='''Store plain-text
> password in ~/.subversion/auth/svn.simple/\n\n
> > +Existing passwords and authentication realms can be inspected by:\n\n
> > + svn auth [--show-passwords]\n\n
> > +The authentication realm can also be found using:\n\n
> > + svn info URL\n\n
> > +Realm will be read from stdin if not provided on the command line.''',
> formatter_class=argparse.RawDescriptionHelpFormatter)
>
> Don't embed multiline string literals this way. Either make it a global
> variable, or indent the named actual parameters conventionally:
>
> parser = argparse.ArgumentParser(
> description='''\
> Store plain-text password in ~/.subversion/auth/svn.simple/
> ⋮
> Realm will be read from stdin if not provided on the command line.
> ''',
> formatter_class=argparse.RawDescriptionHelpFormatter,
> )
>
> I'm not sure whether you can indent the string literal's contents there
> and still have it output as desired.
>
> Note you have three \n's between every two lines (two escaped and one
> literal).
>
Indenting the text kept the indentation in the output, doesn't look good.
Moved this to a global variable and removed the escaped \n.
> > +parser.add_argument('realm', help='Server authentication real')
> > +parser.add_argument('-u', '--user', help='Set username', nargs='?')
> > +args = parser.parse_args()
> > +
> > +# The file name is the md5encoding of the realm
> > +m = hashlib.md5()
> > +m.update(args.realm.encode('utf-8'))
> > +authfileName =
> os.path.join(os.path.expanduser('~/.subversion/auth/svn.simple/'),
> m.hexdigest())
> > +
> > +# If new file, check that username was given as an argument before
> prompting for password
> > +if not os.path.isfile(authfileName) and args.user is None:
>
> «not isfile()» could mean it's a directory or a broken symlink. How
> about:
> .
> if isfile():
> elif not exists():
> else:
> .
> ?
>
I switched to exists() only. I'm not sure it makes sense to care for every
possible variation - if it is a directory then open() will raise an
exception and I think that is acceptable in this case.
The check is here because I want to give the error message "username
required" before asking for the password.
To reduce the number of stat() I'm storing the result form exists() in a
variable and reusing late. As for race condition... well it's not really
that kind of script where I could bother.
(The point below about stat() applies to this pseudocode too.)
>
> > + print('New file, username required (see -u)\n', file=sys.stderr)
>
> Error messages should start with the argv[0] of whoever generated them.
>
Point taken, but in this particular case it is displayed by print_help().
>
> > + parser.print_help()
> > + quit()
> > +
> > +# Prompt for password
> > +password = getpass.getpass("Enter password: ")
>
> How about "Enter password for {realm}" or "for {user}", etc?
>
"for {realm}"
>
> > +# In an existing file, we add/replace password/username/passtype
> > +if os.path.isfile(authfileName):
>
> Calling stat() twice has a performance impact and looks like a race
> condition. Maybe in this case it doesn't matter, though…
>
Using the result from the previous check. Race condition, agree..
>
> > + hash = readHashFile(authfileName)
> > + if args.user is not None:
> > + hash['username'] = args.user
> > + hash['password'] = password
> > + hash['passtype'] = 'simple'
> > +
> > +# For a new file, set realmstring, username, password and passtype
> > +else:
> > + hash = {}
> > + hash['svn:realmstring'] = args.realm
> > + hash['username'] = args.user
> > + hash['passtype'] = 'simple'
> > + hash['password'] = password
>
> Style: Use a single, multiline initializer rather than multiple
> assignments?
>
Like this?
hash = {
b'svn:realmstring': args.realm.encode('utf-8'),
b'username': args.user.encode('utf-8'),
b'passtype': 'simple'.encode('utf-8'),
b'password': password.encode('utf-8')
}
> Forward cribbing compatibility: «del password» once it's no longer needed?
>
Sorry, didn't quite understand this. Do you ask for an option to remove a
stored password? Isn't svn auth --remove enough?
> > +
> > +# Write out the resulting file
> > +writeHashFile(authfileName, hash)
>
Kind regards
Daniel Sahlberg
Re: A more permanent home for the
add-a-password-to-a-cached-username script? (was: Re: using svn cli with
--non-interactive (in scripts) securely, without exposing password)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Sahlberg wrote on Fri, Feb 26, 2021 at 13:49:25 +0100:
> +++ contrib/client-side/store-plaintext-password.py (working copy)
> @@ -0,0 +1,127 @@
> +import hashlib
> +import argparse
> +import os
> +import getpass
> +import sys
For bonus points, sort these. PEP 8 doesn't seem to mention this.
> +def readHashFile(filename):
> + """Read a hashfile as written by svn_hash_write2() to a list of tuples (key, value)"""
This returns a dict, not a list of tuples.
> + hash = {}
> + with open(authfileName, 'r', encoding='utf-8') as file:
> + while True:
> + # Expecting K [length] or END
> + line = file.readline()
> + if not line:
> + raise Exception('Parse failed, expected K [length] or END, got EOF')
Rule of thumb: every error message should contain at least one
replaceable. (Why? To identify the context, the data at fault, etc.)
In this case, it might be easiest to wrap the whole block with
a try/except that just adds the filename to the error message.
Which reminds me: in idiomatic Python, functions are passed open
file-like objects, rather than filenames. This way any file-like object
can be passed (e.g., those returned by gzip.open() or
urllib2.urlopen()). Not asking to change this; just FYI.
> + line = line.strip()
> + if line.strip() == 'END':
> + if file.readline():
> + raise Exception('Parse failed, unexpected data after END')
> + return hash
> + elif line[:1] != 'K':
There's line.startswith('K'), but up to you whether to change it.
> + # Read value
> + value = file.readline()
> + if not value:
> + raise Exception('Parse failed, expected value')
> + value = value.strip()
s/strip()/rstrip('\n')/ to allow passwords with trailing whitespace?
> + if len(value.encode('utf-8')) != int(line[2:]):
> + raise Exception('Parse failed, invalid value length {} expected {}'.format(len(value.encode('utf-8')), line[2:]))
OK: This length check will cause the script to abort if someone has
a password that ends with whitespace, or a custom key whose value isn't
just one line, and so on.
I'm not sure file.readline().encode('utf-8') is guaranteed to be the
same number of octets as there are octets in the file between the two
successive newlines (even assuming there isn't any leading/trailing
whitespace). This could happen in two cases: if .decode()/.encode() do
silent normalization (to NFC or to NFD), or if there are two different
sequences of octets that UTF-8 maps to the same codepoint.
> + # Store
> + hash[key] = value
> +
> +def writeHashFile(filename, hash):
> + """Write a list of tuples (key, value) to a hashfile of the same format as svn_hash_write2()"""
Same issue with the docstring.
> + with open(filename + '.tmp', 'x', encoding='utf-8') as file:
See my previous mail about adding 'w' and ensuring CR isn't emitted on
Windows.
> +# Parse arguments
> +parser = argparse.ArgumentParser(description='''Store plain-text password in ~/.subversion/auth/svn.simple/\n\n
> +Existing passwords and authentication realms can be inspected by:\n\n
> + svn auth [--show-passwords]\n\n
> +The authentication realm can also be found using:\n\n
> + svn info URL\n\n
> +Realm will be read from stdin if not provided on the command line.''', formatter_class=argparse.RawDescriptionHelpFormatter)
Don't embed multiline string literals this way. Either make it a global
variable, or indent the named actual parameters conventionally:
parser = argparse.ArgumentParser(
description='''\
Store plain-text password in ~/.subversion/auth/svn.simple/
⋮
Realm will be read from stdin if not provided on the command line.
''',
formatter_class=argparse.RawDescriptionHelpFormatter,
)
I'm not sure whether you can indent the string literal's contents there
and still have it output as desired.
Note you have three \n's between every two lines (two escaped and one
literal).
> +parser.add_argument('realm', help='Server authentication real')
> +parser.add_argument('-u', '--user', help='Set username', nargs='?')
> +args = parser.parse_args()
> +
> +# The file name is the md5encoding of the realm
> +m = hashlib.md5()
> +m.update(args.realm.encode('utf-8'))
> +authfileName = os.path.join(os.path.expanduser('~/.subversion/auth/svn.simple/'), m.hexdigest())
> +
> +# If new file, check that username was given as an argument before prompting for password
> +if not os.path.isfile(authfileName) and args.user is None:
«not isfile()» could mean it's a directory or a broken symlink. How
about:
.
if isfile():
elif not exists():
else:
.
?
(The point below about stat() applies to this pseudocode too.)
> + print('New file, username required (see -u)\n', file=sys.stderr)
Error messages should start with the argv[0] of whoever generated them.
> + parser.print_help()
> + quit()
> +
> +# Prompt for password
> +password = getpass.getpass("Enter password: ")
How about "Enter password for {realm}" or "for {user}", etc?
> +# In an existing file, we add/replace password/username/passtype
> +if os.path.isfile(authfileName):
Calling stat() twice has a performance impact and looks like a race
condition. Maybe in this case it doesn't matter, though…
> + hash = readHashFile(authfileName)
> + if args.user is not None:
> + hash['username'] = args.user
> + hash['password'] = password
> + hash['passtype'] = 'simple'
> +
> +# For a new file, set realmstring, username, password and passtype
> +else:
> + hash = {}
> + hash['svn:realmstring'] = args.realm
> + hash['username'] = args.user
> + hash['passtype'] = 'simple'
> + hash['password'] = password
Style: Use a single, multiline initializer rather than multiple
assignments?
Forward cribbing compatibility: «del password» once it's no longer needed?
> +
> +# Write out the resulting file
> +writeHashFile(authfileName, hash)
Re: A more permanent home for the add-a-password-to-a-cached-username
script? (was: Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password)
Posted by Daniel Sahlberg <da...@gmail.com>.
Den fre 26 feb. 2021 kl 08:31 skrev Daniel Sahlberg <
daniel.l.sahlberg@gmail.com>:
> Moving from users@ to dev@
>
[...]
I was in a bit of a hurry this morning and didn't test properly. The
previous version (store-plaintext-passwords2.txt) contained a number of
bugs which I hope I've corrected here:
- Didn't properly encode keys/values before checking length. Non-ascii
characters in the values raised an exception.
- Didn't properly add the lenght to K when writing the hash file (added the
key name instead)
- Reorganized prompting for password and checking for new file and the -u
argument to show the error message before prompting for a password [UX
improvement]
New version attached.
Kind regards,
Daniel
Re: A more permanent home for the
add-a-password-to-a-cached-username script? (was: Re: using svn cli with
--non-interactive (in scripts) securely, without exposing password)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Sahlberg wrote on Fri, Feb 26, 2021 at 08:31:55 +0100:
> Den tors 25 feb. 2021 kl 08:00 skrev Daniel Shahaf <d....@daniel.shahaf.name>:
>
> > Daniel Sahlberg wrote on Wed, Feb 24, 2021 at 23:27:18 +0100:
> > > Learning Python has been on my todo list for ages, so I've cobbled
> > > together something that seems to do the job.
> >
> > ☺
> >
>
> Thank you very much for your detailed feedback! Based on it I more or less
> did a ground-up rewrite, sorry for forcing you to restart the review from
> scratch. I will address your commets below.
No worries.
> > > +# K [length]
> > > +# [keyname]
> > > +# V [length]
> > > +# [value]
> > > +# ...
> > > +# END
> > > +#
> > > +# The length is not validated
> >
> > It's not just "not validated"; it's not used at all. The file format is
> > binary — the lengths are in octets — but the Python code below treats
> > the file as a line-based format. That may be fine for now, but is not
> > forward-compatible with ~/.subversion/auth/ files that may be created by
> > future versions.
> >
>
> Length is now validated.
>
> I think it is acceptable that we make some assumptions about the format. If
> some future version would make a more complex format, the script can be
> updated. (This part of the code is removed, but the comment is relevant for
> the section updating an existing file).
*nod*
> > I'm not sure what level of the API (public or private) third-party
> > products would need to operate at in order to add their own custom keys
> > to the cache files. I don't know of anyone doing that, but it's not
> > impossible.
> >
>
> Switched to us a dictionary (but the old code should also have been
> agnostic of key ordering).
I was concerned not of key ordering but about cache files that have more
than just the standard set of keys. In particular, such custom keys
aren't necessarily restricted to texty, single-line values.
> > > +# Write a key/value pair to a hashfile
> > > +def writeHash(file, key, val):
> >
> > The docstring should be a string literal as the first statement in the
> > function, as in subversion/tests/cmdline/. That then becomes the
> > function's __doc__ attribute.
> >
>
> Fixed!
Thanks. That's a thing for the top-of-file comment too, by the way. We
aren't consistent about this, but ezt.py and run_tests.py both do this.
> >
> > > + file.write('K ' + str(len(key)) + '\n')
> > > + file.write(key + '\n')
> >
> > str() gives the length in characters, but the file format demands
> > a length in octets, so this would break as soon as somebody puts
> > non-ASCII in their password — which, given the nature of passwords,
> > is plausible.
> >
> > You'll want to convert from str to bytes. See str.encode(). You'll
> > need to use b'' literals in concatenations.
> >
>
> Better now?
Better, yes. However, I think as it stands in v3, it'll write CRLF on
Windows? I.e., don't you need 'b' in open()'s mode=* argument?
> > +if args.listpassword or args.list:
> > > + data = []
> > > + if args.listpassword:
> > > + header = ['Realm', 'Username', 'Password']
> > > + else:
> > > + header = ['Realm', 'Username']
> > > + for name in listdir(authpath):
> > > + fullname = join(authpath, name)
> > > + if isfile(fullname):
> > > + file = open(fullname, 'r')
> > > + realm = ''
> > > + user = ''
> > > + password = ''
> > > + while(True):
> >
> > The parentheses aren't idiomatic.
> >
>
> C-sick. ;-)
Then write a C-shell script to upload C-metric eggs to C-pan? I'm sure
I can come up with a cpan(1) invocation that will break a egg… :P
(https://www.cpan.org/; https://packaging.python.org/glossary/#term-Egg)
> > + parser.print_help()
> > > + quit()
> >
> > This will NameError on 'quit'. At least the exit code will be non-zero
> > this way.
That's still applicable to v3.
> > > +# In an existing file, we add/replace password/username/passtype
> >
> > This bit of code conflates several distinct pieces of logic:
> >
> > - Parsing a serialized hash file
> >
> > - Serializing a hash
> >
> > - The actual patching of the hash
> >
> > - Implementing editing a file by an atomic rename
> >
> > I can't say I like this at all. All these concerns should be cleanly
> > separated from one another.
> >
>
> The rewrite first read the complete hash to a dictionary. Then edit the
> dictionary. Finally writing the dictionary to a new file.
Much better.
> > +if isfile(fullname):
> > > + pwdAdded = False
> > > + passtypeAdded = False
> > > + usernameAdded = False
> > > +
> > > + # Read existing file, write to .tmp which we rename in the end
> > > + inFile = open(fullname, 'r')
> > > + outFile = open(fullname + '.tmp', 'w')
> >
> > Also 'x' please for O_EXCL. If nothing else, this'll plug a race
> > condition when two instances of this script run simultaneously.
> >
>
> Fixed.
Thanks. You changed it to 'x'; shouldn't the 'w' be retained as well,
i.e., 'wx' (and 'wxb' in combination with a previous point)? O_EXCL
doesn't imply either O_RDWR or O_WRONLY.
> Thanks again for your review!
You're welcome. I see you've posted v3 so I'll skip reviewing v2.
Cheers,
Daniel
Re: A more permanent home for the add-a-password-to-a-cached-username
script? (was: Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password)
Posted by Daniel Sahlberg <da...@gmail.com>.
Moving from users@ to dev@
Den tors 25 feb. 2021 kl 08:00 skrev Daniel Shahaf <d....@daniel.shahaf.name>:
> Daniel Sahlberg wrote on Wed, Feb 24, 2021 at 23:27:18 +0100:
> > Learning Python has been on my todo list for ages, so I've cobbled
> > together something that seems to do the job.
>
> ☺
>
Thank you very much for your detailed feedback! Based on it I more or less
did a ground-up rewrite, sorry for forcing you to restart the review from
scratch. I will address your commets below.
> > There are basically two modes of operation:
> >
> > ./store-plaintext-password.py --listpassword|--list
> >
> > Which lists all cached realms/usernames (and passwords). I've
> intentionally
> > split it in two in case someone prefer not to put the password on-screen,
> > but I've also added the option to display to show that the password is
> > really in plain text.
>
> Isn't this what `svn auth` does?
>
> Also, "do one thing and do it well". Reasons:
>
> - So users have less code to audit before they run it.
>
> - Extensibility of interface
>
> - Clarity of interface (cf. «svn switch --relocate»)
>
Oops, missed that one. I've removed the --list/--listpassword options and
instead hinted at svn auth in the help text.
> ./store-plaintext-password.py realm password [username]
> >
> > Which stores/updates the password for the given realm. If username is
> given
> > it is stored/updated as well. If there is no cached entry for the
> specified
> > realm a new file will be created (and username is a mandatory argument).
>
> Passwords shouldn't be passed in argv because the values in argv are
> visible to other non-root users.
>
I've switched to read password using getpass.getpass(). The realm and
username is still passed on the command line.
> TODO:
> > - Is the license ok? I stole it from svn.c
>
> I assume you're asking about the header format as opposed to the choice
> of license. Sure, svn.c's header ought to be fine.
>
> > - Improve Python code
>
> Reviewing.
>
Thanks! I've taken note of your suggestions regarding the code related to
--list|--listpassword but since that code has been removed I'm not
considering it here. Sorry for the extra work!
> > - Improve documentation - especially on where to find the 'realm' string
>
> For one, by running `svn info` interactively and ^C'ing it at the prompt.
>
Added!
> > - Decide where to put it - I've gone with contrib for now as per Nathan's
> > and Daniel's suggestions
>
> This was a patch submission and my reply contains a review. We may want
> to move to dev@.
>
> > > > > Regarding the FAQ,
...cut...
Nathan made a very good suggestion about the FAQ in a separate post,
incorporating both my ideas and danielsh's feedback. I'll review his
suggestion and post a reply there (if any).
> +++ contrib/client-side/store-plaintext-password.py (working copy)
> > @@ -0,0 +1,207 @@
> > +#!/usr/bin/env python3
> > +
> > +# Script to store password in plaintext in
> ~/.subversion/auth/svn.simple/
> > +#
> > +# Script require the tabulate module: pip install tabulate
>
> s/require/requires/
>
> More importantly, it would be better not to require non-stdlib modules,
> at least in the "save passwords" codepath. The easiest way to do this
> would be to move the «import» statement into the function that needs it.
>
Noted (but code removed).
> > +# ====================================================================
> > +# Licensed to the Apache Software Foundation (ASF) under one
> > +# or more contributor license agreements. See the NOTICE file
> > +# distributed with this work for additional information
> > +# regarding copyright ownership. The ASF licenses this file
> > +# to you under the Apache License, Version 2.0 (the
> > +# "License"); you may not use this file except in compliance
> > +# with the License. You may obtain a copy of the License at
> > +#
> > +# http://www.apache.org/licenses/LICENSE-2.0
> > +#
> > +# Unless required by applicable law or agreed to in writing,
> > +# software distributed under the License is distributed on an
> > +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
> > +# KIND, either express or implied. See the License for the
> > +# specific language governing permissions and limitations
> > +# under the License.
> > +# ====================================================================
> > +
> > +from hashlib import md5
> > +import argparse
> > +import os
> > +from os import listdir
> > +from os.path import isfile, join
>
> These two imports aren't very idiomatic, at least not in Subversion's
> use of Python. We tend to just call these functions by their
> fully-qualified names.
>
Googled these up :-) Switched to import the lib and use fully-qualified
names.
> > +from pathlib import Path
> > +from tabulate import tabulate
> > +
> > +# Read a hashfile and return a key/value list
> > +#
>
> Err, what? The function reads the hashfile and returns the first
> key-value pair in it as a list. It doesn't read any subsequent
> key-value pairs, which is what this sentence seems to imply.
>
> Also, the function doesn't ensure the 'END\n' terminator and EOF happen
> where they're expected.
>
> Also, don't rely on the key-value pairs in the hash file being in any
> particular order.
>
> > +# A hashfile should contain the following four lines, repeted as
> needed, ending by the line END
>
> "repeated" is misspelled. Also, this should reference the upstream docs
> (svn_hash_write2()), either in addition to or instead of repeating them.
>
Added this reference
> > +# K [length]
> > +# [keyname]
> > +# V [length]
> > +# [value]
> > +# ...
> > +# END
> > +#
> > +# The length is not validated
>
> It's not just "not validated"; it's not used at all. The file format is
> binary — the lengths are in octets — but the Python code below treats
> the file as a line-based format. That may be fine for now, but is not
> forward-compatible with ~/.subversion/auth/ files that may be created by
> future versions.
>
Length is now validated.
I think it is acceptable that we make some assumptions about the format. If
some future version would make a more complex format, the script can be
updated. (This part of the code is removed, but the comment is relevant for
the section updating an existing file).
I'm not sure what level of the API (public or private) third-party
> products would need to operate at in order to add their own custom keys
> to the cache files. I don't know of anyone doing that, but it's not
> impossible.
>
Switched to us a dictionary (but the old code should also have been
agnostic of key ordering).
> +def readHash(file):
> > + # Read K [length] or END but discard it
> > + key = file.readline().strip()
> > + if key == 'END':
> > + return None
> > + if key[:1] != 'K':
> > + raise Exception('Parse failed, expected K...')
> > +
> > + # Read keyname
> > + key = file.readline().strip()
> > +
> > + # Read V [length] but discard it
> > + val = file.readline().strip()
> > + if val[:1] != 'V':
> > + raise Exception('Parse failed, expected V...')
> > +
> > + # Read value
> > + val = file.readline().strip()
> > +
> > + return [key, val]
>
> Returning a tuple would be more idiomatic.
>
Noted (but code removed)!
> > +# Write a key/value pair to a hashfile
> > +def writeHash(file, key, val):
>
> The docstring should be a string literal as the first statement in the
> function, as in subversion/tests/cmdline/. That then becomes the
> function's __doc__ attribute.
>
Fixed!
>
> > + file.write('K ' + str(len(key)) + '\n')
> > + file.write(key + '\n')
>
> str() gives the length in characters, but the file format demands
> a length in octets, so this would break as soon as somebody puts
> non-ASCII in their password — which, given the nature of passwords,
> is plausible.
>
> You'll want to convert from str to bytes. See str.encode(). You'll
> need to use b'' literals in concatenations.
>
Better now?
> + file.write('V ' + str(len(val)) + '\n')
> > + file.write(val + '\n')
> > +
> > +# Main function
> > +parser = argparse.ArgumentParser(description='Store plain text password
> in ~/.subversion/auth/svn.simple/')
> > +parser.add_argument('realm', help='Realm string from server, required
> value for updates', nargs='?')
> > +parser.add_argument('password', help='Password, required value for
> updates', nargs='?')
>
> IIRC, nargs='?' means that both ['store-plaintext-password.py',
> '--password'] and ['store-plaintext-password.py', '--password', 'foo']
> are valid uses, which isn't what's needed here.
>
I've rebuilt the argument handling to specify -u (to store/update a
username), pass the realm in a positional argument and always read the
password using getpass().
> > +parser.add_argument('username', help='Username, optional value for
> updates', nargs='?')
> > +parser.add_argument('--list', help='Lists all stored realms/usernames',
> action='store_true')
> > +parser.add_argument('--listpassword', help='Lists all stored
> realms/usernames/passwords', action='store_true')
> > +
> > +args = parser.parse_args()
> > +authpath = str(Path.home()) + '/.subversion/auth/svn.simple/'
>
> Just use os.path.expanduser() directly? You don't need pathlib.Path's
> bells and whistles.
>
Switched to os.path.expanduser() (and removed the import of pathlib).
> +if args.listpassword or args.list:
> > + data = []
> > + if args.listpassword:
> > + header = ['Realm', 'Username', 'Password']
> > + else:
> > + header = ['Realm', 'Username']
> > + for name in listdir(authpath):
> > + fullname = join(authpath, name)
> > + if isfile(fullname):
> > + file = open(fullname, 'r')
> > + realm = ''
> > + user = ''
> > + password = ''
> > + while(True):
>
> The parentheses aren't idiomatic.
>
C-sick. ;-)
> > + hash = readHash(file)
>
> So this just slurps the file, rather than return each keypair
> iteratively (with «yield»). Good enough for the present use-case.
>
Yes, that's a correct analysis.
> + if hash is None:
> > + break
> > + elif hash[0] == 'svn:realmstring':
> > + realm = hash[1]
> > + elif hash[0] == 'username':
> > + user = hash[1]
> > + elif hash[0] == 'password':
> > + password = hash[1]
> > + file.close()
> > + if args.listpassword:
> > + data.append([realm, user, password])
>
> This sounds like it could become a list of collections.namedtuple
> instance instances (sic), but as mentioned above, this entire function
> seems a reinvention of `svn auth`.
>
And thus removed.
> > + else:
> > + data.append([realm, user])
> > + print(tabulate(data, headers=header))
> > + quit()
> > +
> > +if args.realm is None:
> > + print("Realm required\n")
>
> Errors should go to stderr, and begin with the name (argv[0]) of whoever
> generated them.
>
> Any reason not to let argparse enforce the mandatoriness of the argument?¨
>
Nothing more than not being able to make up my mind about the command line
arguements. No realm is mandatory (positional) and -u/-p flags.
> + parser.print_help()
> > + quit()
>
> This will NameError on 'quit'. At least the exit code will be non-zero
> this way.
>
> > +if args.password is None:
> > + print("Password required\n")
> > + parser.print_help()
> > + quit()
> > +
> > +# The file name is the md5encoding of the realm
> > +m = md5()
> > +m.update(args.realm.encode('utf-8'))
> > +fullname = join(authpath, m.hexdigest())
> > +
> > +# In an existing file, we add/replace password/username/passtype
>
> This bit of code conflates several distinct pieces of logic:
>
> - Parsing a serialized hash file
>
> - Serializing a hash
>
> - The actual patching of the hash
>
> - Implementing editing a file by an atomic rename
>
> I can't say I like this at all. All these concerns should be cleanly
> separated from one another.
>
The rewrite first read the complete hash to a dictionary. Then edit the
dictionary. Finally writing the dictionary to a new file.
> +if isfile(fullname):
> > + pwdAdded = False
> > + passtypeAdded = False
> > + usernameAdded = False
> > +
> > + # Read existing file, write to .tmp which we rename in the end
> > + inFile = open(fullname, 'r')
> > + outFile = open(fullname + '.tmp', 'w')
>
> Also 'x' please for O_EXCL. If nothing else, this'll plug a race
> condition when two instances of this script run simultaneously.
>
Fixed.
> + while(True):
> > + # Read K [length] or END and write back
> > + line = inFile.readline()
> > + if not line:
> > + raise Exception('Parse failed, expected K ... or END')
> > + if line.strip() == 'END':
>
> Duplicates logic from earlier in the file.
>
> > + # If username, password and/or passtype has not been found
> in the file, add them here
> > + if not usernameAdded and args.username is not None:
> > + writeHash(outFile, 'username', args.username)
> > + if not passtypeAdded:
> > + writeHash(outFile, 'passtype', 'simple')
>
> Leaky abstraction: this layer needn't know that the file format
> serialized keys and values in the order K1 V1 K2 V2 … as opposed to,
> say, K1 K2 … V1 V2 ….
>
> > + if not pwdAdded:
> > + writeHash(outFile, 'password', args.password)
> > + outFile.write(line)
> > + break
> > + outFile.write(line)
> > +
> > + # Read keyname and write back, save keyname for later
> > + line = inFile.readline()
> > + outFile.write(line)
>
> Input is used unvalidated.
>
> > + elif key == 'passtype':
> > + outFile.write('V 6\n')
>
> Magic number.
>
Thanks again for your review!
Kind regards
Daniel Sahlberg
Re: A more permanent home for the
add-a-password-to-a-cached-username script? (was: Re: using svn cli with
--non-interactive (in scripts) securely, without exposing password)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Sahlberg wrote on Wed, Feb 24, 2021 at 23:27:18 +0100:
> Learning Python has been on my todo list for ages, so I've cobbled
> together something that seems to do the job.
☺
> There are basically two modes of operation:
>
> ./store-plaintext-password.py --listpassword|--list
>
> Which lists all cached realms/usernames (and passwords). I've intentionally
> split it in two in case someone prefer not to put the password on-screen,
> but I've also added the option to display to show that the password is
> really in plain text.
Isn't this what `svn auth` does?
Also, "do one thing and do it well". Reasons:
- So users have less code to audit before they run it.
- Extensibility of interface
- Clarity of interface (cf. «svn switch --relocate»)
> ./store-plaintext-password.py realm password [username]
>
> Which stores/updates the password for the given realm. If username is given
> it is stored/updated as well. If there is no cached entry for the specified
> realm a new file will be created (and username is a mandatory argument).
Passwords shouldn't be passed in argv because the values in argv are
visible to other non-root users.
> TODO:
> - Is the license ok? I stole it from svn.c
I assume you're asking about the header format as opposed to the choice
of license. Sure, svn.c's header ought to be fine.
> - Improve Python code
Reviewing.
> - Improve documentation - especially on where to find the 'realm' string
For one, by running `svn info` interactively and ^C'ing it at the prompt.
> - Decide where to put it - I've gone with contrib for now as per Nathan's
> and Daniel's suggestions
This was a patch submission and my reply contains a review. We may want
to move to dev@.
> > > > Regarding the FAQ, currently we have [1] "Ahhh! I just discovered that
> > > > my Subversion client is caching passwords in plain-text on disk!
> > > > AHHH!" That is still applicable to 1.10, but now we need an entry to
> > > > answer the opposite question: how to cache the password for svn use
> > > > with cron jobs and non-X environments where Kwallet and GNOME-Keyring
> > > > aren't applicable, and the particularly annoying case in which the
> > > > machine itself has a GUI but the user is logged in via ssh; in this
> > > > case the svn client will "freeze" while waiting for password entry in
> > > > an inaccessible GUI window; I think this would occur with Kwallet,
> > > > GNOME-Keyring, and macOS's Keychain.)
> > > >
> > > > But, as there doesn't seem to be one well-established way to handle
> > > > this, other than just storing the password on disk, would the new FAQ
> > > > entry say just that? Do we have any other concrete suggestions?
> >
> > If a cron job needs authentication, its credentials need to be stored
> > somewhere, either in plaintext or in "as good as" plaintext. I think
> > storing the passwords in unobfuscated plaintext was a deliberate
> > decision, informed by CVS's design choices in this regard, but I wasn't
> > around in the early days.
> >
> > On the other hand, for GUI-less environments and for headful
> > environments ssh'd into, we should simply document the capabilities and
> > workarounds of the libraries we use (possibly by pointing to those
> > libraries' documentations).
> >
>
> Suggestion for new FAQ entry:
> [[[
> Ahhh! I just discovered that my Subversion client is NOT caching passwords
> in plain-text on disk! AHHH!
Having two entry titles that differ only by a "not" isn't a good idea.
> Calm down, take a deep breath.
>
> This is the opposite of the previous question. After changing the compile
> time default to not store passwords in plain-text there has been a number
> of requests in the mailing lists to store the password.
"A number of requests on the mailing lists" seems like too fine a level
of abstraction. I think the context basically needs to be "The default
is X but you may want Y; here's how to do that".
> If you understand the security implications, you have ruled out other
s/, you/, /
> alternatives and still want to cache your password in plain-text on disk
s/ and/, and/
> you can use the script
> https://svn.apache.org/repos/asf/subversion/trunk/contrib/client-side/store-plaintext-password-py
> to store the password in the directory which contains the cached passwords
> (usually ~/.subversion/auth/). The script can also be used to list any
> existing passwords stored in plain-text.
This should just point to `svn auth`, surely?
Should this explicitly say to run the script with --help to get its
usage message?
> ]]]
>
> I'm also suggesting to change the existing FAQ entry (Ahhh! I just
> discovered that my Subversion client is caching passwords in plain-text on
> disk! AHHH!) to mention the changed compile time default since 1.12 to not
> store plain-text passwords:
>
> [[[
> s/Otherwise, the client will fall back/Otherwise, the client can fall back/
>
> Since svn 1.12 the compile time default has been to disable storing new
> passwords in plain-text, but old passwords can still be used. Certain
> distributions may also have selected to use the compile time option to
> enable plain-text password storage.
Spell out that "old" passwords means passwords already cached on disk
("grandfathered") — as opposed to, say, passwords that had been changed.
The wording "Certain distributions may…" sounds like indirect
finger-pointing. Why not s/speculation/fact/: a compile-time option to
enable plaintext passwords storage is provided and may have been
selected by whoever built the binaries you're using (the term "distro"
doesn't capture VisualSVN and TortoiseSVN).
> s/However .*/In case Subversion is compiled with support for storing
> plain-text passwords, you can disable it in your run-time config file by
> setting 'store-plaintext-passwords = no' (so that encrypted stores like
> GNOME Keyring and KWallet will still be used - this is already done in at
> least one distribution which has selected to enable the plain-text password
> storage in svn 1.12). If you want to disable storing any kind of
> credentials you may instead set 'store-auth-creds = no', or you can use the
> more narrowly-defined 'store-passwords = no' (so that server certs are
> still cached). More information on password cacheing is in chapter 6 of the
> "Nightly Build" Subversion book, under "Client Credentials Caching"./
Is the information only available in the nightly build? If not,
s/"Nightly Build"//.
> ]]]
>
> The "Since svn 1.12..." should probably go in the end of the first "On
> UNIX/Linux" section, after "(Since svn 1.6.)"
At this point, a «svn diff -x-U10» unidiff might be easier for everyone.
By the way, how about changing "if you're really worried" in the
preëxisting text. This phrasing crosses the line from discussing the
reader's risk assessment to discussing their mental state.
> +++ contrib/client-side/store-plaintext-password.py (working copy)
> @@ -0,0 +1,207 @@
> +#!/usr/bin/env python3
> +
> +# Script to store password in plaintext in ~/.subversion/auth/svn.simple/
> +#
> +# Script require the tabulate module: pip install tabulate
s/require/requires/
More importantly, it would be better not to require non-stdlib modules,
at least in the "save passwords" codepath. The easiest way to do this
would be to move the «import» statement into the function that needs it.
> +# ====================================================================
> +# Licensed to the Apache Software Foundation (ASF) under one
> +# or more contributor license agreements. See the NOTICE file
> +# distributed with this work for additional information
> +# regarding copyright ownership. The ASF licenses this file
> +# to you under the Apache License, Version 2.0 (the
> +# "License"); you may not use this file except in compliance
> +# with the License. You may obtain a copy of the License at
> +#
> +# http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless required by applicable law or agreed to in writing,
> +# software distributed under the License is distributed on an
> +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
> +# KIND, either express or implied. See the License for the
> +# specific language governing permissions and limitations
> +# under the License.
> +# ====================================================================
> +
> +from hashlib import md5
> +import argparse
> +import os
> +from os import listdir
> +from os.path import isfile, join
These two imports aren't very idiomatic, at least not in Subversion's
use of Python. We tend to just call these functions by their
fully-qualified names.
> +from pathlib import Path
> +from tabulate import tabulate
> +
> +# Read a hashfile and return a key/value list
> +#
Err, what? The function reads the hashfile and returns the first
key-value pair in it as a list. It doesn't read any subsequent
key-value pairs, which is what this sentence seems to imply.
Also, the function doesn't ensure the 'END\n' terminator and EOF happen
where they're expected.
Also, don't rely on the key-value pairs in the hash file being in any
particular order.
> +# A hashfile should contain the following four lines, repeted as needed, ending by the line END
"repeated" is misspelled. Also, this should reference the upstream docs
(svn_hash_write2()), either in addition to or instead of repeating them.
> +# K [length]
> +# [keyname]
> +# V [length]
> +# [value]
> +# ...
> +# END
> +#
> +# The length is not validated
It's not just "not validated"; it's not used at all. The file format is
binary — the lengths are in octets — but the Python code below treats
the file as a line-based format. That may be fine for now, but is not
forward-compatible with ~/.subversion/auth/ files that may be created by
future versions.
I'm not sure what level of the API (public or private) third-party
products would need to operate at in order to add their own custom keys
to the cache files. I don't know of anyone doing that, but it's not
impossible.
> +def readHash(file):
> + # Read K [length] or END but discard it
> + key = file.readline().strip()
> + if key == 'END':
> + return None
> + if key[:1] != 'K':
> + raise Exception('Parse failed, expected K...')
> +
> + # Read keyname
> + key = file.readline().strip()
> +
> + # Read V [length] but discard it
> + val = file.readline().strip()
> + if val[:1] != 'V':
> + raise Exception('Parse failed, expected V...')
> +
> + # Read value
> + val = file.readline().strip()
> +
> + return [key, val]
Returning a tuple would be more idiomatic.
> +# Write a key/value pair to a hashfile
> +def writeHash(file, key, val):
The docstring should be a string literal as the first statement in the
function, as in subversion/tests/cmdline/. That then becomes the
function's __doc__ attribute.
> + file.write('K ' + str(len(key)) + '\n')
> + file.write(key + '\n')
str() gives the length in characters, but the file format demands
a length in octets, so this would break as soon as somebody puts
non-ASCII in their password — which, given the nature of passwords,
is plausible.
You'll want to convert from str to bytes. See str.encode(). You'll
need to use b'' literals in concatenations.
> + file.write('V ' + str(len(val)) + '\n')
> + file.write(val + '\n')
> +
> +# Main function
> +parser = argparse.ArgumentParser(description='Store plain text password in ~/.subversion/auth/svn.simple/')
> +parser.add_argument('realm', help='Realm string from server, required value for updates', nargs='?')
> +parser.add_argument('password', help='Password, required value for updates', nargs='?')
IIRC, nargs='?' means that both ['store-plaintext-password.py',
'--password'] and ['store-plaintext-password.py', '--password', 'foo']
are valid uses, which isn't what's needed here.
> +parser.add_argument('username', help='Username, optional value for updates', nargs='?')
> +parser.add_argument('--list', help='Lists all stored realms/usernames', action='store_true')
> +parser.add_argument('--listpassword', help='Lists all stored realms/usernames/passwords', action='store_true')
> +
> +args = parser.parse_args()
> +authpath = str(Path.home()) + '/.subversion/auth/svn.simple/'
Just use os.path.expanduser() directly? You don't need pathlib.Path's
bells and whistles.
> +if args.listpassword or args.list:
> + data = []
> + if args.listpassword:
> + header = ['Realm', 'Username', 'Password']
> + else:
> + header = ['Realm', 'Username']
> + for name in listdir(authpath):
> + fullname = join(authpath, name)
> + if isfile(fullname):
> + file = open(fullname, 'r')
> + realm = ''
> + user = ''
> + password = ''
> + while(True):
The parentheses aren't idiomatic.
> + hash = readHash(file)
So this just slurps the file, rather than return each keypair
iteratively (with «yield»). Good enough for the present use-case.
> + if hash is None:
> + break
> + elif hash[0] == 'svn:realmstring':
> + realm = hash[1]
> + elif hash[0] == 'username':
> + user = hash[1]
> + elif hash[0] == 'password':
> + password = hash[1]
> + file.close()
> + if args.listpassword:
> + data.append([realm, user, password])
This sounds like it could become a list of collections.namedtuple
instance instances (sic), but as mentioned above, this entire function
seems a reinvention of `svn auth`.
> + else:
> + data.append([realm, user])
> + print(tabulate(data, headers=header))
> + quit()
> +
> +if args.realm is None:
> + print("Realm required\n")
Errors should go to stderr, and begin with the name (argv[0]) of whoever
generated them.
Any reason not to let argparse enforce the mandatoriness of the argument?
> + parser.print_help()
> + quit()
This will NameError on 'quit'. At least the exit code will be non-zero
this way.
> +if args.password is None:
> + print("Password required\n")
> + parser.print_help()
> + quit()
> +
> +# The file name is the md5encoding of the realm
> +m = md5()
> +m.update(args.realm.encode('utf-8'))
> +fullname = join(authpath, m.hexdigest())
> +
> +# In an existing file, we add/replace password/username/passtype
This bit of code conflates several distinct pieces of logic:
- Parsing a serialized hash file
- Serializing a hash
- The actual patching of the hash
- Implementing editing a file by an atomic rename
I can't say I like this at all. All these concerns should be cleanly
separated from one another.
> +if isfile(fullname):
> + pwdAdded = False
> + passtypeAdded = False
> + usernameAdded = False
> +
> + # Read existing file, write to .tmp which we rename in the end
> + inFile = open(fullname, 'r')
> + outFile = open(fullname + '.tmp', 'w')
Also 'x' please for O_EXCL. If nothing else, this'll plug a race
condition when two instances of this script run simultaneously.
> + while(True):
> + # Read K [length] or END and write back
> + line = inFile.readline()
> + if not line:
> + raise Exception('Parse failed, expected K ... or END')
> + if line.strip() == 'END':
Duplicates logic from earlier in the file.
> + # If username, password and/or passtype has not been found in the file, add them here
> + if not usernameAdded and args.username is not None:
> + writeHash(outFile, 'username', args.username)
> + if not passtypeAdded:
> + writeHash(outFile, 'passtype', 'simple')
Leaky abstraction: this layer needn't know that the file format
serialized keys and values in the order K1 V1 K2 V2 … as opposed to,
say, K1 K2 … V1 V2 ….
> + if not pwdAdded:
> + writeHash(outFile, 'password', args.password)
> + outFile.write(line)
> + break
> + outFile.write(line)
> +
> + # Read keyname and write back, save keyname for later
> + line = inFile.readline()
> + outFile.write(line)
Input is used unvalidated.
> + elif key == 'passtype':
> + outFile.write('V 6\n')
Magic number.
Cheers,
Daniel
Re: A more permanent home for the add-a-password-to-a-cached-username
script? (was: Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password)
Posted by Daniel Sahlberg <da...@gmail.com>.
Den tis 23 feb. 2021 kl 17:46 skrev Daniel Shahaf <d....@daniel.shahaf.name>:
> Daniel Sahlberg wrote on Tue, Feb 23, 2021 at 16:50:07 +0100:
> > Den tis 23 feb. 2021 16:40Nathan Hartman <ha...@gmail.com>
> skrev:
> > > I think it's a good candidate for contrib (though it might be better
> > > to port it to portable Bourne shell first).
> > >
> > > Would a Python version be useful?
>
> Porting isa rewrite and as such may introduce bugs.
>
> Porting will increase the number of developers able to maintain the script.
>
> Porting may let some users run the script without installing zsh first.
> However, it's perhaps worth noting (in any eventual FAQ entry as well)
> that running the script doesn't require chsh(1)ing to zsh: there's
> nothing preventing bash users from installing zsh just so as to run
> scripts written in it.
>
> Porting wouldn't be a matter of translating line-by-line, since the
> «select»
> builtin (
> http://zsh.sourceforge.net/Doc/Release/Shell-Grammar.html#index-select)
> doesn't have a direct equivalent in other languages. For translating to
> sh(1)
> specifically, the use of arrays would also be an issue.
>
> As to contrib/, even though it's deprecated, I don't have a better idea.
> (tools/ is probably not appropriate, unless more devs speak zsh than
> I know.)
>
Learning Python has been on my todo list for ages, so I've cobbled
together something that seems to do the job.
There are basically two modes of operation:
./store-plaintext-password.py --listpassword|--list
Which lists all cached realms/usernames (and passwords). I've intentionally
split it in two in case someone prefer not to put the password on-screen,
but I've also added the option to display to show that the password is
really in plain text.
./store-plaintext-password.py realm password [username]
Which stores/updates the password for the given realm. If username is given
it is stored/updated as well. If there is no cached entry for the specified
realm a new file will be created (and username is a mandatory argument).
TODO:
- Is the license ok? I stole it from svn.c
- Improve Python code
- Improve documentation - especially on where to find the 'realm' string
- Decide where to put it - I've gone with contrib for now as per Nathan's
and Daniel's suggestions
> > Regarding the FAQ, currently we have [1] "Ahhh! I just discovered that
> > > my Subversion client is caching passwords in plain-text on disk!
> > > AHHH!" That is still applicable to 1.10, but now we need an entry to
> > > answer the opposite question: how to cache the password for svn use
> > > with cron jobs and non-X environments where Kwallet and GNOME-Keyring
> > > aren't applicable, and the particularly annoying case in which the
> > > machine itself has a GUI but the user is logged in via ssh; in this
> > > case the svn client will "freeze" while waiting for password entry in
> > > an inaccessible GUI window; I think this would occur with Kwallet,
> > > GNOME-Keyring, and macOS's Keychain.)
> > >
> > > But, as there doesn't seem to be one well-established way to handle
> > > this, other than just storing the password on disk, would the new FAQ
> > > entry say just that? Do we have any other concrete suggestions?
>
> If a cron job needs authentication, its credentials need to be stored
> somewhere, either in plaintext or in "as good as" plaintext. I think
> storing the passwords in unobfuscated plaintext was a deliberate
> decision, informed by CVS's design choices in this regard, but I wasn't
> around in the early days.
>
> On the other hand, for GUI-less environments and for headful
> environments ssh'd into, we should simply document the capabilities and
> workarounds of the libraries we use (possibly by pointing to those
> libraries' documentations).
>
Suggestion for new FAQ entry:
[[[
Ahhh! I just discovered that my Subversion client is NOT caching passwords
in plain-text on disk! AHHH!
Calm down, take a deep breath.
This is the opposite of the previous question. After changing the compile
time default to not store passwords in plain-text there has been a number
of requests in the mailing lists to store the password.
If you understand the security implications, you have ruled out other
alternatives and still want to cache your password in plain-text on disk
you can use the script
https://svn.apache.org/repos/asf/subversion/trunk/contrib/client-side/store-plaintext-password-py
to store the password in the directory which contains the cached passwords
(usually ~/.subversion/auth/). The script can also be used to list any
existing passwords stored in plain-text.
]]]
I'm also suggesting to change the existing FAQ entry (Ahhh! I just
discovered that my Subversion client is caching passwords in plain-text on
disk! AHHH!) to mention the changed compile time default since 1.12 to not
store plain-text passwords:
[[[
s/Otherwise, the client will fall back/Otherwise, the client can fall back/
Since svn 1.12 the compile time default has been to disable storing new
passwords in plain-text, but old passwords can still be used. Certain
distributions may also have selected to use the compile time option to
enable plain-text password storage.
s/However .*/In case Subversion is compiled with support for storing
plain-text passwords, you can disable it in your run-time config file by
setting 'store-plaintext-passwords = no' (so that encrypted stores like
GNOME Keyring and KWallet will still be used - this is already done in at
least one distribution which has selected to enable the plain-text password
storage in svn 1.12). If you want to disable storing any kind of
credentials you may instead set 'store-auth-creds = no', or you can use the
more narrowly-defined 'store-passwords = no' (so that server certs are
still cached). More information on password cacheing is in chapter 6 of the
"Nightly Build" Subversion book, under "Client Credentials Caching"./
]]]
The "Since svn 1.12..." should probably go in the end of the first "On
UNIX/Linux" section, after "(Since svn 1.6.)"
> As for the script, IIRC there was a need for the username (?) to be cached
> > before running the script. Where should that be stored?
>
> In the md5(realm)-named file in ~/.subversion/auth/ which the script
> ed(1)s. (The --no-auth-cache flag and the store-auth-creds
> configuration knob may affect whether the username is saved to disk.)
>
> The easiest way to populate that is to run `svn info` on the URL and
> authenticate manually.
>
> A script that takes a URL/username/password and inserts that tuple into
> the cache would be nice, of course, but it'd need to compute the realm
> string of the URL.
>
Thanks for the info, that helped me a lot in crafting the script. I've gone
the easy way and required the user to provide the realm string.
Kind regards,
Daniel Sahlberg
Re: A more permanent home for the add-a-password-to-a-cached-username
script? (was: Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password)
Posted by Nico Kadel-Garcia <nk...@gmail.com>.
On Tue, Feb 23, 2021 at 3:15 PM Branko Čibej <br...@apache.org> wrote:
>
> On 23.02.2021 17:46, Daniel Shahaf wrote:
> > If a cron job needs authentication, its credentials need to be stored
> > somewhere, either in plaintext or in "as good as" plaintext. I think
> > storing the passwords in unobfuscated plaintext was a deliberate
> > decision, informed by CVS's design choices in this regard, but I wasn't
> > around in the early days.
>
> It was deliberate. Reading those passwords requires access to the
> filesystem, so an attacker either has the affected user's credentials --
> in which case they probably have access to any encrypted password store
> as well -- or they're root, and in _that_ case all bets are off anyway.
Sadly, with docker, VM's, filesystem snapshots ,and home directories
on NFS shares exported to the world at large without Kerberos
authenticaiton (which I saw a bunch of computer science professors at
MIT insist on, until finally MIT's IT group stepped on them for it and
shoved them behind NAT), it's too easy to steal such credentials.
> Note that recently operating systems have gone in the direction of _not_
> letting root do everything without extra checks, so maybe the second
> assumption needs to be reconsidered.
>
> In any case, encrypted or otherwise protected password stores have
> master passwords that have to be stored somewhere for unattended
> operation, so that's just moving the problem one level of indirection away.
No. They don'.t. A session can be unlocked with which the live user or
service user can make a successful connection. The private key lives
in RAM, not on disk, and the key needs to be unlocked at user login or
at system start. This was what the old "keychain" perl script provided
along with ssh-agent.
Re: A more permanent home for the add-a-password-to-a-cached-username
script? (was: Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password)
Posted by Branko Čibej <br...@apache.org>.
On 23.02.2021 17:46, Daniel Shahaf wrote:
> If a cron job needs authentication, its credentials need to be stored
> somewhere, either in plaintext or in "as good as" plaintext. I think
> storing the passwords in unobfuscated plaintext was a deliberate
> decision, informed by CVS's design choices in this regard, but I wasn't
> around in the early days.
It was deliberate. Reading those passwords requires access to the
filesystem, so an attacker either has the affected user's credentials --
in which case they probably have access to any encrypted password store
as well -- or they're root, and in _that_ case all bets are off anyway.
Note that recently operating systems have gone in the direction of _not_
letting root do everything without extra checks, so maybe the second
assumption needs to be reconsidered.
In any case, encrypted or otherwise protected password stores have
master passwords that have to be stored somewhere for unattended
operation, so that's just moving the problem one level of indirection away.
-- Brane
Re: A more permanent home for the
add-a-password-to-a-cached-username script? (was: Re: using svn cli with
--non-interactive (in scripts) securely, without exposing password)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Sahlberg wrote on Tue, Feb 23, 2021 at 16:50:07 +0100:
> Den tis 23 feb. 2021 16:40Nathan Hartman <ha...@gmail.com> skrev:
> > I think it's a good candidate for contrib (though it might be better
> > to port it to portable Bourne shell first).
> >
> > Would a Python version be useful?
Porting isa rewrite and as such may introduce bugs.
Porting will increase the number of developers able to maintain the script.
Porting may let some users run the script without installing zsh first.
However, it's perhaps worth noting (in any eventual FAQ entry as well)
that running the script doesn't require chsh(1)ing to zsh: there's
nothing preventing bash users from installing zsh just so as to run
scripts written in it.
Porting wouldn't be a matter of translating line-by-line, since the «select»
builtin (http://zsh.sourceforge.net/Doc/Release/Shell-Grammar.html#index-select)
doesn't have a direct equivalent in other languages. For translating to sh(1)
specifically, the use of arrays would also be an issue.
As to contrib/, even though it's deprecated, I don't have a better idea.
(tools/ is probably not appropriate, unless more devs speak zsh than
I know.)
> > Regarding the FAQ, currently we have [1] "Ahhh! I just discovered that
> > my Subversion client is caching passwords in plain-text on disk!
> > AHHH!" That is still applicable to 1.10, but now we need an entry to
> > answer the opposite question: how to cache the password for svn use
> > with cron jobs and non-X environments where Kwallet and GNOME-Keyring
> > aren't applicable, and the particularly annoying case in which the
> > machine itself has a GUI but the user is logged in via ssh; in this
> > case the svn client will "freeze" while waiting for password entry in
> > an inaccessible GUI window; I think this would occur with Kwallet,
> > GNOME-Keyring, and macOS's Keychain.)
> >
> > But, as there doesn't seem to be one well-established way to handle
> > this, other than just storing the password on disk, would the new FAQ
> > entry say just that? Do we have any other concrete suggestions?
If a cron job needs authentication, its credentials need to be stored
somewhere, either in plaintext or in "as good as" plaintext. I think
storing the passwords in unobfuscated plaintext was a deliberate
decision, informed by CVS's design choices in this regard, but I wasn't
around in the early days.
On the other hand, for GUI-less environments and for headful
environments ssh'd into, we should simply document the capabilities and
workarounds of the libraries we use (possibly by pointing to those
libraries' documentations).
> As for the script, IIRC there was a need for the username (?) to be cached
> before running the script. Where should that be stored?
In the md5(realm)-named file in ~/.subversion/auth/ which the script
ed(1)s. (The --no-auth-cache flag and the store-auth-creds
configuration knob may affect whether the username is saved to disk.)
The easiest way to populate that is to run `svn info` on the URL and
authenticate manually.
A script that takes a URL/username/password and inserts that tuple into
the cache would be nice, of course, but it'd need to compute the realm
string of the URL.
Cheers,
Daniel
Re: A more permanent home for the add-a-password-to-a-cached-username
script? (was: Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password)
Posted by Daniel Sahlberg <da...@gmail.com>.
Den tis 23 feb. 2021 16:40Nathan Hartman <ha...@gmail.com> skrev:
> On Tue, Feb 23, 2021 at 8:35 AM Daniel Shahaf <d....@daniel.shahaf.name>
> wrote:
>
>> Nathan Hartman wrote on Mon, Feb 22, 2021 at 14:36:58 -0500:
>> > In a recent discussion on our dev mailing list, there is an example
>> > shell script (for zsh) that saves a password file. See [2] and note
>> > that there were a few corrections to the script so be sure to use the
>> > latest version in that mail list thread.
>> >
>> > [1]
>> https://subversion.apache.org/docs/release-notes/1.12.html#client-server-improvements
>> >
>> > [2]
>> https://lists.apache.org/thread.html/r0eef40236aeddd1db18bc7882454dd3b18bcd721d8fd8c9e21aca52a%40%3Cdev.subversion.apache.org%3E
>> >
>> > I hope the above is helpful; feel free to ask as many questions as you
>> > need to, or propose improvements to the above-mentioned script or
>> > Subversion itself. We have gotten quite a few questions about this and
>>
>> I'm starting to wonder if that script deserves a home more permanent
>> than the end of a random dev@ thread whose subject line contains "WTF"
>> and "?!"; e.g., perhaps that script should be linked from the FAQ or
>> the release notes.
>
>
>
> I agree we need a better place for the script, especially as the above
> mail thread contains more than one version of it.
>
> I think it's a good candidate for contrib (though it might be better
> to port it to portable Bourne shell first).
>
> Would a Python version be useful?
>
> Regarding the FAQ, currently we have [1] "Ahhh! I just discovered that
> my Subversion client is caching passwords in plain-text on disk!
> AHHH!" That is still applicable to 1.10, but now we need an entry to
> answer the opposite question: how to cache the password for svn use
> with cron jobs and non-X environments where Kwallet and GNOME-Keyring
> aren't applicable, and the particularly annoying case in which the
> machine itself has a GUI but the user is logged in via ssh; in this
> case the svn client will "freeze" while waiting for password entry in
> an inaccessible GUI window; I think this would occur with Kwallet,
> GNOME-Keyring, and macOS's Keychain.)
>
> But, as there doesn't seem to be one well-established way to handle
> this, other than just storing the password on disk, would the new FAQ
> entry say just that? Do we have any other concrete suggestions?
>
I would go with a new FAQ entry. I can prepare a draft in staging.
As for the script, IIRC there was a need for the username (?) to be cached
before running the script. Where should that be stored?
Kind regards
Daniel Sahlberg
> [1] https://subversion.apache.org/faq.html#plaintext-passwords
>
> Nathan
>
>
Re: A more permanent home for the add-a-password-to-a-cached-username
script? (was: Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password)
Posted by Nathan Hartman <ha...@gmail.com>.
On Tue, Feb 23, 2021 at 8:35 AM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> Nathan Hartman wrote on Mon, Feb 22, 2021 at 14:36:58 -0500:
> > In a recent discussion on our dev mailing list, there is an example
> > shell script (for zsh) that saves a password file. See [2] and note
> > that there were a few corrections to the script so be sure to use the
> > latest version in that mail list thread.
> >
> > [1]
> https://subversion.apache.org/docs/release-notes/1.12.html#client-server-improvements
> >
> > [2]
> https://lists.apache.org/thread.html/r0eef40236aeddd1db18bc7882454dd3b18bcd721d8fd8c9e21aca52a%40%3Cdev.subversion.apache.org%3E
> >
> > I hope the above is helpful; feel free to ask as many questions as you
> > need to, or propose improvements to the above-mentioned script or
> > Subversion itself. We have gotten quite a few questions about this and
>
> I'm starting to wonder if that script deserves a home more permanent
> than the end of a random dev@ thread whose subject line contains "WTF"
> and "?!"; e.g., perhaps that script should be linked from the FAQ or
> the release notes.
I agree we need a better place for the script, especially as the above
mail thread contains more than one version of it.
I think it's a good candidate for contrib (though it might be better
to port it to portable Bourne shell first).
Would a Python version be useful?
Regarding the FAQ, currently we have [1] "Ahhh! I just discovered that
my Subversion client is caching passwords in plain-text on disk!
AHHH!" That is still applicable to 1.10, but now we need an entry to
answer the opposite question: how to cache the password for svn use
with cron jobs and non-X environments where Kwallet and GNOME-Keyring
aren't applicable, and the particularly annoying case in which the
machine itself has a GUI but the user is logged in via ssh; in this
case the svn client will "freeze" while waiting for password entry in
an inaccessible GUI window; I think this would occur with Kwallet,
GNOME-Keyring, and macOS's Keychain.)
But, as there doesn't seem to be one well-established way to handle
this, other than just storing the password on disk, would the new FAQ
entry say just that? Do we have any other concrete suggestions?
[1] https://subversion.apache.org/faq.html#plaintext-passwords
Nathan
A more permanent home for the add-a-password-to-a-cached-username
script? (was: Re: using svn cli with --non-interactive (in scripts)
securely, without exposing password)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Nathan Hartman wrote on Mon, Feb 22, 2021 at 14:36:58 -0500:
> In a recent discussion on our dev mailing list, there is an example
> shell script (for zsh) that saves a password file. See [2] and note
> that there were a few corrections to the script so be sure to use the
> latest version in that mail list thread.
>
> [1] https://subversion.apache.org/docs/release-notes/1.12.html#client-server-improvements
>
> [2] https://lists.apache.org/thread.html/r0eef40236aeddd1db18bc7882454dd3b18bcd721d8fd8c9e21aca52a%40%3Cdev.subversion.apache.org%3E
>
> I hope the above is helpful; feel free to ask as many questions as you
> need to, or propose improvements to the above-mentioned script or
> Subversion itself. We have gotten quite a few questions about this and
I'm starting to wonder if that script deserves a home more permanent
than the end of a random dev@ thread whose subject line contains "WTF"
and "?!"; e.g., perhaps that script should be linked from the FAQ or
the release notes.
Cheers,
Daniel
> it has been frustrating for anyone who uses svn as part of cron jobs
> in non-X environments, where the available encrypted password stores,
> Kwallet and Gnome-keyring, aren't much help, and GPG-Agent doesn't
> persist the passwords indefinitely. We would be really grateful if
> someone could propose a solution that works well in these scenarios
> while alleviating people's concerns about storing passwords on disk in
> plaintext.
>
> Cheers,
> Nathan
Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password
Posted by Nathan Hartman <ha...@gmail.com>.
On Mon, Feb 22, 2021 at 1:17 PM CoolCold <co...@gmail.com> wrote:
>
> Good day!
> (please CC me, I'm not subscribed to the list)
>
> A bit of context:
> I was using subversion to store my serves' configs versioned for
> almost a decade, with bash wrapping around it. Simplified, it had repo
> per server name, wrapper called by cron to checkout, rsync over,
> commit changes back, sending email on diffs (
> https://github.com/coolcold/svnbackup ). Had no issue with it, when
> password store was enabled. It's runned under root user and saved
> credentials are not exposed to non-admin users on the system.
>
> Issue: with recent changes hitting packages in distributions (
> https://marc.info/?l=subversion-commits&m=154101482302608&w=2 ), that
> seems to be not possible anymore.
> I did adjust my script to use command line switch --password, but this
> makes it visible for anyone who does simple commands like ps aux.
> I've tried to look around for possible support of environment
> variables / password file support, but couldn't find any except some
> old proposals like
> http://subversion.1072662.n5.nabble.com/Feature-proposal-SVN-USERNAME-and-SVN-PASSWORD-environment-variables-td180031.html
>
> Rebuilding subversion from source is not an option for many reasons.
>
> Seeking for your help on this, what is the proper way of doing this
> with recent versions?
> Thanks in advance.
Hello,
Recent versions (1.12.x and newer [1]) by default don't _save_
passwords to disk in plaintext (unless configured to do so at
build-time).
However, Subversion will _use_ the password, if it is already stored
on disk.
So, as a workaround, you could use some out-of-band method to save the
password to disk either by using an older SVN client or by generating
a file with the right bits in it:
In a recent discussion on our dev mailing list, there is an example
shell script (for zsh) that saves a password file. See [2] and note
that there were a few corrections to the script so be sure to use the
latest version in that mail list thread.
[1] https://subversion.apache.org/docs/release-notes/1.12.html#client-server-improvements
[2] https://lists.apache.org/thread.html/r0eef40236aeddd1db18bc7882454dd3b18bcd721d8fd8c9e21aca52a%40%3Cdev.subversion.apache.org%3E
I hope the above is helpful; feel free to ask as many questions as you
need to, or propose improvements to the above-mentioned script or
Subversion itself. We have gotten quite a few questions about this and
it has been frustrating for anyone who uses svn as part of cron jobs
in non-X environments, where the available encrypted password stores,
Kwallet and Gnome-keyring, aren't much help, and GPG-Agent doesn't
persist the passwords indefinitely. We would be really grateful if
someone could propose a solution that works well in these scenarios
while alleviating people's concerns about storing passwords on disk in
plaintext.
Cheers,
Nathan
Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password
Posted by CoolCold <co...@gmail.com>.
Hello,
On Tue, Feb 23, 2021 at 3:23 AM Yasuhito FUTATSUKI
<fu...@yf.bsdclub.org> wrote:
>
> Hello,
>
> On 2021/02/23 2:40, CoolCold wrote:
> > Good day!
> > (please CC me, I'm not subscribed to the list)
> >
> > A bit of context:
> > I was using subversion to store my serves' configs versioned for
> > almost a decade, with bash wrapping around it. Simplified, it had repo
> > per server name, wrapper called by cron to checkout, rsync over,
> > commit changes back, sending email on diffs (
> > https://github.com/coolcold/svnbackup ). Had no issue with it, when
> > password store was enabled. It's runned under root user and saved
> > credentials are not exposed to non-admin users on the system.
> >
> > Issue: with recent changes hitting packages in distributions (
> > https://marc.info/?l=subversion-commits&m=154101482302608&w=2 ), that
> > seems to be not possible anymore.
> > I did adjust my script to use command line switch --password, but this
> > makes it visible for anyone who does simple commands like ps aux.
> > I've tried to look around for possible support of environment
> > variables / password file support, but couldn't find any except some
> > old proposals like
> > http://subversion.1072662.n5.nabble.com/Feature-proposal-SVN-USERNAME-and-SVN-PASSWORD-environment-variables-td180031.html
> >
> > Rebuilding subversion from source is not an option for many reasons.
> >
> > Seeking for your help on this, what is the proper way of doing this
> > with recent versions?
> > Thanks in advance.
>
> For this purpose, I'm just using svn+ssh:// with dedicated user on
> server side and public key authentication with empty pass phrase.
>
> e.g. svn+ssh://svn-agent@svnhost.example.org/repo/
>
> On svnhost.example.org, authorized_key file for user svn-agent could be:
> [[[
> # only for svn agent via ssh. all lines shoud be started with following
> # command and option specification:
> command="/usr/bin/svnserve -t -r /base/path/to/repo --tunnel-user=root-on-hostA",no-port-forwarding,no-agent-forwarding,no-X11-forwarding,no-pty ssh-ed25519 (public key A) root@hostA.example.org
> command="/usr/bin/svnserve -t -r /base/path/to/repo --tunnel-user=root-on-hostB",no-port-forwarding,no-agent-forwarding,no-X11-forwarding,no-pty ssh-ed25519 (public key B) root@hostB.example.org
> ...
> ]]]
Thanks for sharing this - I still see such a way as a workaround, but
as a reasonable workaround. One may want to prefer it due to data
transfer encryption it provides.
From the other side, it's a trade of "cleartext" password vs
"unprotected ssh key" (while key can be IP bound in
.ssh/authorized_keys thus making it a bit better protected in case of
leakage).
>
> Cf.
> http://svnbook.red-bean.com/nightly/en/svn.serverconfig.svnserve.html#svn.serverconfig.svnserve.sshtricks
>
> If you want to use ssh key other than default key or alternative tcp port
> other than 22, you can use them by overriding ssh tunnel setting with SVN_SSH
> environment variable or config file, etc. (Of course, if you want to use non
> standard port for ssh connection you also need to change configuration of
> sshd on server side).
>
> Cheers,
> --
> Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
--
Best regards,
[COOLCOLD-RIPN]
Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password
Posted by Nico Kadel-Garcia <nk...@gmail.com>.
Sorry for message without content, re-sending with content.
> On Mon, Feb 22, 2021 at 3:25 PM Yasuhito FUTATSUKI
> > If you want to use ssh key other than default key or alternative tcp port
> > other than 22, you can use them by overriding ssh tunnel setting with SVN_SSH
> > environment variable or config file, etc. (Of course, if you want to use non
> > standard port for ssh connection you also need to change configuration of
> > sshd on server side).
No. SSh keys without passphrases are much like Post-it notes with
passwords on them, or stored passwords in Subversion. Tools that can
write to a source control without anyone unlocking the key are quite
dangerous.
It's straightforward to use ssh-agent to unlock and store access to
the key after a server is booted, but requiring a console to set up
*once* and save long-term. The old "keychain" tool works quite well
for this, and can enable ephemereal access to a live ssh-agent as
needed. For automated build tools like Jenkins, they can even store
the private key internally, encrypted with the SSH server's
encryption, and restricted to certain Jenkins "folders" for project
specific access. I use this approach regularly for Jenkins and source
control.
SSH access is also vulnerable to changing host keys in DHCP based
environments. Do look up SSH hostkeys and the keyword "/dev/null" to
find many notes about how to disable this, including ones I've been
publishing for decades.
Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password
Posted by Nico Kadel-Garcia <nk...@gmail.com>.
On Mon, Feb 22, 2021 at 3:25 PM Yasuhito FUTATSUKI
<fu...@yf.bsdclub.org> wrote:
>
> Hello,
>
> On 2021/02/23 2:40, CoolCold wrote:
> > Good day!
> > (please CC me, I'm not subscribed to the list)
> >
> > A bit of context:
> > I was using subversion to store my serves' configs versioned for
> > almost a decade, with bash wrapping around it. Simplified, it had repo
> > per server name, wrapper called by cron to checkout, rsync over,
> > commit changes back, sending email on diffs (
> > https://github.com/coolcold/svnbackup ). Had no issue with it, when
> > password store was enabled. It's runned under root user and saved
> > credentials are not exposed to non-admin users on the system.
> >
> > Issue: with recent changes hitting packages in distributions (
> > https://marc.info/?l=subversion-commits&m=154101482302608&w=2 ), that
> > seems to be not possible anymore.
> > I did adjust my script to use command line switch --password, but this
> > makes it visible for anyone who does simple commands like ps aux.
> > I've tried to look around for possible support of environment
> > variables / password file support, but couldn't find any except some
> > old proposals like
> > http://subversion.1072662.n5.nabble.com/Feature-proposal-SVN-USERNAME-and-SVN-PASSWORD-environment-variables-td180031.html
> >
> > Rebuilding subversion from source is not an option for many reasons.
> >
> > Seeking for your help on this, what is the proper way of doing this
> > with recent versions?
> > Thanks in advance.
>
> For this purpose, I'm just using svn+ssh:// with dedicated user on
> server side and public key authentication with empty pass phrase.
>
> e.g. svn+ssh://svn-agent@svnhost.example.org/repo/
>
> On svnhost.example.org, authorized_key file for user svn-agent could be:
> [[[
> # only for svn agent via ssh. all lines shoud be started with following
> # command and option specification:
> command="/usr/bin/svnserve -t -r /base/path/to/repo --tunnel-user=root-on-hostA",no-port-forwarding,no-agent-forwarding,no-X11-forwarding,no-pty ssh-ed25519 (public key A) root@hostA.example.org
> command="/usr/bin/svnserve -t -r /base/path/to/repo --tunnel-user=root-on-hostB",no-port-forwarding,no-agent-forwarding,no-X11-forwarding,no-pty ssh-ed25519 (public key B) root@hostB.example.org
> ...
> ]]]
>
> Cf.
> http://svnbook.red-bean.com/nightly/en/svn.serverconfig.svnserve.html#svn.serverconfig.svnserve.sshtricks
>
> If you want to use ssh key other than default key or alternative tcp port
> other than 22, you can use them by overriding ssh tunnel setting with SVN_SSH
> environment variable or config file, etc. (Of course, if you want to use non
> standard port for ssh connection you also need to change configuration of
> sshd on server side).
>
> Cheers,
> --
> Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: using svn cli with --non-interactive (in scripts) securely,
without exposing password
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
Hello,
On 2021/02/23 2:40, CoolCold wrote:
> Good day!
> (please CC me, I'm not subscribed to the list)
>
> A bit of context:
> I was using subversion to store my serves' configs versioned for
> almost a decade, with bash wrapping around it. Simplified, it had repo
> per server name, wrapper called by cron to checkout, rsync over,
> commit changes back, sending email on diffs (
> https://github.com/coolcold/svnbackup ). Had no issue with it, when
> password store was enabled. It's runned under root user and saved
> credentials are not exposed to non-admin users on the system.
>
> Issue: with recent changes hitting packages in distributions (
> https://marc.info/?l=subversion-commits&m=154101482302608&w=2 ), that
> seems to be not possible anymore.
> I did adjust my script to use command line switch --password, but this
> makes it visible for anyone who does simple commands like ps aux.
> I've tried to look around for possible support of environment
> variables / password file support, but couldn't find any except some
> old proposals like
> http://subversion.1072662.n5.nabble.com/Feature-proposal-SVN-USERNAME-and-SVN-PASSWORD-environment-variables-td180031.html
>
> Rebuilding subversion from source is not an option for many reasons.
>
> Seeking for your help on this, what is the proper way of doing this
> with recent versions?
> Thanks in advance.
For this purpose, I'm just using svn+ssh:// with dedicated user on
server side and public key authentication with empty pass phrase.
e.g. svn+ssh://svn-agent@svnhost.example.org/repo/
On svnhost.example.org, authorized_key file for user svn-agent could be:
[[[
# only for svn agent via ssh. all lines shoud be started with following
# command and option specification:
command="/usr/bin/svnserve -t -r /base/path/to/repo --tunnel-user=root-on-hostA",no-port-forwarding,no-agent-forwarding,no-X11-forwarding,no-pty ssh-ed25519 (public key A) root@hostA.example.org
command="/usr/bin/svnserve -t -r /base/path/to/repo --tunnel-user=root-on-hostB",no-port-forwarding,no-agent-forwarding,no-X11-forwarding,no-pty ssh-ed25519 (public key B) root@hostB.example.org
...
]]]
Cf.
http://svnbook.red-bean.com/nightly/en/svn.serverconfig.svnserve.html#svn.serverconfig.svnserve.sshtricks
If you want to use ssh key other than default key or alternative tcp port
other than 22, you can use them by overriding ssh tunnel setting with SVN_SSH
environment variable or config file, etc. (Of course, if you want to use non
standard port for ssh connection you also need to change configuration of
sshd on server side).
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>