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) ?
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" ;-)====================================================== 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 :)
Aleksey <SNIP>
Roumen
postx509data.diff.gz
Description: application/gzip
