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

Jakub Hrozek jhrozek at redhat.com
Fri Sep 3 13:25:11 UTC 2010


-----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-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Move-crypto-functions-into-its-own-subdir.patch
Type: text/x-patch
Size: 54027 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100903/fe4fd1fc/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-safe-copy-move-macros-for-uint16_t.patch
Type: text/x-patch
Size: 1687 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100903/fe4fd1fc/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Password-obfuscation-utility-functions.patch
Type: text/x-patch
Size: 21307 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100903/fe4fd1fc/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Fix-pysss-linking.patch
Type: text/x-patch
Size: 852 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100903/fe4fd1fc/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Python-bindings-for-obfuscation.patch
Type: text/x-patch
Size: 6480 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100903/fe4fd1fc/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-sss_obfuscate-tool.patch
Type: text/x-patch
Size: 8933 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100903/fe4fd1fc/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-Deobfuscate-password-in-back-ends.patch
Type: text/x-patch
Size: 5369 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100903/fe4fd1fc/attachment-0006.bin>


More information about the sssd-devel mailing list