[xmlsec] Re: core methods for write of <X509SubjectName/> and <X509IssuerSerial/>

[Roumen Petrov]

  Hi Aleksey,

please find attached file x509-sn_is__.patch.gz with latest changes.

======================================================

> On 18 Jul 2003 Aleksey Sanin wrote:
> <SNIP>
> 1) xmlSecOpenSSLX509NameWrite() function: xmlMalloc may fail. You need
> to check that returned pointer is not NULL and return an error if it's 
> the case.

fixed

> 2) xmlSecOpenSSLASN1IntegerWrite() function: the ASN1_INTEGER_to_BN()
> may return NULL. Instead of assert you should use if() to check the 
> result. 

fixed

> Also I wonder why do you use '_xxx' variable? Why do you need '_'?

now method args are without leadnig '_'

> 2) xmlSecOpenSSLASN1IntegerWrite() function: The function returns
> xmlChar* allocated using OpenSSL function BN_bn2dec(). This is wrong!
> xmlChar* is assumed to be allocated with one of LibXML2 malloc functions
> and is freed with xmlFree. If there is a different memory callbacks 
> initialized
> in LibXML2 this code would crash.

fixed

> 3) testDSig.sh: I don't see reasons to modify existing tests. The 
> right way is to add
> new tests to the suite to test new functionality.

removed

> IMHO, the better approach would be:
> 0) At the very beggining of the xmlSecOpenSSLKeyDataX509XmlWrite()
> function you read the <X509Data/> node content and determine what do 
> you want
> to write (certs, subject names, ...) based on the content of 
> <X509Data/> node
> and the xmlSecKeyInfoCtx flags.

new method xmlSecGetX509NodeWriter

> 1) Create separate methods like:
>       xmlSecOpenSSLX509CertificateNodeWrite
>       xmlSecOpenSSLX509SubjectNameNodeWrite
>       xmlSecOpenSSLX509IssuerSerialNodeWrite
>       xmlSecOpenSSLX509SKINodeWrite

implemented

> 2) Call one these methods from the for() loop in 
> xmlSecOpenSSLKeyDataX509XmlWrite()
> for each cert in the keys data.

implemented call for selected method

> 3) Determine if you want to write CRLs 
> (XMLSEC_KEYINFO_FLAGS_X509DATA_DONT_WRITE CRLS
> flag in the xmlSecKeyInfoCtx and call the new
>       xmlSecOpenSSLX509CRLNodeWrite
> function for each CRL in xmlSecOpenSSLX509Data if needed.

implemented


======================================================

> On 24 Jul 2003 Aleksey Sanin wrote:
> <SNIP>
>     1) I don't like the way you implemented the "empty" check in 
> *Read() functions.
>     IMHO, this is a bad coding style to repeat the same code again 
> and  again.
>     Probably a small internal static function
>         int xmlSecOpenSSLX509IsEmpty(xmlChar*)
>     would be better :)

:-) yes of course - new method xmlSecIsWhiteSpaceString

>     Also I am not sure I understand why you put "XXX" comments around 
> it. Seems
>     useless to me.

now code has real comments instead of XXX and calls for 
xmlSecIsWhiteSpaceString

>     2) You are using the figure brackets to mark block of code all the 
> time
>     (I meant the "write Issuer Name" block in the example bellow):
>      
> +   if(cur == NULL) {
> +       cur = xmlSecAddChild(node, xmlSecNodeX509IssuerSerial, 
> xmlSecDSigNs);
> +       ....
> +    }
> +
> +    { /*write Issuer Name*/
> +       for(node_in = xmlSecGetNextElementNode(cur->children);
> +       ....
> +    }
>
>     Please don't do this. It makes code difficult to read. 

now code is without "figure brackets", but I dont agree that code in 
brackets is difficult to read. See comments at end of mail

>     3) In xmlSecOpenSSLX509NameWrite() function I wonder if there is a 
> way to
>     print name to a buffer, not memory BIO. Mallocs might be expensive 
> :( But I guess
>     the answer is "no".

somebody experienced in BIO and libxml2 architecture might can write 
method BIO_xmlNode for reading/writind data direct from/to xml node 
content, but I'm not that person :-(

>     4) Which OpenSSL version do you use? I wonder if this new code 
> works with OpenSSL 0.9.6.

:-[  code is compiled with OpenSSL 0.9.6e and 0.9.7b




======================================================
Miscellaneous:

In a method we can put part of code in "figure brackets" to make source 
more readable.
Suppose we have method:
int method() {
  int A, .../*5 variables to compute A*/;
  int B, .../*variables to compute B which cannot be shared with already 
defined*/;

  /*code to compute A*/
  ......./*N lines code */
  /*code to compute B*/
  ......./*K lines code */

  return(...);
}

My opinion is to rewrite like this.

int method() {
  int A;
  int B;

  { /*code to compute A*/
    definition of  5 necessary variables to compute A
    ..../*N lines code */
  }

  { /*code to compute B*/
    definition of  variables necessary to compute B
    ..../*K lines code */
  }

  return(...);
}

Code in brackets is complete and I can find easy start/end of 
"code-block". This style has other advantages as easy to modify, low 
stack requrenment.



Best regards
Roumen Petrov
-------------- next part --------------
A non-text attachment was scrubbed...
Name: x509-sn_is__.patch.gz
Type: application/gzip
Size: 3916 bytes
Desc: not available
Url : http://www.aleksey.com/pipermail/xmlsec/attachments/20030728/7f176fa5/x509-sn_is__.patch.bin