-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 09/02/2010 05:43 PM, Stephen Gallagher wrote:
On 08/30/2010 02:38 PM, Jakub Hrozek wrote:
> [PATCH 1/7] Move crypto functions into its own subdir
> A refactoring patch that creates a common util/crypto subdir with
> per-implementation subdirectories for each underlying crypto library
> supported by SSSD.
Nack.
You added a manpage entry in this patch for sss_obfuscate. This belongs
in patch 6.
Otherwise it looks fine.
Botched git commit --amend probably. Thanks, fixed.
> [PATCH 3/7] Password obfuscation utility functions
> This is where the actual obfuscation happens.
> Adds two utility functions - one to obfuscate a password and inverse to
> extract the cleartext password back. So far, only NSS-based
> implementation is provided.
Nack.
Nitpick: please match up the whitespace here:
if HAVE_NSS
SSS_CRYPT_SOURCES = src/util/crypto/nss/nss_sha512crypt.c \
src/util/crypto/nss/nss_obfuscate.c \
src/util/crypto/nss/nss_util.c
SSS_CRYPT_CFLAGS = $(NSS_CFLAGS)
SSS_CRYPT_LIBS = $(NSS_LIBS)
else
SSS_CRYPT_SOURCES = src/util/crypto/libcrypto/crypto_sha512crypt.c
SSS_CRYPT_SOURCES = src/util/crypto/libcrypto/crypto_sha512crypt.c \
src/util/crypto/libcrypto/crypto_obfuscate.c
SSS_CRYPT_CFLAGS = $(CRYPTO_CFLAGS)
SSS_CRYPT_LIBS = $(CRYPTO_LIBS)
endif
You are using tabs instead of spaces.
Nitpick: s/execure/execute/ in nss_obfuscate.c (several places)
Your b64_encode function declaration is confusing. Please change the
variable name "obufsize" to make it clear that it is the size of the
input buffer to the function, not the output size.
The above are fixed.
I would also suggest that in the rare case where the
talloc_realloc()
fails to shorten the buffer in b64_encode(), we should probably just opt
to continue using the original buffer. A tiny waste of memory in this
case is likely preferable to failing. Especially since this memory is
unlikely to live long.
As discussed on IRC, I decided to remove the talloc_realloc call
altogether since it doesn't really do anything unless shrinking by 1024
bytes or more.
obfbuf is PORT_alloc'd by ATOB_AsciiToData() but you never call
PORT_free() on it.
That was a memleak, yes. Thank you, fixed.
We probably want to include a NULL byte (or some other,
more-complicated
terminator like "\0\1\2\3") at the end of the buffer that we can check
before blindly reading memory in. This way we can tell if we have a
corrupted or simply wrong obfuscated password.
Done
> [PATCH 6/7] sss_obfuscate tool
> A tool to add obfuscated passwords into the SSSD config file
Nack.
The default for the filename should be None. The SSSDConfig API
automatically treats None as "the default location".
We want to rely on this, since it's possible that other distributions
will use a different default location. Better if they only need to edit
that in a single place.
Also note the comment for patch 0001.
Both done.
> [PATCH 7/7] Deobfuscate password in back ends
> When obfuscated password is used in config file, the LDAP backend
> converts it back to clear text and uses it to authenticate to the
> server.
Nack.
Nitpick:
"Getting authtok is not supported with the crypto backend used,
authentication might fail!\n"
Please s/crypto backend used/crypto library compiled with/. "Backend"
carries its own connotations and had me confused for a moment.
Done.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora -
http://enigmail.mozdev.org/
iEYEARECAAYFAkyA9zcACgkQHsardTLnvCVDIQCeIuX+PwHwgjUh/5/bIWa/GwwJ
LJ8AoOMDqsGLv9Wm79F5aOQinwh0jwrk
=5bNu
-----END PGP SIGNATURE-----