-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Test vectors bip70 #69
Conversation
|
||
BitcoinJ : https://bitcoinj.github.io/payment-protocol#introduction | ||
|
||
==Tets vectors== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test
Although not standardized, there seems to be a tendency to use the .paymentrequest filename suffix for files containing BIP70 payment requests. It would make it easier to open your test vectors directly from a web browser. |
Fixed |
I'm not sure about the spec change at all. Are you sure you aren't just codifying some bug in the either the .NET or PHP protobuf library? The code in bitcoinj doesn't do anything special here and it verifies signatures just fine. |
I have not changed the spec with this BIP, I am respecting it. If it does not, then there need some clarification to make to the BIP. The question is : If the version_details is omitted in the wire, should it be included in the data that is signed ? From BIP70, it is since it specifies the signature include "All fields serialized in numerical order" (ie, even if version_details is omitted in the wire, the signature verification must include it) Again, this is not a protobuf implementation bug, just a point unclear in the spec. |
Does someone can put some light on this protocol ? The question is simple : If the response is yes, then you can pull this request, if the response is no, please make the BIP clearer. |
The simple answer (after writing a little test case to see): No, the signature of a PaymentRequest from the wire with details_version omitted is NOT the same as a request with details_version explicitly set to 1 and serialized. I'll put making the BIP clearer on my (long) TODO list... |
Perfect, so my test case is wrong. I got confused with the following sentence : I need to change the test vectors |
I clarified the BIP; see b2c0b87 |
Excellent, I'll rebase and fix my test vectors later this week, thanks ! |
Thanks Gavin. The behaviour is as I thought it was. |
I fixed NBitcoin, as well as the test vectors. I also added my own test certificate (+ private key) so implementers can verify if the signing process is correct. |
Excellent; I'll pull as soon as I (or somebody else) has time to make sure the test vectors work with another implementation. Question: when does the test certificate expire? |
It expires in 2064 |
mikehearn, can you try parse it from BitcoinJ ? |
Just tried most of them in bitcoinj. They fail with "java.security.cert.CertPathValidatorException: subject/issuer name chaining check failed" Did you get the order of the chain right? Also: If those are "vectors", what's the supposed outcome (asserts)? |
Here is a printout of the cert chain to analyze: X.509 Cert Path: length = 3. Key: Sun RSA public key, 1024 bits ] ] =========================================================Certificate 2 start. Key: Sun RSA public key, 2048 bits Certificate Extensions: 7 [2]: ObjectId: 2.5.29.35 Criticality=false ] [3]: ObjectId: 2.5.29.19 Criticality=true [4]: ObjectId: 2.5.29.31 Criticality=false [5]: ObjectId: 2.5.29.32 Criticality=false [6]: ObjectId: 2.5.29.15 Criticality=true [7]: ObjectId: 2.5.29.14 Criticality=false ] ] =========================================================Certificate 3 start. Key: Sun RSA public key, 2048 bits Certificate Extensions: 7 [2]: ObjectId: 2.5.29.35 Criticality=false ] [3]: ObjectId: 2.5.29.19 Criticality=true [4]: ObjectId: 2.5.29.31 Criticality=false [5]: ObjectId: 2.5.29.32 Criticality=false [6]: ObjectId: 2.5.29.15 Criticality=true [7]: ObjectId: 2.5.29.14 Criticality=false ] ] ] |
schildbach, thanks for your help, I effectively added chain certificate to the request that should not be here. |
Retried payreq1_sha1.paymentrequest and payreq2_sha1.paymentrequest, doesn't change anything. They still contain 3 certs each. |
Self signed certs are always rejected in BIP70 so I think it'd be better if we used a real cert, as we already do (I think) in the bitcoinj unit tests. I have a couple of SSL certs we can test with, if necessary. |
Do you have the private key of such cert mike ? |
Well they're private :) But for test vectors the private key should be unnecessary, all we're checking is that the signatures are verified correctly right? |
schildbach, this is strange, I double checked, the requests don't have 3 certificates. |
I'd like implementer to be able to test their signature algorithm. |
Alternatively I can create a custom Certificate chain, can you deactivate trust verification in bitcoinJ ? |
Yes but then what's the point? The tests are supposed to test verification, right? If necessary we can get a free SSL cert for some throwaway test domain and put the private key into the test vectors, but I really don't think that's needed. The test data should only be checking verification works. Implementors can then make sure their signatures and cert chains are accepted by the main implementations if they have doubts that they got it right. Experience tells us that the info on what certs to include isn't right: lots of people get this wrong, especially as Bitcoin Core is quite permissive about what it allows. |
No need for throwaway domain and free SSL, I can create an untrusted CA and emit a MerchantCertificate from it. For testing, trust checks and time validity checks should be off, so the vector don't suddenly become invalid. If you are willing to change tests to turn off these checks, then I can quickly generate a full chain. Does it sounds good ? |
So far most of the bugs people have hit are in chain construction and verification actually, so it does seem important to have various combinations in the test setups. OK, sure, we can require a custom root cert for unit tests. |
Mike, I will add one test with a valid chain using Gavin's web tool. |
Note that in bitcoinj, there is already such a custom root cert, along with the private key. |
Ok I updated everything, now all tests have a CA. By testing chain checking against my own custom chain, and Gavin's chain, I've uncovered some bugs in NBitcoin Chain verification, so that was definitively a good idea ;) Let me know if this pass correctly. |
I get java.security.cert.CertPathValidatorException: Path does not chain with any of the trust anchors. Guess that's expected. Can you provide your CA cert as a JKS keystore or use the one I linked to in my last comment? |
Just added a JKS with the CA inside |
Now I get java.security.InvalidAlgorithmParameterException: the trustAnchors parameter must be non-empty Can you try using the CA I linked? |
Send them in PFX format and I will. The password of NicolasDorierCA is "password" if it does not work, try using nothing. |
Or just remove trust check... |
Could you remove the trust check (except for the last vector that test the chain), or put the CA in a JKS store that work ? I added a test vector at the end just for chain trust checking. (From Gavin's implementation) |
Rebased. Can someone test these vectors without the trust check ? |
I am implementing BIP70 and really need these test vectors. Can you please confirm they are correct? |
The reason we haven't verified it yet is that Java doesn't make it easy to extract the public key from the chain when that chain isn't actually valid. At least the "obvious" way I tried didn't work. And this key store doesn't seem to be valid - the cert is not marked as being a CA cert, so obviously, it's not happy (that's what the error message means). Honestly I'm not convinced these test vectors are all that useful; we already have some test payment requests from Gavin's repo and demo site, along with "real world" tests like BitPay and Coinbase. The test vectors you produced are simply checking that protobufs works as we thought it worked and how it's documented to work - it must be your own .NET implementation that was non-conformant, so perhaps these tests are better re-cast as unit tests for whatever .NET protobuf library you're using rather than BIP70. |
Mike, if I created this test it is because it was a real pain in the ass to conform just from the spec. I did all I can to help the community, my tests are not here to test the chain trust algorithm (even if I added a valid test destined to that), but for checking you sign the right thing. I wasted hours, I would prefer other people would not. And the protobuf lib was not at fault, and was working fine. It tests some edge case that such "real world" implementation does not. And it was pure coincidence that BitcoinJ implemented right. (About the clarification Gavin's made in b2c0b87) A BIP without test vectors is dangerous. |
Well, I'm indeed very sorry that this took up a lot of your time, but that doesn't change the base facts here: your protobuf library was wrong and that's where the tests should go. Actually I was wondering how it was possible that protobuf-net deviated from Google's spec API and implementation in such a basic way, but I think I see the issue. The website says: Rather than being "a protocol buffers implementation that happens to be on .NET", it is a ".NET serializer that happens to use protocol buffers" i.e. the fact that it appears to use the same wire format as BIP 70 is just an implementation detail. IMHO that's a very confusing design decision by the protobuf-net authors and I'm not surprised it tripped you up. I'd suggest avoiding this library in future and using something that is "a protocol buffers implementation that happens to be on .NET", otherwise you might hit other oddities in future. BTW the reference implementation is in Bitcoin Core, not bitcoinj. I tested the bitcoinj implementation against the Bitcoin Core implementation when I wrote it. |
Mike Hearn, it was NOT because of the protobuf-net implementation. That was clarified by Gavin, and this is what my tests are testing. |
ALL protocol buffer libraries are meant to work this way and all the ones I've encountered do. Trust me on this, I was using protocol buffers almost every day for the last 7.5 years, long before they were open sourced. Look at the code generated by the reference implementations: the classes provide has_foo() accessors for optional fields. How could those ever return false if protobufs was meant to work the way you described? The library you used even explicitly states that it's not a protocol buffers implementation at all! Anyway, this is a pointless argument. We've now spelled out more of how protobufs work in BIP70, I feel like that is enough. The C++ implementation is in the src/qt directory, where the paymentrequest.proto file is. Look at paymentrequestplus.cpp and the PaymentRequestPlus::getMerchant() method specifically. |
Closing as there is no consensus to merge these. |
Removed reference to 0xc1 leaf version.
Some implementation links + test vectors