Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Mar 12, 2024

Conversation

tdivis
Copy link
Contributor

@tdivis tdivis commented Mar 22, 2023

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

@tdivis tdivis force-pushed the 244-fix-test-xmlsec-1-2-36 branch from ff60c2f to b0f48a3 Compare March 22, 2023 19:58
@tdivis
Copy link
Contributor Author

tdivis commented Mar 22, 2023

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 (b0f48a3) and could confirm, that it's OK that<X509SubjectName> and <X509SKI> are empty. Maybe @lsh123 himself could help? :-)

@lsh123
Copy link

lsh123 commented Mar 22, 2023

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
Copy link

lsh123 commented Mar 22, 2023

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

@tdivis tdivis force-pushed the 244-fix-test-xmlsec-1-2-36 branch from b0f48a3 to 2a1bf30 Compare March 23, 2023 09:03
@tdivis
Copy link
Contributor Author

tdivis commented Mar 23, 2023

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
Copy link

lsh123 commented Mar 23, 2023

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
Copy link
Contributor Author

tdivis commented Mar 23, 2023

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
Copy link

lsh123 commented Mar 23, 2023

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
Copy link

lsh123 commented Mar 23, 2023

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
Copy link

lsh123 commented Mar 23, 2023

But indeed I see empty nodes in 1.2.37:

 <X509SubjectName/>
 <X509SKI/>

Let me investigate (lsh123/xmlsec#616)

@lsh123
Copy link

lsh123 commented Mar 23, 2023

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

lsh123/xmlsec@c4529d3

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 tdivis force-pushed the 244-fix-test-xmlsec-1-2-36 branch from 2a1bf30 to 9e052ab Compare March 27, 2023 08:16
@tdivis tdivis force-pushed the 244-fix-test-xmlsec-1-2-36 branch from 9e052ab to 2c58d43 Compare April 12, 2023 14:09
@tdivis
Copy link
Contributor Author

tdivis commented Apr 13, 2023

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.

@jimjag jimjag merged commit 2c58d43 into xmlsec:master Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants