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.

Sign up for GitHub

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

Allow GitHub OAuth params to be overridden at runtime #103

Merged
merged 1 commit into from
Apr 28, 2020
Merged

Allow GitHub OAuth params to be overridden at runtime #103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/shared/GitHub/GitHubAuthentication.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ public async Task<OAuth2TokenResult> GetOAuthTokenAsync(Uri targetUri, IEnumerab
{
ThrowIfUserInteractionDisabled();

var oauthClient = new GitHubOAuth2Client(HttpClient, targetUri);
var oauthClient = new GitHubOAuth2Client(HttpClient, Context.Settings, targetUri);

// If we have a desktop session try authentication using the user's default web browser
if (Context.IsDesktopSession)
Expand Down
13 changes: 13 additions & 0 deletions src/shared/GitHub/GitHubConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,13 @@ public static class GitHubConstants

public const string AuthHelperName = "GitHub.UI";

public const string OAuthClientId = "0120e057bd645470c1ed";
public const string OAuthClientSecret = "18867509d956965542b521a529a79bb883344c90";
public static readonly Uri OAuthRedirectUri = new Uri("http://localhost/");
public static readonly Uri OAuthAuthorizationEndpointRelativeUri = new Uri("/login/oauth/authorize", UriKind.Relative);
public static readonly Uri OAuthTokenEndpointRelativeUri = new Uri("/login/oauth/access_token", UriKind.Relative);
public static readonly Uri OAuthDeviceEndpointRelativeUri = new Uri("/login/oauth/authorize/device", UriKind.Relative);

/// <summary>
/// The GitHub required HTTP accepts header value
/// </summary>
Expand Down Expand Up @@ -51,13 +58,19 @@ public static class OAuthScopes
public static class EnvironmentVariables
{
public const string AuthenticationModes = "GCM_GITHUB_AUTHMODES";
public const string DevOAuthClientId = "GCM_DEV_GITHUB_CLIENTID";
public const string DevOAuthClientSecret = "GCM_DEV_GITHUB_CLIENTSECRET";
public const string DevOAuthRedirectUri = "GCM_DEV_GITHUB_REDIRECTURI";
}

public static class GitConfiguration
{
public static class Credential
{
public const string AuthModes = "gitHubAuthModes";
public const string DevOAuthClientId = "gitHubDevClientId";
public const string DevOAuthClientSecret = "gitHubDevClientSecret";
public const string DevOAuthRedirectUri = "gitHubDevRedirectUri";
}
}
}
Expand Down
60 changes: 51 additions & 9 deletions src/shared/GitHub/GitHubOAuth2Client.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,75 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license.
using System;
using System.Net.Http;
using Microsoft.Git.CredentialManager;
using Microsoft.Git.CredentialManager.Authentication.OAuth;

namespace GitHub
{
public class GitHubOAuth2Client : OAuth2Client
{
private static readonly string ClientId = "0120e057bd645470c1ed";
private static readonly string ClientSecret = "18867509d956965542b521a529a79bb883344c90";
private static readonly Uri RedirectUri = new Uri("http://localhost/");

public GitHubOAuth2Client(HttpClient httpClient, Uri baseUri)
: base(httpClient, CreateEndpoints(baseUri), ClientId, RedirectUri, ClientSecret) { }
public GitHubOAuth2Client(HttpClient httpClient, ISettings settings, Uri baseUri)
: base(httpClient, CreateEndpoints(baseUri),
GetClientId(settings), GetRedirectUri(settings), GetClientSecret(settings)) { }

private static OAuth2ServerEndpoints CreateEndpoints(Uri baseUri)
{
Uri authEndpoint = new Uri(baseUri, "/login/oauth/authorize");
Uri tokenEndpoint = new Uri(baseUri, "/login/oauth/access_token");
Uri authEndpoint = new Uri(baseUri, GitHubConstants.OAuthAuthorizationEndpointRelativeUri);
Uri tokenEndpoint = new Uri(baseUri, GitHubConstants.OAuthTokenEndpointRelativeUri);

Uri deviceAuthEndpoint = null;
if (GitHubConstants.IsOAuthDeviceAuthSupported)
{
deviceAuthEndpoint = new Uri(baseUri, "/login/oauth/authorize/device");
deviceAuthEndpoint = new Uri(baseUri, GitHubConstants.OAuthDeviceEndpointRelativeUri);
}

return new OAuth2ServerEndpoints(authEndpoint, tokenEndpoint)
{
DeviceAuthorizationEndpoint = deviceAuthEndpoint
};
}

private static string GetClientId(ISettings settings)
{
// Check for developer override value
if (settings.TryGetSetting(
GitHubConstants.EnvironmentVariables.DevOAuthClientId,
Constants.GitConfiguration.Credential.SectionName, GitHubConstants.GitConfiguration.Credential.DevOAuthClientId,
out string clientId))
{
return clientId;
}

return GitHubConstants.OAuthClientId;
}

private static Uri GetRedirectUri(ISettings settings)
Copy link

Choose a reason for hiding this comment

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

This would let me route the redirect to something besides localhost. I can't express
whether this bothers me or whether I'm just being paranoid. I tried to walk thru the
MITM diagrams from the other day, but I'm in over my head I fear. It may not be a
problem, but I don't know that.

I'm wondering what you would use this env var for in testing -- if as I understand it,
the responses need to come back into the builtin GCM server to advance the state
machine. It seems like this would route the redirect somewhere else and you wouldn't
get responses back in the GCM. But again, I don't completely understand the flow.

Alternatively, would it be safer to have this env var be under a #if DEBUG ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Redirect URIs in OAuth2 are tied to the client ID. That is to say each registered application must define at least one redirect URI, and during the various endpoint calls the client must send a redirect URI that is associated with that app.

For example GCM just uses http://localhost/ as the only redirect URI (loopback hosts are special cased in that the port is lazily matched; otherwise port must === too).

Other applications such as GitHub CLI use http://127.0.0.1/callback as the redirect URI (it includes the path /callback).

Also note that 127.0.0.1 != localhost textually, so these are considered different redirect URIs.

This would let me route the redirect to something besides localhost.

In theory yes, in practice no since the OAuth2SystemWebBrowser component that we use in GCM (the component that launches the browser, and starts the HTTP listener for the redirect) mandates that the redirect URI Uri::IsLoopback here and here.

So we won't end up gifting http://evil-server.com with a authorisation code because:

  1. We won't even start the auth flow via the browser unless we have a local redirect URI,
  2. The server would need to agree to send the code to the redirect URI.

The 'problem' that might occur however is redirecting to another localhost application listening on with a different path prefix. This is 'fixed' with PKCE as mentioned and implemented by GCM previously, but that requires server support.

{
// Check for developer override value
if (settings.TryGetSetting(
GitHubConstants.EnvironmentVariables.DevOAuthRedirectUri,
Constants.GitConfiguration.Credential.SectionName, GitHubConstants.GitConfiguration.Credential.DevOAuthRedirectUri,
out string redirectUriStr) && Uri.TryCreate(redirectUriStr, UriKind.Absolute, out Uri redirectUri))
{
return redirectUri;
}

return GitHubConstants.OAuthRedirectUri;
}

private static string GetClientSecret(ISettings settings)
{
// Check for developer override value
if (settings.TryGetSetting(
GitHubConstants.EnvironmentVariables.DevOAuthClientSecret,
Constants.GitConfiguration.Credential.SectionName, GitHubConstants.GitConfiguration.Credential.DevOAuthClientSecret,
out string clientSecret))
{
return clientSecret;
}

return GitHubConstants.OAuthClientSecret;
}
}
}