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

[Roumen Petrov]

This is a multi-part message in MIME format.
--------------060506020600030608050303
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

  Thanks Aleksey,

Please find enclosed file "postx509data.diff.gz" - with xmlSecError 
fixes. At moment it's not necessary to be checked in CVS. I would like 
to add more changes. Please find my comments in quoted text.

Aleksey Sanin wrote:

> Roumen Petrov wrote:
>
>> please find attached file x509-sn_is__.patch.gz with latest changes.
>
> Well, this patch still has some problems:
>     0) The "writer" approach you use is less flexible than "flags" 
> approach
>     I have suggested. For example, it does not allow you to write *both*
>     subject name and issuer serial nodes which might be required.

O.K. I'm surprised - verify work when <X509Data> contain more than one 
tag ( as example X509SubjectName and X509Certificate) for same certificate.

>     1) The patch erases the content of the X509Data node when it writes
>     data out. This seems wrong to me. For example, application might want
>     to put a pointer common cert (say, with X509SubjectName) and write 
> key
>     specific certs in X509Certificate. The only way to do it is to 
> write X509SubjectName
>     node manually and put a placeholder for X509Certificate in the 
> template:
>        <X509Data>
>             <X509SubjectName>some subject</X509SubjectName>          
>             <X509Certificate/>
>        </X509Data>
>     I think it's a good idea to keep this ability to "add" data to 
> X509Data node as
>     we do now.

X509IssuerSerial node can contain two subnodes and check for empty 
should be more precise. Current source will remove content of non empty 
X509IssuerSerial node.
Between X509Data subnodes we can have text nodes. I think that we can 
use code:
==================
.........
    if(deleteCurNode) {
        /* remove "template" nodes */
        xmlNodePtr textnode;
        textnode = cur->prev;
        if (xmlNodeIsText(textnode)) {
            /*to check for empty content ?*/
            xmlUnlinkNode(textnode);
            xmlFreeNode(textnode);
        }
        xmlUnlinkNode(cur);
        xmlFreeNode(cur);
    }
    cur = next;
.........
==================
to remove empty lines.

I will check, fix and inform you for results.

Is allowed to have text in X509Data ?
<X509Data>
  Next tag contain issuer certificate:
  <X509SubjectName>some subject</X509SubjectName>          
  Document is signed by:
  <X509Certificate/>
</X509Data>

>     2) You changed the code to skip "empty" nodes. In most cases it 
> can cause no
>     harm but some applications may decide that it's not acceptable. I 
> think a flag
>     in xmlSecKeyInfoCtx should solve this issue.

At moment I'm not sure what is better:
- XMLSEC_KEYINFO_FLAGS_STOP_ON_EMPTY_NODE
or
- XMLSEC_KEYINFO_FLAGS_CONTINUE_ON_EMPTY_NODE
When we try to verify xml file, according to schema definition X509Data 
can be empty, but subnode cannot be empty. In this case I think that we 
should stop on empty subnode. This mode I denominate "plain reading".
When we try to sign xml we are in other mode "template reading". In this 
mode empty subnodes should be allowed.
I will try to find where to set up flag in the source code 
(xmlSecDSigCtxVerify/xmlSecDSigCtxSign  ....?).

>     3) There were few memory leaks in the *Read functions with the 
> same pattern:
>     get node content, check that it's empty, return 0 w/o freeing content.

Thanks - four eyes are better than two. I fix silently one memory leak 
with new method xmlSecOpenSSLX509CRLNodeWrite (buf isn't released in old 
code) but blind Roumen introduce many new :-).

>     4) After fixing the code I realized that the new xmlSecKeyInfoCtx 
> flag
>     XMLSEC_KEYINFO_FLAGS_X509DATA_DONT_WRITE_CRLS you have added
>     is not necessary. If application does not want to write CRLs then 
> it just uses
>     following template:
>        <X509Data>
>             <X509Certificate/>
>        </X509Data>

O.K.

> Anyway, I have fixed all the issues above (see attached patch). It is 
> checked in CVS
> along with a new test case to make sure it actually works. I hope that 
> Tej would be able
> to do similar thing in xmlsec-nss.
>
> Roumen, I would appreciate if you can try this new code and check that 
> after my changes
> it is still doing what you need.

What is better:
- if(xmlSecIsEmptyNode(cur) == 1)
or
- if(xmlSecIsEmptyNode(cur) != 0)
?

>> ======================================================
>> Miscellaneous:
>>
>> In a method we can put part of code in "figure brackets" to make 
>> source more readable.
>
> Well, your points are valid. The only thing I have to object is that 
> there are already
> a lot of code in xmlsec written w/o using this trick. It makes very 
> difficult (for me at least)
> to switch from one code style to another inside the same source file. 
> I am trying to preserve
> the exisiting code style. It makes much more easier to read the code 
> when  everything
> is written in one style. Even if this style is not the best one :)
>
In method xmlSecOpenSSLKeyDataX509XmlWrite from new code section 
"determine the current node content" is exactly for my "figure brackets" 
coding style, but I PREFFER TO KEEP EXISTING XMLSEC STYLE, i.e. without 
"figure brackets code blocks" ;-)

>
> Aleksey
> <SNIP>

Roumen


--------------060506020600030608050303
Content-Type: application/gzip;
 name="postx509data.diff.gz"
Content-Transfer-Encoding: base64
Content-Disposition: inline;
 filename="postx509data.diff.gz"

H4sICIhCJj8AA3Bvc3R4NTA5ZGF0YS5kaWZmAK1S74+aQBD9DH/FhKQJFjnwV1SuJipHLRXh
Atf0+olwsBhSXC67aGz6z3cX0Dur196lnUTGYecN770dGydobwAlsVY8Ikxpru0H+vgqFif/
HqJvBpBmOTJAi3dUW+Nig7T9Jqco1s4/2d6JBJUkQ7sMr4GwRLMCQ+eqNxKTLE1B3YJKeAkN
R1VVL1AXuiP4vM2hq+s96IwNfWgMBqDqLIRqmKIoF2HjJ5g+NvoDozesYeJ0Cmp/0B6CUj2n
UxHgPf/9p9BEoGVUZjFkuBQEZlGAYo/RCwLnnrG7icrILRLkoygRBLk+X6If/P1tSSBhuS2q
wEJoAlgTh/BjzHJbVF4+OkHVo22cFma55z3fj1Xr+i9ETUTKLM3iqERHvi/R/QNV7veoP+KG
14k7LnB5WSqzJYEPoLfgJx9RD7cIKYh8v3ICywwt3/f8IPxk+Vb9FQ581keDKEUB2zS8PuW2
zCNKF6h0ow2Ss6TVYp42cOlMarC0OemvJCuRVLt7udH0neeNB0KnZP2wqT/ObMe6eWqTHgs6
eZdIbWB/uP+CwAzYEiyrHVZyp7qdejebXG3nwSoKkwm4XxyntusVbnEoB1TaKwazwO2Etntn
LSw/LItw7tZ6D3JXUZ4XsXRE/65sNXMc70TZeZfrhSsrCGYLq9I4t70wJQiFbLS8QZuj0lG3
VlrlaiveJKkppLkbPuBugjhr9VX3oVzuMv1vt3fe27S5lTT5AVdlc5vVHV2LvwCUjHwbkQUA
AA==
--------------060506020600030608050303--