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
Index: contrib/client-side/store-plaintext-password.py =================================================================== --- contrib/client-side/store-plaintext-password.py (nonexistent) +++ contrib/client-side/store-plaintext-password.py (working copy) @@ -0,0 +1,120 @@ +#!/usr/bin/env python3 + +# Script to store password in plaintext in ~/.subversion/auth/svn.simple/ +# +# ==================================================================== +# 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. +# ==================================================================== + +import hashlib +import argparse +import os +import getpass + +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) != int(line[2:]): + raise Exception('Parse failed, invalid key length {} expected {}'.format(len(key), 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) != int(line[2:]): + raise Exception('Parse failed, invalid value length {} expected {}'.format(len(value), line[2:])) + + # 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()""" + with open(filename + '.tmp', 'x', encoding='utf-8') as file: + for key, val in hash.items(): + file.write('K '.format(len(key.encode('utf-8')))) + file.write('{}\n'.format(key)) + file.write('V {}\n'.format(len(val.encode('utf-8')))) + file.write('{}\n'.format(val)) + file.write('END\n') + os.rename(filename + '.tmp', filename) + +# 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) +parser.add_argument('realm', help='Server authentication real') +parser.add_argument('-u', '--user', help='Set username', nargs='?') +args = parser.parse_args() +realm = args.realm +password = getpass.getpass("Enter password: ") +if args.user is not None: + username = args.user + +# The file name is the md5encoding of the realm +m = hashlib.md5() +m.update(realm.encode('utf-8')) +authfileName = os.path.join(os.path.expanduser('~/.subversion/auth/svn.simple/'), m.hexdigest()) + +# In an existing file, we add/replace password/username/passtype +if os.path.isfile(authfileName): + hash = readHashFile(authfileName) + if args.user is not None: + hash['username'] = username + hash['password'] = password + hash['passtype'] = 'simple' +else: + if args.user is None: + print('Username required', file=sys.stderr) + quit() + hash = {} + hash['svn:realmstring'] = realm + hash['username'] = username + hash['passtype'] = 'simple' + hash['password'] = password +writeHashFile(authfileName, hash) Property changes on: contrib/client-side/store-plaintext-password.py ___________________________________________________________________ Added: svn:eol-style ## -0,0 +1 ## +native \ No newline at end of property Added: svn:executable ## -0,0 +1 ## +* \ No newline at end of property
