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

Revision history for this message
Jean-Paul Calderone (exarkun) wrote :

  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?
  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).
  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)
  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.
  5. Likewise, crypto_X509Name_Check should be defined for ssl_Context_set_client_CA_list to use.

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.

review: Needs Fixing

« Back to merge proposal