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

Jump to bottom

Enable local dev OIDC #6529

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Enable local dev OIDC #6529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0f6ffed
Enables OIDC to be configured in local dev scenarios
los93sol Oct 23, 2024
c0c335f
-Drop dependency on Microsoft.AspNetCore.Authentication.OpenIdConnect…
los93sol Oct 24, 2024
dea1492
-Reverts moving FrontendAuthMode to Shared and re-implements as speci…
los93sol Oct 28, 2024
97ba097
-Removes FrontendAuthMode from Shared
los93sol Oct 28, 2024
115bba4
-Reverts moving OpenIdConnectOptions to shared and instead uses a POC…
los93sol Oct 28, 2024
7849572
Merge branch 'dotnet:main' into enable-local-dev-oidc
los93sol Oct 28, 2024
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
55 changes: 48 additions & 7 deletions src/Aspire.Hosting/Dashboard/DashboardLifecycleHook.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,9 @@ private void ConfigureAspireDashboardResource(IResource dashboardResource)
var otlpGrpcEndpointUrl = options.OtlpGrpcEndpointUrl;
var otlpHttpEndpointUrl = options.OtlpHttpEndpointUrl;

var environment = options.AspNetCoreEnvironment;
var dashboardAuthMode = options.DashboardAuthMode;
var browserToken = options.DashboardToken;
var environment = options.AspNetCoreEnvironment;
var otlpApiKey = options.OtlpApiKey;

var resourceServiceUrl = await dashboardEndpointProvider.GetResourceServiceUriAsync(context.CancellationToken).ConfigureAwait(false);
Expand Down Expand Up @@ -214,15 +215,55 @@ private void ConfigureAspireDashboardResource(IResource dashboardResource)
}
}

// Configure frontend browser token
if (!string.IsNullOrEmpty(browserToken))
// Configure frontend auth
context.EnvironmentVariables[DashboardConfigNames.DashboardFrontendAuthModeName.EnvVarName] = dashboardAuthMode.ToString();

if (dashboardAuthMode == DashboardAuthMode.OpenIdConnect)
{
context.EnvironmentVariables[DashboardConfigNames.DashboardFrontendAuthModeName.EnvVarName] = "BrowserToken";
context.EnvironmentVariables[DashboardConfigNames.DashboardFrontendBrowserTokenName.EnvVarName] = browserToken;
if (options.OpenIdConnect != null)
{
context.EnvironmentVariables[DashboardConfigNames.DashboardFrontendOpenIdConnectNameClaimType.EnvVarName] = options.OpenIdConnect.NameClaimType;
context.EnvironmentVariables[DashboardConfigNames.DashboardFrontendOpenIdConnectUsernameClaimType.EnvVarName] = options.OpenIdConnect.UsernameClaimType;
context.EnvironmentVariables[DashboardConfigNames.DashboardFrontendOpenIdConnectRequiredClaimType.EnvVarName] = options.OpenIdConnect.RequiredClaimType;
context.EnvironmentVariables[DashboardConfigNames.DashboardFrontendOpenIdConnectRequiredClaimValue.EnvVarName] = options.OpenIdConnect.RequiredClaimValue;
}

if (options.OpenIdConnectSettings != null)
{
var prefix = "AUTHENTICATION__SCHEMES__OPENIDCONNECT__";
Copy link
Member

Choose a reason for hiding this comment

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

Is this relying on the default options binding in OpenIdConnectConfigureOptions? If so, would it make more sense to forward the entire "Authentication:Schemes:OpenIdConnect" IConfigurationSection via environment variables instead of just what we remember to include in "OpenIdConnectSettings"?

Also, would it make sense to make the scheme name and/or configuration section prefix configurable?

Copy link
Contributor Author

@los93sol los93sol Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Originally it was, but earlier feedback was to switch to POCO to avoid the dependency on it. If there was a way to copy that entire section in then I guess you don't need the reference and could just copy that section in directly, but since there's currently no way to bring the whole section in I think POCO is a reasonable approach for now that doesn't really bind you in any way long term.

Copy link
Member

Choose a reason for hiding this comment

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

@davidfowl Can you explain your earlier feedback? I don't like that it's using the same default configuration section OpenIdConnectConfigureOptions does but only supports a subset of the options supported by that. If the goal is just to remove Microsoft.AspNetCore.Authentication.OpenIdConnect PackageReference, I would just forward the entire "Authentication:Schemes:OpenIdConnect" IConfigurationSection via environment variables.


if (!string.IsNullOrWhiteSpace(options.OpenIdConnectSettings.Authority))
{
context.EnvironmentVariables[$"{prefix}AUTHORITY"] = options.OpenIdConnectSettings.Authority;
}

if (!string.IsNullOrWhiteSpace(options.OpenIdConnectSettings.ClientId))
{
context.EnvironmentVariables[$"{prefix}CLIENTID"] = options.OpenIdConnectSettings.ClientId;
}

if (!string.IsNullOrWhiteSpace(options.OpenIdConnectSettings.ClientSecret))
{
context.EnvironmentVariables[$"{prefix}CLIENTSECRET"] = options.OpenIdConnectSettings.ClientSecret;
}

if (!string.IsNullOrWhiteSpace(options.OpenIdConnectSettings.MetadataAddress))
{
context.EnvironmentVariables[$"{prefix}METADATAADDRESS"] = options.OpenIdConnectSettings.MetadataAddress;
}

var i = 0;
foreach (var scope in options.OpenIdConnectSettings.Scope)
{
context.EnvironmentVariables[$"{prefix}SCOPE__{i}"] = scope;
i++;
}
}
}
else
else if (dashboardAuthMode == DashboardAuthMode.BrowserToken && !string.IsNullOrEmpty(browserToken))
{
context.EnvironmentVariables[DashboardConfigNames.DashboardFrontendAuthModeName.EnvVarName] = "Unsecured";
// Configure frontend browser token
context.EnvironmentVariables[DashboardConfigNames.DashboardFrontendBrowserTokenName.EnvVarName] = browserToken;
}

// Configure resource service API key
Expand Down
54 changes: 50 additions & 4 deletions src/Aspire.Hosting/Dashboard/DashboardOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,23 +11,69 @@ internal class DashboardOptions
{
public string? DashboardPath { get; set; }
public string? DashboardUrl { get; set; }
public DashboardAuthMode DashboardAuthMode { get; set; }
public string? DashboardToken { get; set; }
public OpenIdConnectPolicyOptions? OpenIdConnect { get; set; } = new();
public OpenIdConnectSettings? OpenIdConnectSettings { get; set; } = new();
public string? OtlpGrpcEndpointUrl { get; set; }
public string? OtlpHttpEndpointUrl { get; set; }
public string? OtlpApiKey { get; set; }
public string AspNetCoreEnvironment { get; set; } = "Production";
}

internal enum DashboardAuthMode
{
Unsecured,
OpenIdConnect,
BrowserToken
}

internal sealed class OpenIdConnectPolicyOptions
{
public string NameClaimType { get; set; } = "name";
public string UsernameClaimType { get; set; } = "preferred_username";

/// <summary>
/// Gets the optional name of a claim that users authenticated via OpenID Connect are required to have.
/// If specified, users without this claim will be rejected. If <see cref="RequiredClaimValue"/>
/// is also specified, then the value of this claim must also match <see cref="RequiredClaimValue"/>.
/// </summary>
public string RequiredClaimType { get; set; } = "";

/// <summary>
/// Gets the optional value of the <see cref="RequiredClaimType"/> claim for users authenticated via
/// OpenID Connect. If specified, users not having this value for the corresponding claim type are
/// rejected.
/// </summary>
public string RequiredClaimValue { get; set; } = "";
}

internal class OpenIdConnectSettings
{
public string? Authority { get; set; }
public string? MetadataAddress { get; set; }
public string? ClientId { get; set; }
public string? ClientSecret { get; set; }
public ICollection<string> Scope { get; } = [];
}

internal class ConfigureDefaultDashboardOptions(IConfiguration configuration, IOptions<DcpOptions> dcpOptions) : IConfigureOptions<DashboardOptions>
{
public void Configure(DashboardOptions options)
{
options.DashboardPath = dcpOptions.Value.DashboardPath;
options.DashboardUrl = configuration["ASPNETCORE_URLS"];
options.DashboardToken = configuration["AppHost:BrowserToken"];
options.DashboardUrl = configuration[KnownConfigNames.AspNetCoreUrls];

options.OtlpGrpcEndpointUrl = configuration["DOTNET_DASHBOARD_OTLP_ENDPOINT_URL"];
options.OtlpHttpEndpointUrl = configuration["DOTNET_DASHBOARD_OTLP_HTTP_ENDPOINT_URL"];
if (Enum.TryParse<DashboardAuthMode>(configuration[DashboardConfigNames.DashboardFrontendAuthModeName.ConfigKey], out var dashboardAuthMode))
{
options.DashboardAuthMode = dashboardAuthMode;
}

options.DashboardToken = configuration["AppHost:BrowserToken"];
configuration.Bind("Dashboard:Frontend:OpenIdConnect", options.OpenIdConnect);
configuration.Bind("Authentication:Schemes:OpenIdConnect", options.OpenIdConnectSettings);
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @JamesNK comment on the earlier PR that these two related sections seem a bit too disconnected. Just because this is the section normal web apps use to configure OpenIdConnect doesn't mean that we have to do that for the dashboard, right? Could we include everything under "Dashboard:Frontend:OpenIdConnect"? We could forward it to "Authentication:Schemes:OpenIdConnect" in the orchestrated apps.

Another option might be to add the NameClaimType/UsernameClaimType/RequiredClaimType/RequiredClaimValue logic from aspire to ASP.NET Core's OIDC config binding logic and include everything under "Authentication:Schemes:OpenIdConnect" except for the AuthMode=OpenIdConnect part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it really just the configuration keys you think need to be changed over?

Currently the appsettings config would look something like this...

"Dashboard": {
  "Frontend": {
    "AuthMode": "OpenIdConnect",
    "OpenIdConnect": {
      "RequiredClaimType": "",
      "RequiredClaimValue": ""
    }
  }
},
"Authentication": {
  "Schemes": {
    "OpenIdConnect": {
      "Scope": [ "openid", "profile" ],
      "MetadataAddress": "",
      "ClientId": "",
      "ClientSecret": ""
    }
  }
}

I'm guessing you'd rather see something like....

"Dashboard": {
  "AuthMode": "OpenIdConnect",
  "OpenIdConnect": {
    "Policy": {
      "RequiredClaimType": "",
      "RequiredClaimValue": ""
    },
    "Settings": {
      "Scope": [ "openid", "profile" ],
      "MetadataAddress": "",
      "ClientId": "",
      "ClientSecret": ""
    }
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@halter73 Let me know if I’m interpreting this correctly and I will make the changes

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the delay. I just got back from vacation. I don't have much background on this PR, I only dropped a review because @eerhardt requested it. I have a deeper understanding of the inner workings of the OpenIdConnectHandler than the Aspire dashboard.

Is the intention of this PR to only enable/configure OIDC for the dashboard? Or can this configuration also be used by the projects orchestrated by aspire?

If it's the former, I would like to see something like your second example. It makes it much more clear the configuration only applies to the dashboard. The "AuthMode" might not even be necessary since the presence of the "Dashboard:OpenIdConnect" configuration section ought to be enough to indicate that you want to use it.

However, if the idea is to be able to share OIDC configuration between the dashboard and other projects, it does make more sense to have it split out the way you currently have it. But then, I don't see what things like NameClaimType and UsernameClaimType would be specific to the dashboard. Wouldn't you want to share that between all projects?

I can see why RequiredClaimType and RequiredClaimValue might be specific to the dashboard, but I'm not sure that would belong in an "OpenIdConnect" subsection. What if we added support for another authentication mode later that also provides claims? Would it be more future proof to put RequiredClaimType and RequiredClaimValue in a "Dashboard:Frontend:AuthorizationPolicy" section to avoid duplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just for the dashboard, that’s why I thought it made sense to put it in the normal location you’d use for any app instead of encapsulating it. If there was a way to forward the entire section to the context then I think we can avoid taking a dependency on the OpenIdConnect package and it should support every option in whatever version of the package the user is consuming. I tried to sort that out but came up empty. It seemed like it would be there already since you can take config and bind it, but I don’t see a clear path to just copying the config section since the config builder in the context doesn’t even exist at that point

Copy link
Member

Choose a reason for hiding this comment

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

If there was a way to forward the entire section to the context then I think we can avoid taking a dependency on the OpenIdConnect package and it should support every option in whatever version of the package the user is consuming.

It would be nice if there was a one-liner like Bind that supported Dictonary<string, object> and created nested dictionaries when necessary, but that's not currently supported. Plus, that's not exactly what we need here since it wouldn't create UNDERSCORE__DELIMITED__SECTION__NAMES for the keys.

It doesn't seem like it would be too hard to write custom code that calls IConfguration.GetChildren() and builds up the dictionary with the right keys though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! That’s exactly what I was trying originally but the only way I could think of to do it was feed a root like authentication__ and go grab all the env variables that start with that and bring them over directly. Doesn’t seem like a very good solution, but it might be doable

options.OtlpGrpcEndpointUrl = configuration[KnownConfigNames.DashboardOtlpGrpcEndpointUrl];
options.OtlpHttpEndpointUrl = configuration[KnownConfigNames.DashboardOtlpHttpEndpointUrl];
options.OtlpApiKey = configuration["AppHost:OtlpApiKey"];

options.AspNetCoreEnvironment = configuration["ASPNETCORE_ENVIRONMENT"] ?? "Production";
Expand Down
43 changes: 31 additions & 12 deletions src/Aspire.Hosting/DistributedApplicationBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,17 +204,11 @@ public DistributedApplicationBuilder(DistributedApplicationOptions options)
// Dashboard
if (!options.DisableDashboard)
{
if (!IsDashboardUnsecured(_innerBuilder.Configuration))
{
// Set a random API key for the OTLP exporter.
// Passed to apps as a standard OTEL attribute to include in OTLP requests and the dashboard to validate.
_innerBuilder.Configuration.AddInMemoryCollection(
new Dictionary<string, string?>
{
["AppHost:OtlpApiKey"] = TokenGenerator.GenerateToken()
}
);
var dashboardAuthMode = GetDashboardAuthMode(_innerBuilder.Configuration);
_innerBuilder.Configuration[DashboardConfigNames.DashboardFrontendAuthModeName.ConfigKey] = dashboardAuthMode.ToString();

if (dashboardAuthMode == DashboardAuthMode.BrowserToken)
{
// Determine the frontend browser token.
if (_innerBuilder.Configuration[KnownConfigNames.DashboardFrontendBrowserToken] is not { Length: > 0 } browserToken)
{
Expand All @@ -228,6 +222,18 @@ public DistributedApplicationBuilder(DistributedApplicationOptions options)
["AppHost:BrowserToken"] = browserToken
}
);
}

if (dashboardAuthMode != DashboardAuthMode.Unsecured)
{
// Set a random API key for the OTLP exporter.
// Passed to apps as a standard OTEL attribute to include in OTLP requests and the dashboard to validate.
_innerBuilder.Configuration.AddInMemoryCollection(
new Dictionary<string, string?>
{
["AppHost:OtlpApiKey"] = TokenGenerator.GenerateToken()
}
);

// Determine the resource service API key.
if (_innerBuilder.Configuration[KnownConfigNames.DashboardResourceServiceClientApiKey] is not { Length: > 0 } apiKey)
Expand Down Expand Up @@ -348,9 +354,22 @@ private void MapTransportOptionsFromCustomKeys(TransportOptions options)
}
}

private static bool IsDashboardUnsecured(IConfiguration configuration)
private static DashboardAuthMode GetDashboardAuthMode(IConfiguration configuration)
{
return configuration.GetBool(KnownConfigNames.DashboardUnsecuredAllowAnonymous) ?? false;
if (configuration.GetBool(KnownConfigNames.DashboardUnsecuredAllowAnonymous) ?? false)
{
return DashboardAuthMode.Unsecured;
}

var authModeString = configuration[DashboardConfigNames.DashboardFrontendAuthModeName.EnvVarName] ??
configuration[DashboardConfigNames.DashboardFrontendAuthModeName.ConfigKey];

if (Enum.TryParse<DashboardAuthMode>(authModeString, true, out var authMode))
{
return authMode;
}

return DashboardAuthMode.BrowserToken;
}

private void ConfigurePublishingOptions(DistributedApplicationOptions options)
Expand Down
4 changes: 4 additions & 0 deletions src/Shared/DashboardConfigNames.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ internal static class DashboardConfigNames
public static readonly ConfigName DashboardOtlpCorsAllowedHeadersKeyName = new("Dashboard:Otlp:Cors:AllowedHeaders", "DASHBOARD__OTLP__CORS__ALLOWEDHEADERS");
public static readonly ConfigName DashboardOtlpAllowedCertificatesName = new("Dashboard:Otlp:AllowedCertificates", "DASHBOARD__OTLP__ALLOWEDCERTIFICATES");
public static readonly ConfigName DashboardFrontendAuthModeName = new("Dashboard:Frontend:AuthMode", "DASHBOARD__FRONTEND__AUTHMODE");
public static readonly ConfigName DashboardFrontendOpenIdConnectNameClaimType = new("Dashboard:Frontend:OpenIdConnect:NameClaimType", "DASHBOARD__FRONTEND__OPENIDCONNECT__NAMECLAIMTYPE");
public static readonly ConfigName DashboardFrontendOpenIdConnectUsernameClaimType = new("Dashboard:Frontend:OpenIdConnect:UsernameClaimType", "DASHBOARD__FRONTEND__OPENIDCONNECT__USERNAMECLAIMTYPE");
public static readonly ConfigName DashboardFrontendOpenIdConnectRequiredClaimType = new("Dashboard:Frontend:OpenIdConnect:RequiredClaimType", "DASHBOARD__FRONTEND__OPENIDCONNECT__REQUIREDCLAIMTYPE");
public static readonly ConfigName DashboardFrontendOpenIdConnectRequiredClaimValue = new("Dashboard:Frontend:OpenIdConnect:RequiredClaimValue", "DASHBOARD__FRONTEND__OPENIDCONNECT__REQUIREDCLAIMVALUE");
public static readonly ConfigName DashboardFrontendBrowserTokenName = new("Dashboard:Frontend:BrowserToken", "DASHBOARD__FRONTEND__BROWSERTOKEN");
public static readonly ConfigName DashboardFrontendMaxConsoleLogCountName = new("Dashboard:Frontend:MaxConsoleLogCount", "DASHBOARD__FRONTEND__MAXCONSOLELOGCOUNT");
public static readonly ConfigName ResourceServiceClientAuthModeName = new("Dashboard:ResourceServiceClient:AuthMode", "DASHBOARD__RESOURCESERVICECLIENT__AUTHMODE");
Expand Down