[SSSD] [PATCH] libcrypto fully implemented

Stephen Gallagher sgallagh at redhat.com
Mon Jun 25 12:08:47 UTC 2012


On Tue, 2012-06-19 at 13:28 -0500, George McCollister wrote:
> I implemented missing libcrypto functionality.


Thank you very much for this patch, George!

This isn't a blocker for this patch, but would you mind expanding the
base64 encode/decode tests? I'd like to see a situation where you read a
dozen or so random bytes and then pass it through both encode and decode
to confirm that it returns the original byte array. If we do this with
random data, it could conceivably discover issues that are not present
when managing only a known ASCII string. As I said, I'm not nacking the
patch based on this, but it would be a nice future enhancement to the
tests.

Ok, so a few NACKs:

Minor: you have non-zero-length lines containing only whitespace. This
is a style violation (and shows up bright red in my VIM).

In sss_base64_encode(), you return NULL if
bmem = BIO_new(BIO_s_mem());
fails, but this leaks the memory of b64.

Please use more whitespace to logically separate your actions in
sss_base64_encode() and sss_base64_decode(). They're bunched together
and it's difficult to see what actually goes together.

In sss_base64_decode(), please have tmp_ctx be a child of NULL, not
mem_ctx. This makes it possible for valgrind to detect memory leaks.

When you do
in_dup = talloc_size(mem_ctx, inlen+1);
this should be parented to tmp_ctx, not mem_ctx. You're leaking this.

If you 'goto done' in multiple places, you are leaking memory created by
BIO_new(). Please make sure that you clean up after yourself on error.


Other than a blank line with whitespace, the HMAC-SHA1 portion looks
good.

The configure script warns that building with libcrypto will not support
password obfuscation, but your patch implements it. One of these are
lying :) (For the record, if you opted not to implement this misfeature,
I wouldn't have shed a tear).

As above, please allocate tmp_ctx on NULL for improved memory-leak
detection. The only time mem_ctx should ever be used is when all data is
finalized and ready to be returned.

Please use SSSDBG_* macros when adding DEBUG messages. They are more
readable (and help us actually stick to reasonable meanings for the
levels). In sss_password_encrypt() they should be SSSDBG_CRIT_FAILURE
and SSSDBG_TRACE_FUNC



-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120625/154e1e32/attachment.sig>


More information about the sssd-devel mailing list