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

Resource service supports API key authentication #3400

Merged
merged 7 commits into from
Apr 10, 2024

Conversation

drewnoakes
Copy link
Member

@drewnoakes drewnoakes commented Apr 4, 2024

Adds support for an API key to authenticate the resource service.

Unless unsecured, the app host will generate an API key and pass it to the dashboard via an environment variable. The dashboard then includes this key in a header for all gRPC calls. The app host's resource service validates that the expected key is received and rejects requests where the key is omitted.

Microsoft Reviewers: Open in CodeFlow

Unless unsecured, the app host will generate an API key and pass it to the dashboard via an environment variable. The dashboard then includes this key in a header for all gRPC calls. The app host's resource service validates that the expected key is received and rejects requests where the key is omitted.
@drewnoakes drewnoakes force-pushed the resource-service-token-auth branch from 84bba9c to cf1276f Compare April 4, 2024 11:39
@drewnoakes
Copy link
Member Author

CI failure seems unrelated to these changes. Also occurring in #3402.

Copy link
Member

@tlmii tlmii left a comment

Choose a reason for hiding this comment

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

I'm not the expert in this area but this all makes sense to me.

src/Aspire.Dashboard/README.md Outdated Show resolved Hide resolved
Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

Needs integration tests. Either added now or in a follow up issue.

src/Aspire.Dashboard/Model/DashboardClient.cs Show resolved Hide resolved
src/Aspire.Dashboard/README.md Outdated Show resolved Hide resolved
src/Aspire.Hosting/Dashboard/DashboardServiceHost.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting/Dashboard/ResourceServiceOptions.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting/Dcp/ApplicationExecutor.cs Outdated Show resolved Hide resolved
@danmoseley
Copy link
Member

danmoseley commented Apr 5, 2024

In general we should have any necessary tests included in any port PR to release branch.

@davidfowl
Copy link
Member

What's the state of this PR @drewnoakes? main and release/8.0 have diverged a bit on the app model WRT how the dashboard is configured. That will make backporting a little more challenging

@drewnoakes
Copy link
Member Author

I'm bringing this up to date and looking at feedback today.

# Conflicts:
#	src/Aspire.Hosting/Dcp/ApplicationExecutor.cs
#	src/Aspire.Hosting/DistributedApplicationBuilder.cs
#	tests/Aspire.Hosting.Tests/Dcp/ApplicationExecutorTests.cs
@drewnoakes
Copy link
Member Author

Needs integration tests. Either added now or in a follow up issue.

Already called out in linked issue.

Comment on lines +21 to +24
builder.Configuration.AddInMemoryCollection(new Dictionary<string, string?>
{
["DOTNET_ASPIRE_SHOW_DASHBOARD_RESOURCES"] = null
});
Copy link
Member

Choose a reason for hiding this comment

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

This got fixed in VS today 😄

@drewnoakes drewnoakes requested review from JamesNK and davidfowl April 9, 2024 09:05
@@ -75,6 +76,32 @@ internal sealed class DashboardServiceHost : IHostedService
// Configuration
builder.Services.AddSingleton(configuration);

var resourceServiceConfigSection = configuration.GetSection("AppHost:ResourceService");
Copy link
Member

Choose a reason for hiding this comment

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

We don't want any auth in unsecured mode.

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 [Authorize(Policy = ...)] attribute on the gRPC service requires us to wire up auth.

We can switch on the auth mode and register a no-op policy that just allows everything, and not register the auth scheme. However, this introduces a bunch more code (as far as I can tell). We need to add the config in two layers (both outer and inner apps).

Overall I don't think it's an improvement. Here's the diff I came up with to do what I think you're asking:

Diff
diff --git a/src/Aspire.Hosting/Dashboard/DashboardServiceAuth.cs b/src/Aspire.Hosting/Dashboard/DashboardServiceAuth.cs
index a82b5bbd5..56da4b091 100644
--- a/src/Aspire.Hosting/Dashboard/DashboardServiceAuth.cs
+++ b/src/Aspire.Hosting/Dashboard/DashboardServiceAuth.cs
@@ -36,22 +36,25 @@ protected override Task<AuthenticateResult> HandleAuthenticateAsync()
     {
         var options = resourceServiceOptions.CurrentValue;

-        if (options.AuthMode is ResourceServiceAuthMode.ApiKey)
+        if (options.AuthMode is not ResourceServiceAuthMode.ApiKey)
         {
-            if (!Request.Headers.TryGetValue(ApiKeyHeaderName, out var headerValues))
-            {
-                return Task.FromResult(AuthenticateResult.Fail($"'{ApiKeyHeaderName}' header not found"));
-            }
+            // Should be unreachable.
+            throw new InvalidOperationException("Auth mode must be ApiKey when this handler is active.");
+        }

-            if (headerValues.Count != 1)
-            {
-                return Task.FromResult(AuthenticateResult.Fail($"Expecting only a single '{ApiKeyHeaderName}' header."));
-            }
+        if (!Request.Headers.TryGetValue(ApiKeyHeaderName, out var headerValues))
+        {
+            return Task.FromResult(AuthenticateResult.Fail($"'{ApiKeyHeaderName}' header not found"));
+        }

-            if (!CompareHelpers.CompareKey(expectedKeyBytes: options.GetApiKeyBytes(), requestKey: headerValues.ToString()))
-            {
-                return Task.FromResult(AuthenticateResult.Fail($"Invalid '{ApiKeyHeaderName}' header value."));
-            }
+        if (headerValues.Count != 1)
+        {
+            return Task.FromResult(AuthenticateResult.Fail($"Expecting only a single '{ApiKeyHeaderName}' header."));
+        }
+
+        if (!CompareHelpers.CompareKey(expectedKeyBytes: options.GetApiKeyBytes(), requestKey: headerValues.ToString()))
+        {
+            return Task.FromResult(AuthenticateResult.Fail($"Invalid '{ApiKeyHeaderName}' header value."));
         }

         return Task.FromResult(
diff --git a/src/Aspire.Hosting/Dashboard/DashboardServiceHost.cs b/src/Aspire.Hosting/Dashboard/DashboardServiceHost.cs
index 36ba2b39d..74f1bb1c9 100644
--- a/src/Aspire.Hosting/Dashboard/DashboardServiceHost.cs
+++ b/src/Aspire.Hosting/Dashboard/DashboardServiceHost.cs
@@ -51,6 +51,7 @@ internal sealed class DashboardServiceHost : IHostedService
         DistributedApplicationModel applicationModel,
         IKubernetesService kubernetesService,
         IConfiguration configuration,
+        IOptions<ResourceServiceOptions> resourceServiceOptions,
         DistributedApplicationExecutionContext executionContext,
         ILoggerFactory loggerFactory,
         IConfigureOptions<LoggerFilterOptions> loggerOptions,
@@ -76,31 +77,45 @@ internal sealed class DashboardServiceHost : IHostedService
             // Configuration
             builder.Services.AddSingleton(configuration);

+            // These options are also constructed in the outer app. They are validated there,
+            // rather than here, so we don't perform validation twice.
             var resourceServiceConfigSection = configuration.GetSection("AppHost:ResourceService");
             builder.Services.AddOptions<ResourceServiceOptions>()
-                .Bind(resourceServiceConfigSection)
-                .ValidateOnStart();
-            builder.Services.AddSingleton<IValidateOptions<ResourceServiceOptions>, ValidateResourceServiceOptions>();
-
-            // Configure authentication scheme for the dashboard service
-            builder.Services
-                .AddAuthentication()
-                .AddScheme<ResourceServiceApiKeyAuthenticationOptions, ResourceServiceApiKeyAuthenticationHandler>(
-                    ResourceServiceApiKeyAuthenticationDefaults.AuthenticationScheme,
-                    options => { });
-
-            // Configure authorization policy for the dashboard service.
-            // The authorization policy accepts anyone who successfully authenticates via the
-            // specified scheme, and that scheme enforces a valid API key (when configured to
-            // use API keys for calls.)
-            builder.Services
-                .AddAuthorizationBuilder()
-                .AddPolicy(
-                    name: ResourceServiceApiKeyAuthorization.PolicyName,
-                    policy: new AuthorizationPolicyBuilder(
-                        ResourceServiceApiKeyAuthenticationDefaults.AuthenticationScheme)
-                        .RequireAuthenticatedUser()
-                        .Build());
+                .Bind(resourceServiceConfigSection);
+
+            if (resourceServiceOptions.Value.AuthMode == ResourceServiceAuthMode.ApiKey)
+            {
+                // Configure authentication scheme for the resource service
+                builder.Services
+                    .AddAuthentication()
+                    .AddScheme<ResourceServiceApiKeyAuthenticationOptions, ResourceServiceApiKeyAuthenticationHandler>(
+                        ResourceServiceApiKeyAuthenticationDefaults.AuthenticationScheme,
+                        options => { });
+
+                // Configure authorization policy for the resource service.
+                // The authorization policy accepts anyone who successfully authenticates via the
+                // specified scheme, and that scheme enforces a valid API key (when configured to
+                // use API keys for calls.)
+                builder.Services
+                    .AddAuthorizationBuilder()
+                    .AddPolicy(
+                        name: ResourceServiceApiKeyAuthorization.PolicyName,
+                        policy: new AuthorizationPolicyBuilder(
+                            ResourceServiceApiKeyAuthenticationDefaults.AuthenticationScheme)
+                            .RequireAuthenticatedUser()
+                            .Build());
+            }
+            else
+            {
+                builder.Services.AddAuthentication();
+                builder.Services
+                    .AddAuthorizationBuilder()
+                    .AddPolicy(
+                        name: ResourceServiceApiKeyAuthorization.PolicyName,
+                        policy: new AuthorizationPolicyBuilder()
+                            .RequireAssertion(_ => true)
+                            .Build());
+            }

             // Logging
             builder.Services.AddSingleton(loggerFactory);
diff --git a/src/Aspire.Hosting/DistributedApplicationBuilder.cs b/src/Aspire.Hosting/DistributedApplicationBuilder.cs
index 705077790..c1b758d98 100644
--- a/src/Aspire.Hosting/DistributedApplicationBuilder.cs
+++ b/src/Aspire.Hosting/DistributedApplicationBuilder.cs
@@ -151,6 +151,12 @@ public DistributedApplicationBuilder(DistributedApplicationOptions options)
                     );
                 }

+                var resourceServiceConfigSection = _innerBuilder.Configuration.GetSection("AppHost:ResourceService");
+                _innerBuilder.Services.AddOptions<ResourceServiceOptions>()
+                    .Bind(resourceServiceConfigSection)
+                    .ValidateOnStart();
+                _innerBuilder.Services.AddSingleton<IValidateOptions<ResourceServiceOptions>, ValidateResourceServiceOptions>();
+
                 _innerBuilder.Services.AddOptions<TransportOptions>().ValidateOnStart().PostConfigure(MapTransportOptionsFromCustomKeys);
                 _innerBuilder.Services.TryAddEnumerable(ServiceDescriptor.Singleton<IValidateOptions<TransportOptions>, TransportOptionsValidator>());
                 _innerBuilder.Services.AddSingleton<DashboardServiceHost>();

@drewnoakes drewnoakes force-pushed the resource-service-token-auth branch from d5bbee5 to 230dae9 Compare April 10, 2024 09:32
@davidfowl
Copy link
Member

Approved contingent on adding a test to make sure that auth works.

@drewnoakes drewnoakes merged commit a56a76d into dotnet:main Apr 10, 2024
8 checks passed
@drewnoakes drewnoakes deleted the resource-service-token-auth branch April 10, 2024 23:45
@drewnoakes
Copy link
Member Author

/backport to release/8.0

Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/aspire/actions/runs/8639818679

Copy link
Contributor

@drewnoakes backporting to release/8.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Make CompareHelpers shared
Applying: Resource service supports API keys
error: sha1 information is lacking or useless (src/Shared/CompareHelpers.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Resource service supports API keys
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Copy link
Contributor

@drewnoakes an error occurred while backporting to release/8.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

drewnoakes added a commit to drewnoakes/aspire that referenced this pull request Apr 11, 2024
* Make CompareHelpers shared

* Resource service supports API keys

Unless unsecured, the app host will generate an API key and pass it to the dashboard via an environment variable. The dashboard then includes this key in a header for all gRPC calls. The app host's resource service validates that the expected key is received and rejects requests where the key is omitted.

* Remove test's exposure to ambient environment variables

* Review feedback

* Require authenticated user

* Renaming
@drewnoakes
Copy link
Member Author

Manual cherry pick of the merge commit worked.

RussKie pushed a commit that referenced this pull request Apr 11, 2024
* Make CompareHelpers shared

* Resource service supports API keys

Unless unsecured, the app host will generate an API key and pass it to the dashboard via an environment variable. The dashboard then includes this key in a header for all gRPC calls. The app host's resource service validates that the expected key is received and rejects requests where the key is omitted.

* Remove test's exposure to ambient environment variables

* Review feedback

* Require authenticated user

* Renaming
@danmoseley danmoseley mentioned this pull request Apr 12, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants