xmlsec / python-xmlsec

Python bindings for the XML Security Library.
MIT License
95 stars 99 forks source link

Fix #244 - Fix failing test with libxmlsec-1.2.36, also make libxmlse… #251

Closed tdivis closed 8 months ago

tdivis commented 1 year ago

Fix failing test with libxmlsec-1.2.36, also make libxmlsec version available from Python.

tdivis commented 1 year ago

I rebased it on #250 who fixed precommit.ci error with Python 3.11, so this should merge after #250. Would be nice if someone who understands XML singing would look at my commit (b0f48a3c5e4393508b38cec052d0c657f1f11c55) and could confirm, that it's OK that<X509SubjectName> and <X509SKI> are empty. Maybe @lsh123 himself could help? :-)

lsh123 commented 1 year ago

If I understand the test correctly, the test signs file with an RSA key from "rsakey.pem". In this case, there is no certificate for the key thus there is nothing to write out any X509 information into the output. I am not sure if this is a good test but the current output is correct.

You should consider using a key + cert from a pkcs12 file as a more realistic example. That should populate the X509Data (you should probably remove the X509Certificate content as well so it is "filled in" by xmlsec during signing).

lsh123 commented 1 year ago

Side note -- would be great if you can test 1.3.0-rc candidates (latest is RC3: https://github.com/lsh123/xmlsec/discussions/612). There are quite a lot of changes and some are API/ABI breaking (https://github.com/lsh123/xmlsec/discussions/593).

tdivis commented 1 year ago

If I understand the test correctly, the test signs file with an RSA key from "rsakey.pem". In this case, there is no certificate for the key thus there is nothing to write out any X509 information into the output. I am not sure if this is a good test but the current output is correct.

There is also ctx.key.load_cert_from_file(self.path('rsacert.pem'), consts.KeyDataFormatPem), so certificate data should be there. Point is that with libxmlsec version prior to 1.2.36, the elementes <X509SubjectName> and <X509SKI> are filled but with 1.2.36 they are empty.

I found that you marked the elements DEPRECATED (in docs/api/xmlsec-x509.html), so I figured that this might be intentional. Is it?

lsh123 commented 1 year ago

Sorry I missed that there is a cert. In this case something is not correct. XMLSec should write out all the X509Data children nodes. Not sure what do you mean by "deprecated", which specific function was deprecated?

tdivis commented 1 year ago

Not sure what do you mean by "deprecated", which specific function was deprecated?

Not a function, but elements, you can see it in the commit from you I linked (github folded the docs/api/xmlsec-x509.html because the diff is too big), you can see it also here: https://github.com/lsh123/xmlsec/blob/01e95e393275fee0328e37ca14f522eedc640810/docs/api/xmlsec-x509.html#L94

lsh123 commented 1 year ago

Not sure what do you mean by "deprecated", which specific function was deprecated?

Not a function, but elements, you can see it in the commit from you I linked (github folded the docs/api/xmlsec-x509.html because the diff is too big), you can see it also here: https://github.com/lsh123/xmlsec/blob/01e95e393275fee0328e37ca14f522eedc640810/docs/api/xmlsec-x509.html#L94

Ah that's just to move this code from public to private :) It's internals and nobody should be using it outside of xmlsec itself.

lsh123 commented 1 year ago

This is current master (which will be 1.3.0 in a couple weeks). I removed bunch of stuff to make it easier to see the X509Data:

$ cat test.xml 
...
    <KeyInfo>
      <X509Data>
     <X509Certificate/>     
     <X509SubjectName/>
     <X509SKI/>
      </X509Data>
    </KeyInfo>
...
$ ./apps/xmlsec1 sign --lax-key-search --privkey-pem ./tests/keys/largersakey.pem,./tests/keys/largersacert.pem test.xml 
...
    <KeyInfo>
      <X509Data>
     <X509Certificate>MIIFYTCCBQugAwIBAgIJAK+ii7kzrdqwMA0GCSqGSIb3DQEBBQUAMIGcMQswCQYD
...
JdtBK1hZRzOc3RfF75OAl3yD+yqw1ZMM/3HML6fcLwWZHP0w+g==
</X509Certificate>      
     <X509SubjectName>emailAddress=xmlsec@aleksey.com,CN=Aleksey Sanin,OU=Test Large RSA Key,O=XML Security Library (http://www.aleksey.com/xmlsec),ST=California,C=US</X509SubjectName>
     <X509SKI>kDU2EVL5AGX8cedzsJHtCxmExig=
</X509SKI>
      </X509Data>
    </KeyInfo>
...
lsh123 commented 1 year ago

But indeed I see empty nodes in 1.2.37:

 <X509SubjectName/>
 <X509SKI/>

Let me investigate (https://github.com/lsh123/xmlsec/issues/616)

lsh123 commented 1 year ago

Thanks, it's indeed a change in behavior and the fix is here:

https://github.com/lsh123/xmlsec/commit/c4529d3661278aa1b4657d03406871bae3aa9f12

It shouldn't have any real world impact since XMLSec code in 1.2.36 / 37 was selecting the "best" output (X509Certificate first, then subject, then ....). But nevertheless, I will rollout 1.2.38 with this (and a couple other bug fixes) soon.

tdivis commented 1 year ago

So I added file with signed result for xmlsec-1.2.36 and 37 so tests will pass. Also added function for the version of xmlsec so we can do specific decisions on xmlsec version in clear way. Ready for CR.