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

Extending NSUrlSessionHandler with client certificates #13856

Closed
MitchellFreeman opened this issue Jan 23, 2022 · 13 comments
Closed

Extending NSUrlSessionHandler with client certificates #13856

MitchellFreeman opened this issue Jan 23, 2022 · 13 comments
Assignees
Labels
enhancement The issue or pull request is an enhancement
Milestone

Comments

@MitchellFreeman
Copy link

I'm working on a Xamarin iOS project that requires the use of mutual TLS. I tried multiple different ways to get this working, but most used the HttpClientHandler and I couldn't get any of them to work.
This was when I found the DidReceiveChallenge method of NSUrlSessionHandler. I made my own copy of this class and added

if (credentials != null && challenge.ProtectionSpace.AuthenticationMethod == NSUrlProtectionSpace.AuthenticationMethodClientCertificate)
{
    completionHandler(NSUrlSessionAuthChallengeDisposition.UseCredential, credentials);
}

to this method along with some code to store the client certificate. This works, but I'm wondering why this this simple change isn't already supported in some way.

Expected Behavior

The NSUrlSessionHandler class supports client certificates

Actual Behavior

The DidReceiveChallenge method of its delegate needs a small modification so that it supports client certificates

@mandel-macaque
Copy link
Member

mandel-macaque commented Jan 24, 2022

Hello,

There is indeed a way for you to accept client certificates in the NSUrlSessionHandler without the need to copy the class. As you can see in the class we provided two delegates:

	public delegate bool NSUrlSessionHandlerTrustOverrideCallback (NSUrlSessionHandler sender, SecTrust trust);
	public delegate bool NSUrlSessionHandlerTrustOverrideForUrlCallback (NSUrlSessionHandler sender, string url, SecTrust trust);

Which you can set via the properties:

public NSUrlSessionHandlerTrustOverrideCallback TrustOverride;
public NSUrlSessionHandlerTrustOverrideForUrlCallback TrustOverrideForUrl;

The delegate NSUrlSessionHandlerTrustOverrideCallback has been obsoleted and you should use NSUrlSessionHandlerTrustOverrideForUrlCallback since it allows to filter per url.

You can see in the code that those delegates are executed as part of the handler management of the certs: https://github.com/xamarin/xamarin-macios/blob/main/src/Foundation/NSUrlSessionHandler.cs#L823

I am closing the issue but feel free to re-open if needed.

@mandel-macaque mandel-macaque added the support The issue is related to support label Jan 24, 2022
@MitchellFreeman MitchellFreeman changed the title Is there any reason why NSUrlSessionHandler does not support client certificates? Extending NSUrlSessionHandler with client certificates Jan 24, 2022
@MitchellFreeman
Copy link
Author

MitchellFreeman commented Jan 24, 2022

@mandel-macaque While I understand that this can be used to evaluate self signed certificates, I don't know how this could be used to add client certificates.
I see that there is a credential created here but I don't see how this this is derived in a way from SecTrust that allows the addition of a client certificate.

@mandel-macaque
Copy link
Member

@MitchellFreeman sorry, I misunderstood the question. Can you install the client certificate in the device? Or is this an issue. We might need to add an extra callback for this :/

@mandel-macaque mandel-macaque added the enhancement The issue or pull request is an enhancement label Jan 24, 2022
@mandel-macaque mandel-macaque added need-info Waiting for more information before the bug can be investigated and removed support The issue is related to support labels Jan 24, 2022
@MitchellFreeman
Copy link
Author

@mandel-macaque Sorry that may be partially my fault. Based on how iOS installs certificates I don't believe this is something I will be able to do. But thanks for pointing me towards those existing callbacks as validating self signed certificates is something I also need to do.

@mandel-macaque
Copy link
Member

@MitchellFreeman ok, so since you have no way to install the client certificate we have to find a way to do so, I'll do some research on who is the secure way to do this, since we are bypassing the OS and we don't want to introduce an attack vector. Does that sound like a plan? Ideally I'll get back with a objc sample and will do the appropriate in c#

@MitchellFreeman
Copy link
Author

MitchellFreeman commented Jan 25, 2022

@mandel-macaque Sounds good. Although I don't see how this is bypassing the OS as we are simply replying to NSUrlProtectionSpace.AuthenticationMethodClientCertificate with our client certificate.

@mandel-macaque
Copy link
Member

Yes, I meant to be able not to install them and pass them to the OS, I have another issue in which we want to add some extra properties for the handler, this probably will be included there. I might merge both issues: #13579

@mandel-macaque mandel-macaque added this to the Future milestone Jan 31, 2022
@chamons chamons removed the need-info Waiting for more information before the bug can be investigated label Apr 20, 2022
@mandel-macaque mandel-macaque self-assigned this Jun 15, 2023
@dotMorten
Copy link
Contributor

I found this:

if (challenge.ProtectionSpace.AuthenticationMethod == NSUrlProtectionSpace.AuthenticationMethodServerTrust) {

I think what is missing is also a check for
if (challenge.ProtectionSpace.AuthenticationMethod == NSUrlProtectionSpace.AuthenticationMethodClientCertificate)

@dotMorten
Copy link
Contributor

dotMorten commented Mar 16, 2024

Did a bit more hacking. At this line I inserted the following code. That is of course just for testing, but proves it can be done:

if (sessionHandler.ClientCertificates is not null && sessionHandler.ClientCertificates.Count > 0 &&
    challenge.ProtectionSpace.AuthenticationMethod == NSUrlProtectionSpace.AuthenticationMethodClientCertificate)
{
    var cert = new SecCertificate(sessionHandler.ClientCertificates[0]);
    var identity = SecIdentity.Import(sessionHandler.ClientCertificates[0] as X509Certificate2);
    var trust = new SecTrust(sessionHandler.ClientCertificates, SecPolicy.CreateBasicX509Policy());
    var credential = new NSUrlCredential(identity, new SecCertificate[] { cert }, NSUrlCredentialPersistence.ForSession);
    completionHandler(NSUrlSessionAuthChallengeDisposition.UseCredential, credential);
    return;
}

This (wrongly) assumes there's only one certificate and it has a private key available, but the request succeeded with this code added, so it is definitely on the right track. Testcode:

X509Certificate2 certificate = await LoadCertificateFromFile();
var handler = new NSUrlSessionHandler();
handler.ClientCertificates = new X509Certificate2Collection(certificate); //Note: Also made handler.ClientCertificates settable to match other APIs
using var client = new HttpClient(handler);
var response = await client.GetAsync(infoUri);
response = response.EnsureSuccessStatusCode();

This is with file-based certificates - I haven't tried using certificates that are installed on the device.

One simple solution that we could use for now, is to provide a public callback similar to the server validation callback, and requiring you to return a NSUrlCredential, and let the user implement the certificate selection. Then at a later stage add support for just picking certificates from the certificate collection is the callback wasn't used.

@dotMorten
Copy link
Contributor

dotMorten commented Mar 16, 2024

Here's that alternative solution with a callback.
I inserted the code below here:

else if (challenge.ProtectionSpace.AuthenticationMethod == NSUrlProtectionSpace.AuthenticationMethodClientCertificate)
{
    if(sessionHandler.TryInvokeClientCertificateChallengeCallback(inflight.Request, challenge, out NSUrlCredential? credential))
    {
        completionHandler(NSUrlSessionAuthChallengeDisposition.UseCredential, credential);
        return;
     }
}

And added this to NSUrlSessionHandler:

    public Func<HttpRequestMessage, NSUrlAuthenticationChallenge, NSUrlCredential?>? ClientCertificateChallengeCallback
    {
        get; set;
    }

     private bool TryInvokeClientCertificateChallengeCallback(HttpRequestMessage request, NSUrlAuthenticationChallenge challenge, [NotNullWhen(true)] out NSUrlCredential? credential)
     {			
        var callback = ClientCertificateChallengeCallback;
        credential = null;
        if (callback is null)
            return false;
        credential = callback(request, challenge);
        return credential != null;
     }

@dotMorten
Copy link
Contributor

@MitchellFreeman I put up a PR that adds certificate support linked above

rolfbjarne added a commit that referenced this issue Sep 24, 2024
…icates (#21284)

Addresses #13856

This was originally created by @dotMorten in #20434.

Also make SecIdentity.Import use an in-memory keychain on macOS 15+, so that
SecIdentity.Import works like all othe other platforms (i.e. not requiring
access to the default keychain, which, among other things, is not ideal on
bots).

---------

Co-authored-by: Morten Nielsen <[email protected]>
Co-authored-by: dotMorten <[email protected]>
Co-authored-by: Manuel de la Pena <[email protected]>
@rolfbjarne
Copy link
Member

Fixed in #21284.

@dotMorten
Copy link
Contributor

Just wanted to confirm that things works beautifully in RC2 ! Thank you all for helping this getting in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue or pull request is an enhancement
Projects
None yet
Development

No branches or pull requests

5 participants