-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
[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.
[PATCH 2/7] Add safe copy/move macros for uint16_t These will be used later on in the code
[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.
[PATCH 4/7] Fix pysss linking I noticed that the python bindings were not properly linked so a simple import pysss failed
[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.
[PATCH 6/7] sss_obfuscate tool A tool to add obfuscated passwords into the SSSD config file
[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.
-----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 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 SIGNED MESSAGE----- Hash: SHA1
Patch 0001: Ack. Patch 0002: Ack.
Patch 0003: Nack
[PATCH 3/7] Password obfuscation utility functions
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.
You changed the wrong variable name. The ofbufsize argument is describing the size of the input buffer, not the size of the output buffer (which is allocated internally by BTOA_DataToAscii)
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.
A good start, but I think you missed the point a little bit. I want us to read up to ctsize, then test that the four bytes p+ctsize are the sentinel.
This way if the buffer is corrupt or malicious (e.g. the ctsize == MAX_UINT16) we aren't suddenly allocating and copying 64MB of data.
Patch 0004: Ack. Patch 0005: Ack. Patch 0006: Ack. Patch 0007: Ack.
Almost there :)
- -- 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 SIGNED MESSAGE----- Hash: SHA1
On 09/03/2010 08:16 PM, Stephen Gallagher wrote:
Patch 0001: Ack. Patch 0002: Ack.
Patch 0003: Nack
[PATCH 3/7] Password obfuscation utility functions
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.
You changed the wrong variable name. The ofbufsize argument is describing the size of the input buffer, not the size of the output buffer (which is allocated internally by BTOA_DataToAscii)
Ah, OK, I though you were referring to the buffer name as to indicate whether the conversion is done in-place or not, I though that the size variable was clear since its pass-by-value. Anyway, changed, hopefully the declaration looks clear now.
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.
A good start, but I think you missed the point a little bit.
I did :-)
I want us to read up to ctsize, then test that the four bytes p+ctsize are the sentinel.
This way if the buffer is corrupt or malicious (e.g. the ctsize == MAX_UINT16) we aren't suddenly allocating and copying 64MB of data.
Agreed, this is better, fixed now.
Patch 0004: Ack. Patch 0005: Ack. Patch 0006: Ack. Patch 0007: Ack.
Almost there :)
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/06/2010 09:10 AM, Jakub Hrozek wrote:
On 09/03/2010 08:16 PM, Stephen Gallagher wrote:
[PATCH 3/7] Password obfuscation utility functions
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.
You changed the wrong variable name. The ofbufsize argument is describing the size of the input buffer, not the size of the output buffer (which is allocated internally by BTOA_DataToAscii)
Ah, OK, I though you were referring to the buffer name as to indicate whether the conversion is done in-place or not, I though that the size variable was clear since its pass-by-value. Anyway, changed, hopefully the declaration looks clear now.
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.
A good start, but I think you missed the point a little bit.
I did :-)
I want us to read up to ctsize, then test that the four bytes p+ctsize are the sentinel.
This way if the buffer is corrupt or malicious (e.g. the ctsize == MAX_UINT16) we aren't suddenly allocating and copying 64MB of data.
Agreed, this is better, fixed now.
Sorry, I noticed two more things (warnings from gcc):
In file included from /home/bos/sgallagh/workspace/sssd/src/util/util.h:38, from /home/bos/sgallagh/workspace/sssd/src/util/crypto/nss/nss_obfuscate.c:30: ./config.h:91:1: warning: "HAVE_LONG_LONG" redefined In file included from /usr/include/nspr4/prtypes.h:58, from /usr/include/nspr4/prerror.h:41, from /home/bos/sgallagh/workspace/sssd/src/util/crypto/nss/nss_obfuscate.c:24: /usr/include/nspr4/prcpucfg.h:807:1: warning: this is the location of the previous definition
and also:
/home/bos/sgallagh/workspace/sssd/src/util/crypto/nss/nss_obfuscate.c: In function ‘sss_password_encrypt’: /home/bos/sgallagh/workspace/sssd/src/util/crypto/nss/nss_obfuscate.c:287: warning: cast discards qualifiers from pointer target type
- -- 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 SIGNED MESSAGE----- Hash: SHA1
On 09/07/2010 04:20 PM, Stephen Gallagher wrote:
Sorry, I noticed two more things (warnings from gcc):
Thanks, new patches are attached.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/07/2010 12:42 PM, Jakub Hrozek wrote:
On 09/07/2010 04:20 PM, Stephen Gallagher wrote:
Sorry, I noticed two more things (warnings from gcc):
Thanks, new patches are attached.
Ack.
- -- 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 SIGNED MESSAGE----- Hash: SHA1
On 09/08/2010 09:26 AM, Stephen Gallagher wrote:
On 09/07/2010 12:42 PM, Jakub Hrozek wrote:
On 09/07/2010 04:20 PM, Stephen Gallagher wrote:
Sorry, I noticed two more things (warnings from gcc):
Thanks, new patches are attached.
Ack.
All seven patches pushed to master.
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
Stephen Gallagher wrote:
On 09/08/2010 09:26 AM, Stephen Gallagher wrote:
On 09/07/2010 12:42 PM, Jakub Hrozek wrote:
On 09/07/2010 04:20 PM, Stephen Gallagher wrote:
Sorry, I noticed two more things (warnings from gcc):
Thanks, new patches are attached.
Ack.
All seven patches pushed to master.
And as a follow up question (unfortunately did not have time to read the patch, sorry) : Is there any limitation on the length or characters that can be included in the password being obfuscated?
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/08/2010 06:01 PM, Dmitri Pal wrote:
And as a follow up question (unfortunately did not have time to read the patch, sorry) : Is there any limitation on the length or characters that can be included in the password being obfuscated?
No theoretical limit, I just tested a string 2048 chars long and everything seems fine.
Jakub
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/08/2010 06:10 PM, Jakub Hrozek wrote:
No theoretical limit
This is not accurate, actually, as the length indicator in the buffer is uint32_t - that is the limit..
Jakub Hrozek wrote:
On 09/08/2010 06:10 PM, Jakub Hrozek wrote:
No theoretical limit
This is not accurate, actually, as the length indicator in the buffer is uint32_t - that is the limit..
This is a good limit to have :-)
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Jakub Hrozek wrote:
On 09/08/2010 06:01 PM, Dmitri Pal wrote:
And as a follow up question (unfortunately did not have time to read the patch, sorry) : Is there any limitation on the length or characters that can be included in the password being obfuscated?
No theoretical limit, I just tested a string 2048 chars long and everything seems fine.
Jakub
Good, thanks! I imagine a unit test that generates a password of X length in the loop from X =1 to 10K and then it gets obfuscated, then unwrapped and matched to the original value. Just a thought...
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel@lists.fedorahosted.org
-
Dmitri Pal -
Jakub Hrozek -
Stephen Gallagher