[SSSD] [PATCH] Support password obfuscation in sssd.conf

Stephen Gallagher sgallagh at redhat.com
Thu Sep 2 15:43:19 UTC 2010


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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.

> 
> [PATCH 2/7] Add safe copy/move macros for uint16_t
> These will be used later on in the code

Ack.

> 
> [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.

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.

obfbuf is PORT_alloc'd by ATOB_AsciiToData() but you never call
PORT_free() on it.

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.

> 
> [PATCH 4/7] Fix pysss linking
> I noticed that the python bindings were not properly linked so a simple
> import pysss failed
> 

Ack.

> [PATCH 5/7] Python bindings for obfuscation
> Provides python bindings for utility functions from patch 3/7. These
> will be used later on in the obfuscation tool itself.
> 

Ack.

> [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.


> [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.



- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkx/xhcACgkQeiVVYja6o6MoTQCgn21l2gFHJ1SEBYEGIQxIuoJ7
MioAniLChGJ72oFlmQQ6uh2oknsPZ8C8
=cH5s
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list