DiUS/pact-consumer-swift

Self signed SSL certificates do not work

georgescumihai opened this issue · 12 comments

georgescumihai commented

starting the pact-mock-service with --ssl will create a self signed certificate, which will be discarded by PactVerificationService, because it is using a session without a delegate, so it is not handling didReceiveAuthenticationChallenge.
To reproduce the issue,

  • change the start script to use --ssl option
  • change the base URL of the mockService to "https://localhost"
  • run the tests.

Possible solution will be to expose the authentication challenge or just a simple bool on the PactVerificationService to allow all insecure certificates.

PactVerificationService


var allowInsecureCertificates: Bool = false


public func urlSession(_ session: URLSession,
                           didReceive challenge: URLAuthenticationChallenge,
                           completionHandler: @escaping (URLSession.AuthChallengeDisposition, URLCredential?) -> Void) {

        guard allowInsecureCertificates,
             challenge.protectionSpace.authenticationMethod == NSURLAuthenticationMethodServerTrust,
            let serverTrust = challenge.protectionSpace.serverTrust else {
                completionHandler(.performDefaultHandling, nil)
        }

        let proposedCredential = URLCredential(trust: serverTrust)
        completionHandler(URLSession.AuthChallengeDisposition.useCredential, proposedCredential)
    }
andrewspinks commented

@georgescumihai Thanks for reporting! I suspect we should have an option on the MockService to use ssl (defaulting to false, and could also switch the url the PactVerificationService is using). I don't have a lot of spare time at the moment, so it might take a while to get around to making the change. PR's welcome :-D

mefellows commented

Honestly I'm not sure why we support SSL for this at all (saying this even though I've added it to Pact JS and others). Can you elaborate on your use case? Might just help us understand if it's something we want to keep and continue supporting.

I've never had the need, and can't see how it helps increase confidence in an integration.

andrewspinks commented

haha @mefellows, I was wondering the same and looked at pactjs for guidance :-)

mefellows commented

Rookie mistake, never look at JS for guidance on anything 😬

georgescumihai commented

In our use case, we are computing a hash out of the url, which is included in the http request, so not having https, will make the hash not match with the one computed by the server, and all the api calls will fail.
The server is only working on https at this moment.

Maybe it is not the most common use case, in an ideal case, it should also work on http, so this would not be necessary.

mefellows commented

So can you not a) have the server configurable (i.e. allow an http URL) or b) not include the scheme in the hashing?

georgescumihai commented

a) at this point not
b) is totally not doable, not only 1 client, other platforms as well, a breaking change

If you do not intend to have this functionality, I can close the PR and we can close this issue.
We still have our fork for this change and will probably work for now.

mefellows commented

Fair enough. I guess when you have a large number of people/companies/use-cases, all roads lead to more features :)

I'm satisfied.

surpher commented

Was this issue resolved with the already merged PR #74?

surpher commented

It was, but was overwritten with the next PR 🤕
It is re-introduced with PR #82 and #84

surpher commented

@georgescumihai @andrewspinks I believe this issue can now finally be closed having expanded implementation and tests merged into master.

georgescumihai commented

Yes, can be closed.