Are there any negative tests we should consider here?

I think the negative tests are the same as the "classic" certificate pinning case, i.e. if none of the server´s certificates is pinned, the connection should be cancelled. The same goes for expired and invalid certificates, I guess.

I can't think of a specific negative case for this particular issue. Any idea?

I couldn't think of any either, just wanted to pose the question.

🎉 🍻 🎉

@p4checo it looks like expiration day has come so the test suite is now failing.

Can you walk me through where you grabbed these two chains so I can get them updated?

Hi @kcharwood,

I used https://www.ssllabs.com/ssltest/analyze.html?d=google.com to validate whether it was using two chains, and then I got the certificates using OpenSSL:

openssl s_client -showcerts -connect google.com:443 </dev/null 2>/dev/null
(I think I ran the command multiple times for both chains to appear, but I'm not sure)

However, I've now checked that google.com seems to no longer have 2 certification paths 😓

How should we proceed here? I don't want to disable the test if I don't have too.

Ideally, we should find another case of a multiple certification path host and update the test. 😄

At the time I was lucky google.com itself was an example. But surely there are other cases.

K. I don't have a lot of time to dig on this issue at the moment, so I may need to disable it in the near term to allow the test suite to pass for some other issues I need to push through.

Me neither 😞. If I manage to put aside some time for this I'll let you know.

If we've set our pinned certs as the exclusive anchors in our trust store, why do we need to explicitly compare cert data at all?

NSMutableArray *pinnedCertificates = [NSMutableArray array];
for (NSData *certificateData in self.pinnedCertificates) {
    [pinnedCertificates addObject:(__bridge_transfer id)SecCertificateCreateWithData(NULL, (__bridge CFDataRef)certificateData)];
}
SecTrustSetAnchorCertificates(serverTrust, (__bridge CFArrayRef)pinnedCertificates);
 
return AFServerTrustIsValid(serverTrust);

What problem is the additional code solving?

The issue this PR was fixing was due to incomplete certificate chains being sent by the server and making certificate pinning fail precisely because of this certificate data comparison.

However, the point you raise is valid since the certificates are already defined as anchors, we shouldn't have to compare the data ourselves manually. That should be (and already is) done more efficiently by SecTrustEvaluate (via AFServerTrustIsValid), right?

So, that data comparison check (and my complete "fix") would be unnecessary because AFServerTrustIsValid would handle it automatically by using the defined anchors. I guess I didn't see the complete picture back then 😅

I heard they welcome PR's in this project, @RadimC 😉

Cheers! 🍻