XMLSec 1.3.0-rc2 #595
Replies: 3 comments 22 replies
-
|
This builds. :-) When running the libreoffice tests, I hit a case where we insert some certificates and a signing certificate from code, try to sign a template and that works with 1.2.37 but not with 1.3.0-rc2. I'm not sure if the problem is on your end or on our end. Let me describe what we try to do and perhaps you can guess; if not, then I'll try to write some standalone reproducer (if you want). First, here is the xmlDocDump(() output for the template we try to sign: (In the unlikely case you would be interested in building the libreoffice code, the local change is: and to run the problematic testcase from the problematic suite: But I guess you want to avoid building that codebase. :-) ) Anyhow, when we use the NSS backend on Linux, we call xmlSecDSigCtxSign(), the output we get is: So far I only investigating that NULL certsList. It seems 1.2.x had no assert for that, at least in 1.2.x I guess the equivalent NSS function was xmlSecNssX509StoreFindCert_ex(), and there I see in gdb that also ctx->certsList is NULL, but no error is emitted. Perhaps the first question would be if I'm OK to ignore that assertion failure for now (because it's not relevant for why the whole signing fails) or not. If it helps, here is the testcase: I.e. we load the certs & signing key from DER files, try to sign and for some reason now this stopped working. Do you have a hint where to start debugging this? It's also possible that our test .der files are in some strange format and that needs changing. I also thought about trying to bisect this, if I can find out how to generate a release tarball from a given commit. Thanks. |
Beta Was this translation helpful? Give feedback.
-
Yes, that eliminated the warning, thanks. So the new output when callingxmlSecDSigCtxSign() is:
Hmm, there is a single place where we already set there, the output is still the same.
When we add certificates, we use NSS functions to add them. CERT_DecodeCertFromPackage() creates a CERTCertificate*, then PK11_ImportCert() does the actual work. See the code here: When we import a signing certificate (certificate + private key), then we import the private key to have a SECKEYPrivateKey*, see the code here: and the above code also imports the matching certificate. Anyhow, thanks for the clarification on the cert list warning, at least I now know that was not the root cause, I'll try to investigate how xmlSecKeysMngrGetKey() used to work in 1.2 and not in 1.3 for this use-case. |
Beta Was this translation helpful? Give feedback.
-
|
Ah that makes sense, I was looking for XMLsec key loading functions and didn't find any :) Anyway, I understand why 1.3.0 doesn't work and there is an improvement I am going to make to improve the situation (namely, ensure to search NSS keys db using X509Data as well). However, I don't understand how it is working with xmlsec 1.2.x. In order to search the key in NSS database, one needs to have a key name (and this code was there forever): Until xmlsec 1.3.0, then X509Data was not used to search the key so I am really puzzled to understand how it worked before. Would it be possible to put a breakpoint or print a debug statement in xmlSecNssKeysStoreFindKey (for xmlsec 1.2.x) and see what is the key name value when it;s called? I believe in this test it should be called once and key name to search for should be NULL. |
Beta Was this translation helpful? Give feedback.
-
|
@vmiklos could you please try PR #598 -- it should fix the test (and key search) for 1.3.0. I am still very much puzzled by how 1.2.x finds the key in this test. |
Beta Was this translation helpful? Give feedback.
-
|
Any chance you can run tests with 1.2.x and put a breakpoint in xmlSecNssPKIKeyDataAdoptKey() here: https://github.com/lsh123/xmlsec/blob/xmlsec-1_2_x/src/nss/pkikeys.c#L153 And share the stack trace? it will give us a hint where the private key for the signature is coming from. |
Beta Was this translation helpful? Give feedback.
-
|
BTW, by setting XMLSEC_KEYINFO_FLAGS_X509DATA_DONT_VERIFY_CERTS you are completely disabling all verifications for certs which is not a good thing to do unless you are doing post-verification yourself |
Beta Was this translation helpful? Give feedback.
-
Yes, that's the case here, we ask NSS / mscrypto to do the verification separately so we can inform the user if the failure is around the certificates or something else (digest mismatch, etc). It was done like this before my time (in openoffice), but given that the verification does happen, I think it's reasonable. For everything else: thanks, will try out and report back. :-) |
Beta Was this translation helpful? Give feedback.
-
|
Ah cool, that makes sense. Alternatively, I am going to add a more detailed reason to xmlSecDsigCtx and xmlSecEncCtx (likely after 1.3.0) -- see issue #600 |
Beta Was this translation helpful? Give feedback.
-
Thanks, this indeed fixes all reported errors from xmlsec (all of them were during xmlSecDSigCtxSign() previously). The test still fails, but now it's some assert on our side, so I need to investigate: I'll check what is the output from the template we give to xmlSecDSigCtxSign(), if that's unchanged then the root cause will be on our side, I guess.
First hit: Next hit: Next hit: Next hit: Next hit: Last hit: |
Beta Was this translation helpful? Give feedback.
-
The 1.2 output is like this: The 1.3 output is like this: The diff is: Looks like one cert used to be listed only once with 1.2 but is listed 3 times with 1.3. Should I tweak things on our side to deduplicate the cert list or is this unintentional in xmlsec and would be rather better to fix it there? Thanks. |
Beta Was this translation helpful? Give feedback.
-
|
First, thanks a lot for the tests!
I wonder if this is caused by multiple certs :) |
Beta Was this translation helpful? Give feedback.
-
|
Thanks, error was telling :) PR #609 should make it work for you. Sorry I usually don't test without certs verification like you are using and this is why we run into all these issues. I added a high level failure reasons here: I would recommend to consider switching to use xmlsec certs verification over time. I can work with you to get the information propagated for error messages as needed. Honestly using XMLSEC_KEYINFO_FLAGS_X509DATA_DONT_VERIFY_CERTS scares me a lot :) |
Beta Was this translation helpful? Give feedback.
-
Indeed, great. :-) I'll go back to a full 'make check' to see if I find anything else, but hopefully it'll be now all fine.
I'm not against that at all, though probably separate from the 1.2 -> 1.3 upgrade. My understanding is that the openoffice dsig code had its own cert verify code so that the UI can tell if the dsig verification failed in general, or there was a problem with the cert verification specifically. (And we inherited this in libreoffice, unchanged.) If we can keep telling the users about this difference, replacing own code with library code sound great. I'm a bit confused about |
Beta Was this translation helpful? Give feedback.
-
|
Well currently xmlsec wouldn't even check signature if cert verification failed :) But as I said, I would love to work with you to figure out how to make it work for your use case. And I argee, 1.2 -> 1.3 is big enough on its own :) |
Beta Was this translation helpful? Give feedback.
-
OK, I'll start a separate discussion/thread post 1.3; possibly we can even accept that way (do cert verify first, and only then sig verify), as long as we change documentation on our side. Anyhow, as a data point, there was a last libreoffice testcase that failed with xmlsec 1.3, but it was a PDF signing one, so really just worked by accident before (I would say), and other than that, at least Linux/nss 'make check' now passes for me locally with xmlsec 1.3. If that's interesting for you, I can do similar integration testing for mscng with a next xmlsec 1.3 RC. And thanks again for your fixes that help our use-case. |
Beta Was this translation helpful? Give feedback.
-
|
great, thank you! and yes more tests is good! :) |
Beta Was this translation helpful? Give feedback.
-
Soruce tar file:
Changes from 1.3.0-RC1
include/xmlsec/x509.h(thanks @vmiklos for bug report)XMLSec 1.3.0-RC1
The XMLSec 1.3.0 release includes a large number of changes including several API / ABI breaking changes (hence version bump). Please test the release candidate and let me know if you run into any issues! I plan to have at least one more release candidate in 2-3 weeks before the release mid-April.
core xmlsec and all xmlsec-crypto libraries:
xmlsec-openssl library:
xmlsec-nss library:
xmlsec-gnutls library:
xmlsec-mscng library:
xmlsec-mscrypto library:
xmlsec-gcrypt library:
xmlsec command line utility:
This discussion was created from the release 1.3.0-rc2.
Beta Was this translation helpful? Give feedback.
All reactions