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
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,184 @@
+#!/usr/bin/env python3
+"""\
+Script to store password in plaintext in ~/.subversion/auth/svn.simple/
+
+Useful in case Subversion is compiled without support for writing
+passwords in plaintext.
+
+Only use this script if the security implications are understood
+and it is acceptable by your organization to store passwords in plaintext.
+
+See http://subversion-staging.apache.org/faq.html#plaintext-passwords
+"""
+
+# ====================================================================
+# 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 os
+import sys
+
+TERMINATOR = b"END\n"
+
+PARSERDESCR = """\
+Store plaintext password in ~/.subversion/auth/svn.simple/
+
+Existing passwords and authentication realms can be inspected by:
+
+ svn auth [--show-passwords]
+
+The authentication realm can also be found using:
+
+ svn info URL
+"""
+
+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 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".
+ """
+ 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)
+ except:
+ os.remove(tmpFilename)
+ raise
+
+def main():
+ """Main function"""
+ # 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='?')
+ 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', file=sys.stderr)
+ parser.print_help()
+ return
+
+ # Prompt for password
+ import getpass
+ password = getpass.getpass("Enter password for realm {}:
".format(args.realm))
+
+ # In an existing file, we add/replace password/username/passtype
+ if existingFile:
+ hash = svn_hash_read(open(authfileName, 'rb'))
+ if args.user is not None:
+ hash[b'username'] = args.user.encode('utf-8')
+ hash[b'password'] = password.encode('utf-8')
+ hash[b'passtype'] = b'simple'
+
+ # For a new file, set realmstring, username, password and passtype
+ else:
+ hash = {
+ b'svn:realmstring': args.realm.encode('utf-8'),
+ b'username': args.user.encode('utf-8'),
+ b'passtype': b'simple',
+ b'password': password.encode('utf-8')
+ }
+
+ # Write out the resulting file
+ writeHashFile(authfileName, hash)
+
+
+if __name__ == '__main__':
+ main()
+
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
