Add authn/authz to OTLP endpoint, refactor dashboard endpoint creation, integration tests #2316

JamesNK · 2024-02-20T08:50:54Z

Lots of dashboard changes for the reasons of auth. Work in progress.

Change dashboard endpoint configuration from code to configuration + code. Allows external endpoint configuration when starting the dashboard. This gives people who want to launch the dashboard a lot more flexibility over endpoint configuration. For example, the TLS cert can be different per endpoint via configuration, Kestrel:Endpoints:Otlp:Certification:...=xxx.
Enable auth on OTLP endpoints
- Add composite auth
- Add API key auth (enabled via config)
- Add client cert auth (enabled via config)
- Change connection validation to happen in auth instead of gRPC interceptor.
Integration tests of all the scenarios (http vs https vs different certs vs mTLS vs api key auth)

Microsoft Reviewers: Open in CodeFlow

src/Aspire.Dashboard/Authentication/ApiKeyAuthorizationHandler.cs

src/Aspire.Dashboard/Authentication/OtlpCompositeAuthenticationHandler.cs

src/Aspire.Dashboard/DashboardWebApplication.cs

davidfowl · 2024-02-23T06:52:31Z

Do we need to test this with anything?

joperezr · 2024-02-26T23:35:46Z

Friendly reminder that this PR is assigned to the Preview 4 milestone which is snapping in a couple of hours. Please ensure it is merged by 4pm PST in order to ensure the change will be part of Preview 4.

src/Aspire.Dashboard/DashboardWebApplication.cs

mitchdenny · 2024-02-29T12:36:51Z

I might be blind, but I'm not seeing where we make use of these values when we launch the dashboard from the apphost? Is that going to be done separately?

JamesNK · 2024-02-29T12:40:51Z

They're mostly for cloud providers to opt-in to using. We might add API key auth in the local environment.

JamesNK · 2024-03-07T04:41:16Z

I think this PR is mergable as a starting point for dashboard auth. I think there will still be changes, especially to the OTLP API key auth based on feedback, but this is a good starting point.

I don't want to leave this changing around, building up merge conflicts.

kvenkatrajan

Looks great! Thanks for providing tests.

kvenkatrajan · 2024-03-07T15:55:59Z

src/Aspire.Dashboard/DashboardWebApplication.cs

-    private const string DashboardUrlVariableName = "ASPNETCORE_URLS";
-    private const string DashboardUrlDefaultValue = "http://localhost:18888";
+    internal const string DashboardOtlpAuthModeVariableName = "DOTNET_DASHBOARD_OTLP_AUTH_MODE";
+    internal const string DashboardOtlpApiKeyVariableName = "DOTNET_DASHBOARD_OTLP_API_KEY";


Are we planning to add the keyperfile configuration/volume info here to support ACA? We can do it in a separate PR post this merge.

Maybe. Will talk more with people to figure out exact requirements. But I don't think that blocks this PR. Merging something that isn't the final result is ok.

kvenkatrajan · 2024-03-07T16:03:17Z

src/Aspire.Dashboard/DashboardWebApplication.cs

+                case OtlpAuthMode.None:
+                    break;
+                case OtlpAuthMode.ApiKey:
+                    otlpApiKey = configuration[DashboardOtlpApiKeyVariableName];


Similarly here do we to fork out logic to retreive apiKey for ACA scenario? How would we determine from the appModel whether the dashboard is hosted in ACA? Can be in a separate PR - just curious

Still to figure out.

iphdav · 2024-03-10T06:41:26Z

One question: Has the industry adopted x-otlp-api-key as the name of the HTTP Header for the API Key? It is obviously easy to use any header when emitting the telemetry. Should the Options class in the Dashboard app allow a different key http header name to be specified as well as the key's value?
Great work though, this will be great.

JamesNK · 2024-03-10T06:45:26Z

I made it up. If there is a standard then let me know.

I don't see a reason to make the name configurable.

iphdav · 2024-03-10T06:56:18Z

I have just adopted your new standard James :-)

JamesNK · 2024-03-11T22:38:33Z

I'm still looking for someone to review this. Please review this so I don't have to keep rebasing changes to avoid merge conflicts.

davidfowl · 2024-03-11T22:44:24Z

I'll review

drewnoakes

Would be good to document the new options in the dashboard's README.md file.

Also, lots of new public types here. Can these be internal, so we aren't tied to back compat in future? I don't see why consumers would need to use these types.

drewnoakes · 2024-03-13T23:34:15Z

src/Aspire.Dashboard/DashboardWebApplication.cs

+                    OnCertificateValidated = context =>
+                    {
+                        var claims = new[]
+                        {
+                            new Claim(ClaimTypes.NameIdentifier,
+                                context.ClientCertificate.Subject,
+                                ClaimValueTypes.String, context.Options.ClaimsIssuer),
+                            new Claim(ClaimTypes.Name,
+                                context.ClientCertificate.Subject,
+                                ClaimValueTypes.String, context.Options.ClaimsIssuer)
+                        };
+
+                        context.Principal = new ClaimsPrincipal(new ClaimsIdentity(claims, context.Scheme.Name));
+                        context.Success();
+
+                        return Task.CompletedTask;
+                    }


Checking my understanding here: the dashboard will accept any valid certificate here, not a pre-shared/specific one?

If that's the case, then this isn't mTLS (as I understand it), as the identity of the other party is not restricted to a given certificate. Any cert that's trusted on the machine will be accepted. Maybe we're ok with that.

That's right. I was planning to get feedback whether more to lock down the client cert was required.

The ability to lock down the cert should be supported (file a follow up).

Directory.Packages.props

src/Aspire.Dashboard/Authentication/OtlpApiKey/OtlpApiKeyAuthenticationHandler.cs

src/Aspire.Dashboard/Authentication/OtlpCompositeAuthenticationHandler.cs

src/Aspire.Dashboard/DashboardWebApplication.cs

src/Aspire.Dashboard/DashboardConfiguration.cs

tests/Shared/TestCertificates/eku.client.pfx

tests/Aspire.Dashboard.Tests/Integration/OtlpServiceTests.cs

JamesNK · 2024-03-14T03:26:11Z

Would be good to document the new options in the dashboard's README.md file.

That's the plan, but I'll wait for concensus about the changes before updating docs.

JamesNK · 2024-03-14T04:04:50Z

I'll review

@davidfowl I'm interested in your feedback about how I changed the configuration of endpoints. The new system allows for basic configuration using ASPNETCORE_URLS + DOTNET_DASHBOARD_OTLP_ENDPOINT_URL or using more advanced Kestrel config, e.g. Kestrel::Endpoints::OtlpEndpoint::xxxx.

drewnoakes · 2024-03-18T21:14:07Z

@JamesNK @davidfowl can we get this merged in please? I have more auth work to submit that'll conflict a little with this.

davidfowl · 2024-03-18T21:20:24Z

src/Aspire.Dashboard/DashboardWebApplication.cs

+            values[$"Kestrel:Endpoints:{endpointName}:Url"] = url;
+
+            if (protocols != null)
+            {
+                values[$"Kestrel:Endpoints:{endpointName}:Protocols"] = protocols.ToString();
+            }
+
+            if (requiredClientCertificate)
+            {
+                values[$"Kestrel:Endpoints:{endpointName}:ClientCertificateMode"] = ClientCertificateMode.RequireCertificate.ToString();
+            }
+        }


My only question (non blocking) was why do this instead of using the API to create named endpoints?

From PR description:

Change dashboard endpoint configuration from code to configuration + code. Allows external endpoint configuration when starting the dashboard. This gives people who want to launch the dashboard a lot more flexibility over endpoint configuration. For example, the TLS cert can be different per endpoint via configuration, Kestrel:Endpoints:Otlp:Certification:...=xxx.

Without this, we'd need to recreate a lot of configuration infrastructure ourselves, i.e. provide a way to configure the TLS cert for each endpoint from a cert store, or from a pdf, or from a cert+key, etc.

drewnoakes · 2024-03-18T22:38:40Z

Pushed a fix to resolve a simple conflict in eng/Versions.props.

eng/Versions.props

… creation, integration tests (#2316)" This reverts commit 1c55890.

JamesNK added the area-dashboard label Feb 20, 2024

davidfowl reviewed Feb 20, 2024

View reviewed changes

src/Aspire.Dashboard/Authentication/ApiKeyAuthorizationHandler.cs Outdated Show resolved Hide resolved

JamesNK requested a review from halter73 February 21, 2024 03:47

davidfowl reviewed Feb 21, 2024

View reviewed changes

src/Aspire.Dashboard/Authentication/OtlpCompositeAuthenticationHandler.cs Show resolved Hide resolved

davidfowl reviewed Feb 21, 2024

View reviewed changes

src/Aspire.Dashboard/DashboardWebApplication.cs Outdated Show resolved Hide resolved

mitchdenny assigned JamesNK Feb 23, 2024

mitchdenny added this to the preview 4 (Mar) milestone Feb 23, 2024

JamesNK modified the milestones: preview 4 (Mar), preview 5 (Apr) Feb 26, 2024

JamesNK added the security 🔐 label Feb 28, 2024

mitchdenny reviewed Feb 29, 2024

View reviewed changes

src/Aspire.Dashboard/DashboardWebApplication.cs Outdated Show resolved Hide resolved

JamesNK force-pushed the jamesnk/dashboard-authpocalypse branch from c47e359 to 984afa0 Compare March 7, 2024 03:50

JamesNK requested review from adamint, drewnoakes, kvenkatrajan and tlmii March 7, 2024 04:36

kvenkatrajan reviewed Mar 7, 2024

View reviewed changes

JamesNK mentioned this pull request Mar 10, 2024

Azure Dashboard OTLP Receiver - Verify HTTP Header Key before accepting telemetry #2757

Closed

drewnoakes reviewed Mar 14, 2024

View reviewed changes

Dashboard authpocalypse

995c6fa

JamesNK force-pushed the jamesnk/dashboard-authpocalypse branch from cee44ec to 995c6fa Compare March 14, 2024 03:11

JamesNK added 2 commits March 14, 2024 11:38

PR feedback

3da57b8

PR feedback

1f364d0

Clean up

4810647

drewnoakes approved these changes Mar 14, 2024

View reviewed changes

davidfowl approved these changes Mar 18, 2024

View reviewed changes

davidfowl reviewed Mar 18, 2024

View reviewed changes

Merge branch 'main' into jamesnk/dashboard-authpocalypse

54e64aa

JamesNK changed the title ~~Dashboard authpocalypse~~ Add authn/authz to OTLP endpoint, refactor dashboard endpoint creation, integration tests Mar 18, 2024

JamesNK commented Mar 18, 2024

View reviewed changes

eng/Versions.props Outdated Show resolved Hide resolved

Update eng/Versions.props

e4e34bb

JamesNK enabled auto-merge (squash) March 18, 2024 23:38

JamesNK merged commit 1c55890 into main Mar 19, 2024

JamesNK deleted the jamesnk/dashboard-authpocalypse branch March 19, 2024 00:24

JamesNK added a commit that referenced this pull request Mar 19, 2024

Revert "Add authn/authz to OTLP endpoint, refactor dashboard endpoint…

11f3b69

… creation, integration tests (#2316)" This reverts commit 1c55890.

kvenkatrajan mentioned this pull request Mar 25, 2024

Create and review threat model for Aspire Dashboard #2679

Closed

10 tasks

github-actions bot locked and limited conversation to collaborators Apr 21, 2024

Add authn/authz to OTLP endpoint, refactor dashboard endpoint creation, integration tests #2316

Add authn/authz to OTLP endpoint, refactor dashboard endpoint creation, integration tests #2316

Uh oh!

Conversation

JamesNK commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davidfowl commented Feb 23, 2024

Uh oh!

joperezr commented Feb 26, 2024

Uh oh!

Uh oh!

mitchdenny commented Feb 29, 2024

Uh oh!

JamesNK commented Feb 29, 2024

Uh oh!

JamesNK commented Mar 7, 2024

Uh oh!

kvenkatrajan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iphdav commented Mar 10, 2024

Uh oh!

JamesNK commented Mar 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iphdav commented Mar 10, 2024

Uh oh!

JamesNK commented Mar 11, 2024

Uh oh!

davidfowl commented Mar 11, 2024

Uh oh!

drewnoakes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JamesNK commented Mar 14, 2024

Uh oh!

JamesNK commented Mar 14, 2024

Uh oh!

drewnoakes commented Mar 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drewnoakes commented Mar 18, 2024

Uh oh!

Uh oh!

Reviewers

JamesNK commented Feb 20, 2024 •

edited

Loading

JamesNK commented Mar 10, 2024 •

edited

Loading

JamesNK Mar 18, 2024 •

edited

Loading