Issues in the VS extension's proxy code?

Topics: General
Jul 3, 2013 at 8:48 PM
Hi folks. I've been working on the implementation of the nuget based extensions for ReSharper, and I'm just looking at adding proxy support. I've got it working (from limited testing with Fiddler), but I'm a bit confused by the code that's in NuGet.VisualStudio. Can someone check it over and confirm it's ok?

Specifically, the VisualStudioCredentialProvider class has a GetCredentials method that I have a few issues with. The class is trying to get an ICredentials by calling Visual Studio's IVsWebProxy. But IVsWebProxy doesn't return credentials, instead, it prepares the proxy in the WebRequest.DefaultWebProxy static property with the credentials for the passed in URL, potentially prompting the user.

My issues are this:
  1. At the end of the method, the DefaultWebProxy property is reset to the value of the originalProxy variable, but this isn't the original value of that property. Is this a bug?
  2. There is some logic to try to figure out which proxy to use - the passed in proxy, or a new proxy derived from the url of the proxy that would be used to download the feed url (phew!). Why is this done? Is this choosing between requesting credentials for the site and the proxy?
  3. The logic to figure out which proxy to use is ignored - the VSRequestCredentialProvider class simply sets the WebRequest.DefaultWebProxy to a new proxy where the proxy url is the feed url. Is this a bug? It seems to be saying that we're requesting credentials for the feed url, not the proxy.
I've got my version working by simplifying massively. If there isn't a passed in proxy, or the url is bypassed, I create a new proxy with the feed url, so essentially I'm requesting credentials for the feed. Otherwise, I simply use the passed in proxy, which means I'm requesting credentials to talk to the proxy. I'm also setting this proxy on the WebRequest, and correctly saving and restoring it after calling VS. So it looks something like:
    public ICredentials GetCredentials(Uri uri, IWebProxy proxy, bool allowPrompt)
    {
      var originalProxy = ReplaceDefaultWebProxy(uri, proxy);
      try
      {
        // Calls IVsWebProxy and returns the credentials from WebRequest.DefaultWebProxy
        return GetCredentialsFromDefaultWebProxy(uri, allowPrompt);
      }
      finally
      {
        // WebRequest.DefaultWebProxy = originalProxy;
        RestoreOriginalProxy(originalProxy);
      }
    }

    private IWebProxy ReplaceDefaultWebProxy(Uri url, IWebProxy proxy)
    {
      var originalProxy = WebRequest.DefaultWebProxy;

      // Get credentials for the passed in proxy server. If the url is bypassed, get
      // credentials for the url
      var newProxy = proxy;
      if (proxy == null || proxy.IsBypassed(url))
      {
        newProxy = new WebProxy(url);
      }

      WebRequest.DefaultWebProxy = newProxy;

      return originalProxy;
    }
While this works with testing against Fiddler requiring authentication, have I got things wrong? Should my code be more like NuGet.VisualStudios? But are there bugs in the VS code?

Thanks
Matt
Aug 9, 2013 at 1:20 PM
Actually, my implementation evolved past this.

There are two other issues - I couldn't properly use the Chain of Command pattern with ICredentialProvider, and the VS web proxy is called incorrectly, meaning you always get prompted, and never use cached credentials.

The Chain of Command issue is that the ICredentialProvider is a singleton, so can't maintain state, and only has a boolean "retrying" parameter to work with. If you want to have multiple means of getting at credentials, that boolean really starts to hurt. For example, I use NuGet's SettingsCredentialProvider to allow credentials to be stored in package source settings, then a custom provider to look at ReSharper's own proxy settings, and then fall back to the VS web proxy service. But if the SettingsCredentialProvider tries and fails, the inner provider(s) get called with the "retrying" parameter set to true, and they have no way of knowing if they've had an attempt and failed, or if the "parent" provider has failed.

Fortunately, I'm mostly ok with this - SettingsCredentialProvider only retries if a feed url fails, and my ReSharper provider only fails if a proxy url fails, so they're mutually exclusive. However, it does mean that the VS web proxy service can be called with "retrying" set to true, and so only gets one chance instead of two.

This could be fixed by not having the providers as singletons, or by replacing the "retrying" boolean with a retry count. If the count is greater than zero, you know it's your number of retries, i.e. if you get a retry count of 0, try your first method. If you get 1, try your second, 2, third, and so on. If you run out of methods, subtract the numbr of methods and pass to the inner provider. If the inner provider hasn't tried anything, the count will be zero, otherwise it'll be the number of retries for that provider (hope this makes sense). Either of these would be breaking interface changes - HttpClient.DefaultCredentialProvider would have to be a factory (ideally, it would also be an instance property, so multiple hosts can have proxy providers - e.g. VS and ReSharper) or ICredentialProvider would have a count instead of a bool.

The second issue is that the IVsWebProxy isn't being called correctly, and the chain of command issue exacerbates this.

The idea is to repeatedly call IVsWebProxy.PrepareWebProxy while making web requests. Each time, you pass in the state of the previous call. You start with a state of NoCredentials and VS will set up the proxy with default credentials (proxy.Credentials = CredentialCache.DefaultCredentials), and return a state of DefaultCredentials. If this fails, you call again, passing in that returned DefaultCredentials state. VS then looks for cached credentials, using P/Invoke to talk to CredRead, etc. If it finds some, it applies them to the proxy and returns CachedCredentials. If it doesn't find any cached credentials, or if they fail and you call again, it will prompt the user for credentials (using CredPromptUI), and either return a state of PromptForCredentials or Abort. If you call it with PromptForCrednetials again, it will repeatedly prompt the user.

(For future reference, IVsWebProxy is implemented by VsWebProxyService and VsWebProxy in Microsoft.VisualStudio.CommonIDE.dll from the GAC)

The NuGet code calls it once with PromptForCredentials as the initial state, so it will always prompt the user, and never look for cached credentials. Also, it falls foul of the "retrying" parameter. The method needs to be called repeatedly, but with the boolean parameter, you can only call it twice - and if SettingsCredentialProvider has failed, you can only call it once. Assuming you do get to call it twice, you can call it initially with DefaultCredentials to get cached credentials, and subsequently PromptForCredentials (but you'll only have one chance for prompting). Default credentials are added by default by NuGet's RequestHelper.

I worked around this by implementing my VS web proxy ICredentialsProvider with a weak reference dictionary - I map the Uri to the current state, start with NoCredentials and allow it to retry until it gets to Abort. Having ICredentialsProvider not be a singleton would be great here. I could maintain state for the current web request and handle things a whole lot cleaner.

I'd like to see NuGet handle this cleaner - and use the VS web proxy service correctly. I'd vote for changing providers from being singleton to being per-request. I'm happy to work on a PR for this, but would like to get an agreement on approach before I work on anything.
Sep 30, 2013 at 3:32 PM
Anyone interested in this? I know it's a huge block of text, but it's an issue I'm happy to send a PR for, but I need some guidance before I wade in...
Developer
Sep 30, 2013 at 3:39 PM
Would love to see this one in, a lot of people seem to have issues with secured feeds (on ProGet, MyGet and TeamCity)
Dec 9, 2013 at 7:03 PM
We would very much appreciate a pull request for this! We are getting ready to cut the 2.8 release branch to work on the final stabilization of that release. We would accept this into master after we create the branch, to queue this up for the 2.9 release in the spring. We would invite people to run daily builds after this PR comes in to help crowdsource the testing of this since the proxies available to us for testing are limited.

Thank you!
Dec 9, 2013 at 7:04 PM
Note by the way that we would need to have an issue with a clearly stated bug with repro steps, so that the pull request could be associated with the issue for testing.

Thanks again,
Jeff
Apr 28, 2014 at 8:49 AM
A pull request for a fix on VS credential issues (issue #4075) that I have just submitted may be of interest for this discussion. Indeed there were some code paths where HttpClient.DefaultCredentialProvider was not initialized (in restore on build for example), and thus for secured feeds VS would prompt for credentials instead of using the ones stored in the config file:
https://nuget.codeplex.com/SourceControl/network/forks/jmdressler/nuget/contribution/6708