Skip to content

Conversation

@simonrozsival
Copy link
Member

@simonrozsival simonrozsival commented Jan 3, 2023

Fixes #7650

We check if the server certificate hostname matches the request's RequestUri. When the request is redirected to a different host, the verification failed because we tested the next server's hostname against the original request URI. This PR keeps the RequestUri in sync with the redirect status to avoid this problem.

@simonrozsival simonrozsival force-pushed the fix-servercertificatevalidationcallback-after-redirect branch from b5cbd72 to 66e1076 Compare January 4, 2023 13:10
@simonrozsival simonrozsival force-pushed the fix-servercertificatevalidationcallback-after-redirect branch from 66e1076 to c126fae Compare January 4, 2023 13:11
@simonrozsival simonrozsival marked this pull request as ready for review January 5, 2023 09:44
var client = new HttpClient (handler);
var result = await client.GetAsync ("https://httpbin.org/redirect-to?url=https://www.microsoft.com/");

Assert.AreEqual (2, callbackCounter);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simonrozsival: why is this 2? Are there any docs around how many times ServerCertificateCustomValidationCallback will be invoked, other than "at least once"? Why is the callback being called twice? Is that a bug? Is that likely to change at any point in the future?

Or should it be 2: once for httpbin.org/redirect-to, and once again for www.microsoft.com, and it's called "once per redirect", i.e. if you "nested" redirects with https://httpbin.org/redirect-to?url=https://httpbin.org/redirect-to?url=…, callbackCounter should be the number of times we hit httpbin.org/redirect-to + 1?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The callback is called once for every sub-request that's made (the first request + all redirects). In this case, it's called once for httpbin.org/... and once for www.microsoft.com. So callbackCounter should equal to 1 + number of redirects. The SocketsHttpHandler behaves the same way.

@jonpryor
Copy link
Contributor

jonpryor commented Jan 9, 2023

Draft commit message:

[Mono.Android] ServerCertificateValidationCallback() and redirects (#7662)

Fixes: https://github.com/xamarin/xamarin-android/issues/7650

`AndroidMessageHandler` was incorrectly validating SSL certificate
after a redirect to another domain if and only if
`AndroidMessageHandler.ServerCertificateCustomValidationCallback`
was set; consider:

	const string url = "https://httpbin.org/redirect-to?url=https://www.microsoft.com/";
	using var handler = new AndroidMessageHandler() {
	    ServerCertificateCustomValidationCallback = (message, certificate, chain, errors) => errors == SslPolicyErrors.None
	};
	using var client = new HttpClient(handler);
	await client.GetAsync(url);

The expectation is that
`AndroidMessageHandler.ServerCertificateCustomValidationCallback`
will be called *twice*:

 1. Once with `message.RequestUri` set to
    `https://httpbin.org/redirect-to?…`, and
 2. Once again with `message.RequestUri` set to
    `https://www.microsoft.com`.

The problem was that in actuality the 2nd invocation was unchanged
from (1), with `message.RequestUri` set to
`https://httpbin.org/redirect-to…?…`, and `errors` set to
`SslPolicyErrors.RemoteCertificateNameMismatch` (!).

The problem was that `AndroidMessageHandler.DoSendAsync()` didn't
update `HttpRequestMessage.RequestUri` to the redirected URI, causing
the `errors` enum value to contain an error, and the second
`ServerCertificateCustomValidationCallback` invocation to have the
wrong information.

Update `AndroidMessageHandler.DoSendAsync()` so that when dealing
with an HTTP redirect, `message.RequestUri` always contains the
"current" value in the redirect chain.  This keeps `RequestUri` in
sync with the redirect status to avoid this problem.

@jonpryor jonpryor merged commit d78d786 into dotnet:main Jan 9, 2023
grendello added a commit to grendello/xamarin-android that referenced this pull request Jan 17, 2023
* main:
  [Xamarin.Android.Build.Tasks] skip XA1034 logic in some cases (dotnet#7680)
  [ci] Move OneLocBuild task to scheduled pipeline (dotnet#7679)
  [Mono.Android] ServerCertificateValidationCallback() and redirects (dotnet#7662)
  Bump to xamarin/Java.Interop/main@cf80deb7 (dotnet#7664)
  Localized file check-in by OneLocBuild (dotnet#7668)
  [api-merge] Correctly compute //method/@deprecated-since (dotnet#7645)
  [Xamarin.Android.Build.Tasks] _Microsoft.Android.Resource.Designer (dotnet#6427)
  [Xamarin.Android.Build.Tasks] downgrade d8/r8 `warning` messages to `info` (dotnet#7643)
  [Xamarin.Android.Build.Tasks] fix cases of missing `@(Reference)` (dotnet#7642)
  [Xamarin.Android.Build.Tasks] delay ToJniName calls in ManifestDocument (dotnet#7653)
  [Xamarin.Android.Build.Tasks] fast path for `<CheckClientHandlerType/>` (dotnet#7652)
@github-actions github-actions bot locked and limited conversation to collaborators Jan 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AndroidMessageHandler - RemoteCertificateNameMismatch after redirect

3 participants