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...@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
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,189 @@
+#!/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".
+
+ The keys and values must have datatype 'bytes' and strings must be
+ encoded using utf-8.
+ """
+ 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 {!r} already exist. Is the script already
running?\n'.format(argv[0], tmpFilename), file=sys.stderr)
+ 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='?')
+ 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)
+ 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'),
+ }
+
+
+ del password
+
+ # 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
