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
