Code review comment for lp:~zseil/pyopenssl/client_CA

Jean-Paul Calderone wrote:
> Review: Needs Fixing
> 1. In the tex docs for add_client_CA, two lines are added which each begin with "%" and repeat the word "Certificate" a number of times. What's this?

That was supposed to be a humorous reminder for myself that the text
should be clarified. It seems it failed in its purpose, I'll remove it
and try to clarify the text.

> 2. From the tex docs, I can't tell what the argument to set_client_CA_list should be a list of. I would have guessed X509Names, but since add_client_CA takes an X509, I'm not sure (the Python docstring clarifies this; it'd be good to have the information in both places).

Ok.

> 3. Why does add_client_CA take an X509 instead of an X509Name? (I suppose it's because that's the how the OpenSSL API works - lame)

Yes, the underlying OpenSSL API accepts certificates instead of
X509Names. Should I drop the whole add_client_CA method? It is
not strictly necessary.

Also, are the current method names ok? The preexisting method
load_client_ca_list has lowercase CA, should the new methods be
consistent with it rather than with the OpenSSL API?

> 4. I've been postponing looking at the cross-module type checking code that you replaced with "import_crypto_type" because it wasn't causing problems and I didn't want to think about it. However... now I've thought about it a little, and I think it should be simpler. If crypto_X509NameObj is defined, then crypto_X509Name_Type should be defined too, since they're defined in the same place. So the code should just use crypto_X509Name_Type (or crypto_PKey_Type or crypto_X509_Type). It doesn't need to do the import dance or check sizes or anything. There was probably a reason not to do it this way long, long ago, but I'm not very interested in it now.

crypto_XXX_Type's can't be used in the same way as crypto_XXXobj's. The
former are true runtime variables, while the latter are just struct
definitions. To use crypto_XXX_Types directly, the SSL module would have
  to be linked against the crypto module.

Other extension modules (Python's datetime for example) usually solve
this by providing the types in the CAPI struct pointer, and then using
some fairly ugly macros to make them look as normal variables. I can do
that, but we would loose ABI compatibility if we would reuse the current
crypto_XXX_Type names. I guess exposing them just with crypto_XXX_Check
macros would be enough?

> 5. Likewise, crypto_X509Name_Check should be defined for ssl_Context_set_client_CA_list to use.

I did try to use it, but that lead to linking errors and a few hours of
headscratching before I figured out what the difference between *Objs
and *Types was :)

>
> Thanks! I generally like the code and it doesn't seem to have any major problems. I'm looking forward to the next version of this.
>

Thank you for the review. I hope I got the workflow right, the status of
the merge request should be changed to resubmit when the fixes are made?
  Launchpad's interface is a bit confusing in this regard, I guess I
should report a bug...

Regards,
Ziga