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

Add Keycloak component #4289

Merged
merged 25 commits into from
Jul 18, 2024
Merged

Add Keycloak component #4289

merged 25 commits into from
Jul 18, 2024

Conversation

julioct
Copy link
Contributor

@julioct julioct commented May 24, 2024

  • Adds the Keycloak resource and extension methods.
  • There is no client, since all services need is to discover the Keycloak server base address to configure the JWT/OpenID Authority.

Contributes to #1326

Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-integrations Issues pertaining to Aspire Integrations packages label May 24, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 24, 2024
@timheuer
Copy link
Member

Given no client and your note about the base address -- I suspect you'd rely on folks using service discovery based on the name then... as AddKeycloak("foo") would then be an env var of services__foo__http__0 and you're expecting them to have the authority set as https+http://foo right?

Some other questions I have as a noob to Keycloak...

  • Config: looks like environment vars are supported for most config, but also a .conf -- might consider adding a bind mount for conf/keycloak.conf (or the correct means as it looks like config file is not an environment config that could read)
  • Production DB: Might be good to see how this all looks with the preferred documentation way of using Postgres as the prod DB for non-dev mode

@julioct
Copy link
Contributor Author

julioct commented May 25, 2024

@timheuer Yes, all this does is enable service discovery so that services can set the base address of the Authority parameter when configuring OIDC.

Here's a simplified example of how I'm using it in the app I'm working on:

AppHost

var builder = DistributedApplication.CreateBuilder(args);

var keycloak = builder.AddKeycloak("keycloak")
                      .WithDataVolume();

var api = builder.AddProject<Projects.GameStore_Api>("gamestore-api")
                 .WithReference(keycloak);

builder.AddProject<Projects.GameStore_Frontend>("gamestore-frontend")
       .WithReference(api)
       .WithReference(keycloak);

builder.Build().Run();

Blazor App

builder.Services.AddHttpClient("OpenIdConnectBackchannel", o => o.BaseAddress = new("http://keycloak"));

builder.Services
        .AddAuthentication(Schemes.Keycloak)
        .AddCookie(CookieAuthenticationDefaults.AuthenticationScheme)
        .AddOpenIdConnect(Schemes.Keycloak, options =>
        {
            options.SignInScheme = CookieAuthenticationDefaults.AuthenticationScheme;
            options.SignOutScheme = Schemes.Keycloak;
            options.ResponseType = OpenIdConnectResponseType.Code;
            options.UsePkce = true;
            options.SaveTokens = true;
            options.MapInboundClaims = false;
            options.Scope.Add("games:all");
            options.Scope.Add(OpenIdConnectScope.OfflineAccess);
            options.TokenValidationParameters.NameClaimType = JwtRegisteredClaimNames.Name;
        });

builder.Services
        .AddOptions<OpenIdConnectOptions>(Schemes.Keycloak)
        .Configure<IConfiguration, IHttpClientFactory, IHostEnvironment>((options, configuration, httpClientFactory, hostEnvironment) =>
        {
            var realm = configuration["Keycloak:Realm"] ?? throw new InvalidOperationException("Realm is not set");
            var clientId = configuration["Keycloak:ClientId"] ?? throw new InvalidOperationException("ClientId is not set");
            var backchannelHttpClient = httpClientFactory.CreateClient("OpenIdConnectBackchannel");

            options.Backchannel = backchannelHttpClient;
            options.Authority = $"{backchannelHttpClient.BaseAddress}/realms/{realm}";
            options.ClientId = clientId;
            options.RequireHttpsMetadata = !hostEnvironment.IsDevelopment();
        });

The whole backchannel deal is weird, but seems to be the only way to do this since setting the Authority like this would not resolve the real keycloak address (which is sad):

options.Authority = $"http://keycloak/realms/{realm}";

I learned about that approach in this eShop workshop.

Regarding the config and Production, I honestly don't know. All I know is how to use Keycloak for my dev environment, but I have no intention to take it to Prod nor I have any idea how to do so. Although there are some pointers here.

I just started using Keycloak a few days ago and, since I got it working with Aspire, I thought I would contribute what I got working.

@davidfowl
Copy link
Member

We need tests and a playground project for this.
We also need to discuss this is a good MVP for keycloak. Seems like we should expose a helper for importing realms as well https://github.com/dotnet-presentations/eshop-app-workshop/blob/10390e5a0a4778d622c218f72578ddc1283ce562/src/eShop.AppHost/KeycloakResource.cs#L51

var myService = builder.AddProject<Projects.MyService>()
.WithReference(keycloak);
```

Copy link
Member

Choose a reason for hiding this comment

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

It would be good to add an example here of what code is required in the consuming ASP.NET Core project resource to configure the OIDC authentication handler to work with the injected Keycloak URL via service discovery.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example added to the Keycloak component.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should have it in both READMEs, so people can connect the 2 packages (one for the AppHost and one for their projects). Or at least point to which NuGet package should be used in the apps here.

@DamianEdwards
Copy link
Member

We need tests and a playground project for this.
We also need to discuss this is a good MVP for keycloak. Seems like we should expose a helper for importing realms as well

I think this is enough for an MVP, assuming the WithDataVolume method works to persist all data between container instances.

@julioct can you add a playground project and tests for this in this PR too please?

@julioct
Copy link
Contributor Author

julioct commented Jun 11, 2024

We need tests and a playground project for this.
We also need to discuss this is a good MVP for keycloak. Seems like we should expose a helper for importing realms as well

I think this is enough for an MVP, assuming the WithDataVolume method works to persist all data between container instances.

@julioct can you add a playground project and tests for this in this PR too please?

@DamianEdwards Yep, WithDataVolume persists all data between container instances.
Working on a playground project, and a few more things...

@julioct
Copy link
Contributor Author

julioct commented Jun 11, 2024

  • Added a new Keycloak component to simplify the JWT bearer and OIDC authentication configuration for consuming projects
  • Added a playground

@@ -0,0 +1,22 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Member

Choose a reason for hiding this comment

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

I think we should consider naming this something more specific so that it's clear it's not a client for the Keycloak admin API, but rather a package for easily configuring an ASP.NET Core app to use Keycloak as an IDP. Perhaps Aspire.Keycloak.Authentication?

@eerhardt thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that Aspire.Keycloak is a pretty broad/general name. I'm not sure I have a great suggestion. But Aspire.Keycloak.Authentication seems better.

Copy link
Member

Choose a reason for hiding this comment

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

@julioct - thoughts on the naming here?

var backchannelHttpClient = httpClientFactory.CreateClient(KeycloakBackchannel);

options.Backchannel = backchannelHttpClient;
options.Authority = $"{backchannelHttpClient.BaseAddress}/realms/{settings.Realm}";
Copy link
Member

Choose a reason for hiding this comment

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

Does this flow from the AppHost or is the user required to set the realm in the consuming app manually? I'd expect the realm to be pre-appended by the WithReference call in the AppHost project, e.g. .WithReference(keycloak, "myrealm")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Damian, would be great to have the realm configured in the AppHost. But what's the exact expectation here?

Like this?

var keycloak = builder.AddKeycloak("keycloak", realm: "myrealm" );

Or like this?

var keycloak = builder.AddKeycloak("keycloak")
                      .WithRealm("myrealm");

Or like this?

var apiService = builder.AddProject<Projects.Keycloak_ApiService>("apiservice")
                        .WithReference(keycloak, "myrealm");

Happy to implement it either way, but could you please point me to an example I can follow of the right way to do it?

Copy link
Member

Choose a reason for hiding this comment

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

Is Realm required (meaning is there a default if none provided)? If not, then other code building up the URI is going to eventually fail as you set the Authority, correct?

Realm to me kinda feels like a database pattern to me like you add the resource, then the realm (AddPostgres().AddDatabase)) - I kinda think this would be AddKeyCloak("keycloak").AddRealm("myrealm")

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking the last example .WithReference(keycloak, "myrealm"), rather than promote realms to child resources of their own, although I see the similarity to databases vs. database servers. Interested in others' thoughts @davidfowl @mitchdenny @eerhardt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DamianEdwards @timheuer Two comments on this one:

  1. Is adding the realm on the host what we really want? The realm is like an AAD tenant and that is something that you usually configure on your consuming projects. Feels a bit too magical for client projects to have no idea what realm they are working with.

  2. Assuming we set the realm on the host, what's the expectation of WithReference(keycloak, "myrealm")? I don't know of a way to construct a discoverable endpoint like http://localhost:1234/realms/WeatherShop directly in the host side. But I could set an environment variable that the client component will know about and read to build the endpoint. Would that work?

Copy link
Member

Choose a reason for hiding this comment

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

Feels a bit too magical for client projects to have no idea what realm they are working with.

The realm has to exist on the Keycloak server though. Client projects get the database details they're connecting to as well so not sure this is much different.

But I could set an environment variable that the client component will know about and read to build the endpoint. Would that work?

Yep that's what I was thinking. This again is very similar to what happens when you call AddDatabase("somename"). The database isn't automatically created, but it gets added to the connection information for any client project calling WithReference on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the WithReference overload. In the host you can do this now:

builder.AddProject<Projects.Keycloak_ApiService>("apiservice")
       .WithReference(keycloak, realm: "WeatherShop");

And client projects can do just this, since host provides everything:

builder.AddKeycloakJwtBearer();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now client projects need to pass in the connection name like this:

builder.AddKeycloakJwtBearer("keycloak");

Or this:

builder.AddKeycloakOpenIdConnect("keycloak");

@davidfowl
Copy link
Member

cc @NikiforovAll are you around to loan any keycloak expertise? How does this gel with https://nikiforovall.github.io/dotnet/keycloak/2024/06/02/aspire-support-for-keycloak.html and your other keycloak APIs?

Copy link

@NikiforovAll NikiforovAll left a comment

Choose a reason for hiding this comment

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

I would like to suggest producing configuration compatible with https://learn.microsoft.com/en-us/aspnet/core/fundamentals/minimal-apis/security?view=aspnetcore-8.0#configuring-authentication-strategy, this way you don't really need to define Keycloak-specific JWT Bearer scheme

ArgumentNullException.ThrowIfNull(builder);
ArgumentNullException.ThrowIfNull(keycloakBuilder);

builder.WithReference(keycloakBuilder)

Choose a reason for hiding this comment

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

Multiple realm registrations are possible. In this case, we override and use the latest one. Is it intended behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, multiple realms are not supported with the proposed implementation.

@NikiforovAll
Copy link

NikiforovAll commented Jun 13, 2024

cc @NikiforovAll are you around to loan any keycloak expertise? How does this gel with https://nikiforovall.github.io/dotnet/keycloak/2024/06/02/aspire-support-for-keycloak.html and your other keycloak APIs?

From a developer perspective, it looks good to me. 👍

It's unfortunate that this serves as an alternative to my project 🥲, Keycloak.AuthServices, there's so much more to it than just OIDC/JwtBearer integration.

It would be worthwhile to explore how to glue them together. Although it's possible to do it, I suspect that users/developers might opt for the easier route of using just the Aspire component, given its similar functionality.


I have a few ideas/comments for the proposed solution in the PR:

  1. Multiple realm registrations: I think we should consider the "multiple realms registration" scenario. Depending on the use case user might want to:
Scenario Comments
Configure "OIDC/JWT" "MyRealm" realm
Configure "Admin API" integration "Master" realm
Configure "Protection API" integration "MyRealm" realm
  1. Make use of Authentication Strategy: It might be a good idea to contribute directly to Authentication.Schemes.{Bearer|OpenIdConnect} configuration as a result of referencing the component, ref: "https://learn.microsoft.com/en-us/aspnet/core/fundamentals/minimal-apis/security?view=aspnetcore-8.0#configuring-authentication-strategy". Although, it might impact how the AppHost is built

  2. Unified auth component: I think @DamianEdwards mentioned the idea of a unified "Authentication" component, it might be more flexible and clear to add it using a separate component that has various adapters. Something like this:

var builder = DistributedApplication.CreateBuilder(args);

var keycloak = builder.AddKeycloak("keycloak").WithDataVolume();

var jwtBearer = builder.AddJwtBearer("JwtBearer")
       .WithReference(new KeycloakRealmJwtBearerAuthentication(realm: "MyRealm", keycloakInstance: keycloak));

var api = builder.AddProject<Projects.GameStore_Api>("gamestore-api")
       .WithReference(jwtBearer);

builder.Build().Run();

@davidfowl
Copy link
Member

Unified auth component: I think @DamianEdwards mentioned the idea of a unified "Authentication" component, it might be more flexible and clear to add it using a separate component that has various adapters. Something like this:

I would defer this until we have multiple auth providers. I'd rather us build the keycloak implementation and another one and then determine if we can extract the common feature set.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

As we are planning on shipping 8.1 in the coming days, do we think this is in a state where it can be merged and shipped as "preview" for 8.1? I think it would be a really good enhancemnt, and would allow us to get feedback on the functionality.

@mitchdenny - thoughts on merging this for 8.1?

tests/Aspire.Hosting.Keycloak.Tests/xunit.runner.json Outdated Show resolved Hide resolved
tests/Aspire.Hosting.Tests/Aspire.Hosting.Tests.csproj Outdated Show resolved Hide resolved
tests/Aspire.Hosting.Tests/Aspire.Hosting.Tests.csproj Outdated Show resolved Hide resolved
@@ -0,0 +1,16 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Member

Choose a reason for hiding this comment

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

Is there some sort of "functional test" that can be written? One that:

  • Starts up the Keycloak container
  • Makes requests to it like an app would
  • Verifies that the requests return successful results

I know it isn't apples-to-apples, but here's that kind of test with a Redis container:

[Fact]
[RequiresDocker]
public async Task VerifyRedisResource()
{
var builder = CreateDistributedApplicationBuilder();
var redis = builder.AddRedis("redis");
using var app = builder.Build();
await app.StartAsync();
var hb = Host.CreateApplicationBuilder();
hb.Configuration.AddInMemoryCollection(new Dictionary<string, string?>
{
[$"ConnectionStrings:{redis.Resource.Name}"] = await redis.Resource.GetConnectionStringAsync()
});
hb.AddRedisClient(redis.Resource.Name);
using var host = hb.Build();
await host.StartAsync();
var redisClient = host.Services.GetRequiredService<IConnectionMultiplexer>();
var db = redisClient.GetDatabase();
await db.StringSetAsync("key", "value");
var value = await db.StringGetAsync("key");
Assert.Equal("value", value);
}

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 there some sort of "functional test" that can be written? One that:

  • Starts up the Keycloak container
  • Makes requests to it like an app would
  • Verifies that the requests return successful results

I know it isn't apples-to-apples, but here's that kind of test with a Redis container:

[Fact]
[RequiresDocker]
public async Task VerifyRedisResource()
{
var builder = CreateDistributedApplicationBuilder();
var redis = builder.AddRedis("redis");
using var app = builder.Build();
await app.StartAsync();
var hb = Host.CreateApplicationBuilder();
hb.Configuration.AddInMemoryCollection(new Dictionary<string, string?>
{
[$"ConnectionStrings:{redis.Resource.Name}"] = await redis.Resource.GetConnectionStringAsync()
});
hb.AddRedisClient(redis.Resource.Name);
using var host = hb.Build();
await host.StartAsync();
var redisClient = host.Services.GetRequiredService<IConnectionMultiplexer>();
var db = redisClient.GetDatabase();
await db.StringSetAsync("key", "value");
var value = await db.StringGetAsync("key");
Assert.Equal("value", value);
}

I guess a "functional" test for the Keycloak component would involve checking that one of the claims of the authenticated user can be retrieved from the UserPrincipal object, either from the minimal API side or the Blazor app side.

In either case, the user has to first authenticate on Keycloak sign-in screen to complete the OIDC flow, which I'm not sure how to automate for the test.

Likely there's a way, but I can't allocate time right now to dive into it.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

From my standpoint, this looks good enough to get a first preview version out.

@mitchdenny - any concerns with merging this for 8.1?

@mitchdenny
Copy link
Member

@eerhardt @julioct happy for this to go in. My perspective is that we should aim to ship this as a preview package for the 8.1 iteration and collect feedback that might result in some API changes.

For example, I think the int? port argument on AddKeycloak(...) might be a bit of a footgun for folks if you almost always need to to be a stable port when doing interactive auth flows. But I think overall this is in a good position now.

I'm going to kick the build just to double check everything is stable against main and then merge.

@mitchdenny
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mitchdenny
Copy link
Member

@julioct thanks for contributing this to the .NET Aspire project! Going through the review process to add not only hosting APIs but also component APIs is a big effort and we appreciate it.

Our plan is to ship this package as a preview in .NET Aspire 8.1 with a view for marking the package as stable in .NET Aspire 8.2.

We'll mention this new resource / component in the upcoming .NET Aspire 8.1 blog post, are you happy for us to link to your GitHub profile and give you a shout out?

@mitchdenny mitchdenny merged commit 724a1de into dotnet:main Jul 18, 2024
9 checks passed
@julioct
Copy link
Contributor Author

julioct commented Jul 18, 2024

@julioct thanks for contributing this to the .NET Aspire project! Going through the review process to add not only hosting APIs but also component APIs is a big effort and we appreciate it.

Our plan is to ship this package as a preview in .NET Aspire 8.1 with a view for marking the package as stable in .NET Aspire 8.2.

We'll mention this new resource / component in the upcoming .NET Aspire 8.1 blog post, are you happy for us to link to your GitHub profile and give you a shout out?

Wow, that's awesome, thank you @mitchdenny @eerhardt and team!

Not looking for fame, but nothing wrong with the shout out.

Eagerly waiting for the 8.1 release!

"type" : "password",
"userLabel" : "My password",
"createdDate" : 1719876373873,
"secretData" : "{\"value\":\"4DfZJfojd4Ef08uoboJ2V+RMpF20yGEbw2pDouefjmcTmAVxE6wPkbLN03u/bAElCm4Y/ndJ9YruH3Q5pFuSLQ==\",\"salt\":\"E8I7GRLxZI2lzpKCI6h/bw==\",\"additionalParameters\":{}}",
Copy link
Member

Choose a reason for hiding this comment

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

This line is tripping credscan. @eerhardt @joperezr

Copy link
Member

Choose a reason for hiding this comment

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

@julioct - is this secret necessary? Can it be removed? I assume/hope it isn't actually a leaked credential that works for something real.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is likely the password of the testuser, prepared for the Playground example.

We can remove the user from there, but then when running the playground you will first have to register a new user on Keycloak before testing the example.

Would that be OK?

Choose a reason for hiding this comment

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

You can use environment variables in the imported realm configuration file: Using Environment Variables within the Realm Configuration Files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@radical @eerhardt I'm removing the test user and associated credentials in this PR

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @julioct. I've merged that change and will backport to release/8.1 to fix our builds.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-integrations Issues pertaining to Aspire Integrations packages community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.