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

Support for SPKI #3269

Open
1 task done
scaio opened this issue Aug 4, 2020 · 6 comments
Open
1 task done

Support for SPKI #3269

scaio opened this issue Aug 4, 2020 · 6 comments

Comments

@scaio
Copy link

scaio commented Aug 4, 2020

Sorry for opening a separate issue for this. I'm trying to draw attention to #2678 which has been closed but seems to have no clear resolution. The PR for it was never merged (#2680) but the issue seemed to be going in a favorable direction for adding support.

If there is no interest in adding SPKI support to alamofire perhaps we could close the PR and state so in the original issue so that future users will know not adding it was a deliberate decision.

Should the maintainers believe SKPI support has value, could we perhaps reopen the original issue until the PR is merged?

Best,

@jshier
Copy link
Contributor

jshier commented Aug 22, 2020

We've discussed adding SPKI support in the past. However, due to the complexity of the feature we decided not to merge the existing PR. Instead, we feel that the feature needs to be built by the Alamofire core team itself, both to develop the expertise necessary to implement and maintain the feature long term, as well as to mitigate the risk of the feature by developing it within the foundation. So while we think the feature would be a useful addition to Alamofire, its complexity, risk, and lower community value have prioritized it lower than our other work on the library.

I'll keep this issue open to track the feature. If anyone has important services or use cases that would benefit from this feature, please let us know.

@jshier jshier mentioned this issue Aug 22, 2020
@scaio
Copy link
Author

scaio commented Aug 23, 2020

@jshier,

Thanks for following up on this and for keeping this open for visibility. I'll update the issue description with data from #2678 so anyone jumping in can understand what the feature is about.

It's a shame the original pull request did not make it. I understand the reservations of the maintainers even if I disagree (I think with a few changes the PR could probably reach a testable enough state to be included as an experimental feature.)

Since the current decision is that this is something only the maintainers can do, could you help me understand how the team currently prioritizes feature requests? What should the community do to indicate they see value in this feature?

Best,

@jshier
Copy link
Contributor

jshier commented Aug 23, 2020

To be clear, there wasn't necessarily anything wrong with the original PR. It's just that, given the risks involved, we decided we should build the feature ourselves. We can't really ship experimental security features in a library as widely used as Alamofire. If anyone really wants the functionality provided by the PR, ServerTrustEvaluating is designed to be extensible, so the functionality could live in a separate library as a separate project and it would work just fine (without the custom error integration).

As to feature prioritization, there is no formal process, but there are a few considerations that must be balanced.

  1. How much time do we have available? There are only two of us that work on the project and we already have full time jobs and personal lives, so our availability varies quite a bit. When it comes down to it, features that can be implemented in the time we have available are more likely to be worked on than features that may take weeks or months of free time.
  2. How complex is the feature? Is it easy to implement and test, or will it require a significant refactor? We have a few in flight PRs that add the ability to be notified when a Request creates a URLRequest or URLSessionTask. These were rather easy to build and test, so prioritizing them was pretty easy.
  3. Does it unlock a new capability for the library? DataStreamRequest is a good example here. We'd removed the ability to stream a DataRequest in Alamofire 5, so I thought it was important to bring the functionality back despite the rather complex and time consuming (it took two months to design and implement, and still had underlying bugs) nature of the feature.
  4. Does it need to live in the library? Again, DataStreamRequest is a good example here. We don't yet have the ability for users to create custom Request types, so if we wanted the functionality it had to live in Alamofire itself, not an external library.
  5. How important or popular is the feature in the community? Is it something that people need to do all the time, or is it something that's only done rarely? A good example here is the recently added AuthenticationInterceptor. It helps users integrate with common authentication flows in a way that helps reduce bugs and eliminate boilerplate from many codebases. Despite its higher difficultly we thought it was important to include due to the common need and how difficult it can be for users to implement themselves.
  6. How risky is the feature for the library? This concern largely applies to features with a security impact but also applies to features that can be easily used improperly. For a library as popular as Alamofire, bugs can hit thousands of apps. Security bugs that have that sort of impact must be avoided.

Every choice to work on a feature is done balancing those concerns. For this SPKI functionality, the time it would take to become comfortable with the standard and implement a solution, the complexity of the solution and testing, the ability for users to build it outside of Alamofire, it's relative unpopularity (it's not even an accepted IETF standard), and riskiness of the feature are all considered in making it a lower priority feature.

@rohitdhawan
Copy link

rohitdhawan commented Jun 19, 2023

Hi @jshier did you get any solution for SSL pinning without certificate or certificate data? I want to do SSL pinning using alamofire but by using jus public key.

@CaioSym
Copy link

CaioSym commented Jun 19, 2023

@rohitdhawan As someone who previously asked for this feature, here are some points:

Certificate Pinning has recently fallen out of favour quite drastically amongst Info Sec specialists. I would advise reviewing the following article prior to doing it: https://www.digicert.com/blog/certificate-pinning-what-is-certificate-pinning

Should you choose to pin anyways then I would advise the following alternatives to accomplish SPKI pinning:

  1. Leverage NSAppTransportSecurity to configure globally for your app. https://developer.apple.com/news/?id=g9ejcf8y has a great tutorial on this. I would advise this if you are building an app as it means even if users leverage URLSession or the default AF session pinning would still apply.
  2. Use TrustKit to pin the keys and wrap it in an ServerTrustEvaluating implementation such as below, which you can then use just like any other evaluator. I would advise this if you are building a framework as you won't be in control of the Info.plist and so would not be able to do the above.
struct TrustKitServerTrustEvaluator: ServerTrustEvaluating {
    let trustKit: TrustKit

    func evaluate(_ trust: SecTrust, forHost host: String) throws {
        let result = trustKit.pinningValidator.evaluateTrust(trust, forHostname: host)
        switch result {
        case .shouldBlockConnection:
            throw AFError.serverTrustEvaluationFailed(reason: .publicKeyPinningFailed(host: host, trust: trust, pinnedKeys: [], serverKeys: []))
        case .domainNotPinned, .shouldAllowConnection:
            return
        @unknown default:
            return
        }
    }
}

Best!

@rohitdhawan
Copy link

@CaioSym a Big Thank you for your reply :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants