313 client assertion pem #42

leknesh · 2025-11-26T07:29:11Z

Added functionality for retrieving private key from PEM file in certificate store, and related tests. Integration tests require access to cert store.

I was a bit unsure how to hook up the functionality when it comes to DI, ended up with adding optional extension method that must be called when pem is relevant. Not sure if this is the best way, there's probably more elegant ways of connecting the dots here.

Sample code will be added in a separate PR

RelatesTo #313

src/Fhi.Authentication.Extensions/ClientCredentials/ClientCredentialsAssertionService.cs

src/Fhi.Authentication.Extensions/ServiceCollectionExtensions.cs

… certificate.

…' into feature/313-client-assertion-PEM # Conflicts: # src/Fhi.Authentication.Extensions/ClientCredentials/ClientAssertionOptions.cs # src/Fhi.Authentication.Extensions/ClientCredentials/ClientCredentialsAssertionService.cs # src/Fhi.Authentication.Extensions/Tokens/ICertificateKeyHandler.cs

Kattemat

Synes det ser bra ut jeg :)

leknesh · 2025-12-05T15:19:06Z

Gikk over til et factory pattern i denne committen: c26d5a0

# Conflicts: # tests/Fhi.Authentication.Extensions.UnitTests/Tokens/ClientAssertionTokenHandlerTests.cs

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

leknesh · 2025-12-17T09:40:05Z

New attempt at making a nice, expandable architecture.
First version
Second version
Current/third version

If none of these are acceptable I suggest someone rewrites this to their liking so I can learn from a final version, I won't be making yet another attempt at this point.

Copilot

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated 22 comments.

Comments suppressed due to low confidence (1)

tests/Fhi.Auth.IntegrationTests/Setup/TestCertificateBuilder.cs:46

This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.
This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.

            var rsa = RSA.Create(_keySize);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tests/Fhi.Authentication.Extensions.UnitTests/Tokens/CertificateKeyHandler.UnitTests.cs

src/Fhi.Authentication.Extensions/Tokens/IPrivateKeyHandler.cs

src/Fhi.Authentication.Extensions/ClientCredentials/CertificateSecretStore.cs

samples/Fhi.Samples.M2M.HelseID/Program.cs

tests/Fhi.Auth.IntegrationTests/Setup/TestCertificateBuilder.cs

src/Fhi.Authentication.Extensions/ClientCredentials/ClientCredentialsExtensions.cs

tests/Fhi.Auth.IntegrationTests/Setup/TestCertificateBuilder.cs

src/Fhi.Authentication.Extensions/ClientCredentials/ClientCredentialsAssertionService.cs

src/Fhi.Authentication.Extensions/Tokens/ICertificateProvider.cs

Copilot

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated 27 comments.

Comments suppressed due to low confidence (2)

tests/Fhi.Auth.IntegrationTests/Setup/TestCertificateBuilder.cs:57

This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.

                var persistable = new X509Certificate2(pfx, (string?)null, X509KeyStorageFlags.PersistKeySet | X509KeyStorageFlags.Exportable);

tests/Fhi.Auth.IntegrationTests/Setup/TestCertificateBuilder.cs:63

This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.

            var publicOnly = X509Certificate2.CreateFromPem("-----BEGIN CERTIFICATE-----\n" + Convert.ToBase64String(der, Base64FormattingOptions.InsertLineBreaks) + "\n-----END CERTIFICATE-----\n");

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-17T11:46:52Z

tests/Fhi.Auth.IntegrationTests/CertificateKeyHandlerTests.cs

+{
+    [TestFixture]
+    [Explicit ("Requires Windows and Local Machine Certificate Store access")]
+    public class CertificateKeyHandlerTests


The test class is named "CertificateKeyHandlerTests" but the field and setup reference "IPrivateKeyHandler". For consistency with the interface/class being tested, consider renaming to "PrivateKeyHandlerTests".

Suggested change

public class CertificateKeyHandlerTests

public class PrivateKeyHandlerTests

Copilot · 2025-12-17T11:46:52Z

tests/Fhi.Auth.IntegrationTests/CertificateKeyHandlerTests.cs

+
+            using (Assert.EnterMultipleScope())
+            {
+                Assert.That(jwkString, Is.Not.Null.Or.Empty);


The assertion uses "Is.Not.Null.Or.Empty" which is not valid NUnit syntax. It should be "Is.Not.Null.And.Not.Empty" to properly combine the null check and empty string check.

Suggested change

Assert.That(jwkString, Is.Not.Null.Or.Empty);

Assert.That(jwkString, Is.Not.Null.And.Not.Empty);

Copilot · 2025-12-17T11:46:52Z

src/Fhi.Authentication.Extensions/ClientCredentials/CertificateSecretManager.cs

+    private bool Load(X509Certificate2 certificate)
+    {
+        ArgumentNullException.ThrowIfNull(certificate);
+
+        // Default: only load certificates with private keys that are not expired
+        if (!certificate.HasPrivateKey)
+        {
+            Logger.LogDebug("Skipping certificate {Thumbprint} - no private key", certificate.Thumbprint);
+            return false;
+        }
+
+        if (certificate.NotAfter < DateTime.Now)
+        {
+            Logger.LogWarning("Skipping expired certificate {Thumbprint}, expired on {ExpiryDate}",
+                certificate.Thumbprint, certificate.NotAfter);
+            return false;
+        }
+
+        if (certificate.NotBefore > DateTime.Now)
+        {
+            Logger.LogWarning("Skipping certificate {Thumbprint}, not valid until {ValidFrom}",
+                certificate.Thumbprint, certificate.NotBefore);
+            return false;
+        }
+
+        return true;
+    }


The Load method is private but follows a naming convention that suggests it should be virtual and overridable (similar to the pattern mentioned in the class documentation about Azure's KeyVaultSecretManager). Consider making this method virtual and protected to allow derived classes to customize certificate filtering logic.

Copilot · 2025-12-17T11:46:53Z

samples/Fhi.Samples.M2M.HelseID/Program.cs

+
+        if (!string.IsNullOrEmpty(certificateThumbprint))
+        {
+            services.AddSingleton<CertificateSecretManager>();


The CertificateSecretManager is registered as a singleton without checking if it's actually needed. When using FileSecretStore (lines 155-161), the CertificateSecretManager is still registered but never used. Consider moving the CertificateSecretManager registration inside the certificate-specific block (line 140) to avoid unnecessary service registrations.

Copilot · 2025-12-17T11:46:53Z

samples/Fhi.Samples.M2M.HelseID/Program.cs

-        var app = builder.Build();

+        var app = builder.Build();
        await app.StartAsync();


The app.StartAsync() is called but the subsequent code to actually use the service (commented out lines for getting health records) is missing. This makes the sample incomplete and non-functional. Either restore the functional code or add a comment explaining that this is intentionally incomplete pending the separate PR mentioned in the description.

Suggested change

await app.StartAsync();

await app.StartAsync();

// The host is now started and HealthRecordService can be resolved from the DI container.

// This sample intentionally stops after startup to focus on configuration of client

// credentials authentication. Example code that calls the protected API using

// HealthRecordService is provided separately.

Copilot · 2025-12-17T11:46:59Z

samples/Fhi.Samples.M2M.HelseID/Program.cs

+                    sp.GetRequiredService<IPrivateKeyHandler>(),
+                    sp.GetRequiredService<ILogger<CertificateSecretStore>>(),
+                    sp.GetRequiredService<CertificateSecretManager>()); 
+            });


The AddCertificateStoreKeyHandler extension method is not called in the ConfigureWithSecretStore method, but IPrivateKeyHandler is retrieved from the service provider. This will cause a runtime exception when GetRequiredService<IPrivateKeyHandler> is called because IPrivateKeyHandler was never registered. Add services.AddCertificateStoreKeyHandler() before the ISecretStore registration.

Copilot · 2025-12-17T11:46:59Z

src/Fhi.Authentication.Extensions/ClientCredentials/ClientCredentialsExtensions.cs

+            if (string.IsNullOrEmpty(optionName))
+                throw new ArgumentException("Option name cannot be null or empty", nameof(optionName));
+


This validation throws an ArgumentException for an empty optionName, but this check is inconsistent with the other overloads which don't perform this validation. Either add this validation to all overloads for consistency, or remove it from this one and let the framework handle the validation.

Suggested change

if (string.IsNullOrEmpty(optionName))

throw new ArgumentException("Option name cannot be null or empty", nameof(optionName));

Copilot · 2025-12-17T11:47:00Z

src/Fhi.Authentication.Extensions/ClientCredentials/ClientCredentialsExtensions.cs

+            services.TryAddTransient<ICertificateProvider>(_ => 
+                new StoreCertificateProvider(certificate.StoreLocation));
+            services.TryAddTransient<IPrivateKeyHandler, PrivateKeyHandler>();


The code uses TryAddTransient for ICertificateProvider and IPrivateKeyHandler, which means if they're already registered, these registrations will be ignored. However, a specific StoreLocation is being configured based on certificate.StoreLocation. If a different registration already exists with a different StoreLocation, this could lead to unexpected behavior. Consider either using AddTransient (which would throw on duplicate) or documenting this behavior clearly.

Suggested change

services.TryAddTransient<ICertificateProvider>(_ =>

new StoreCertificateProvider(certificate.StoreLocation));

services.TryAddTransient<IPrivateKeyHandler, PrivateKeyHandler>();

services.AddTransient<ICertificateProvider>(_ =>

new StoreCertificateProvider(certificate.StoreLocation));

services.AddTransient<IPrivateKeyHandler, PrivateKeyHandler>();

Copilot · 2025-12-17T11:47:00Z

src/Fhi.Authentication.Extensions/ServiceCollectionExtensions.cs

+            this IServiceCollection services,
+            CertificateStoreLocation storeLocation = CertificateStoreLocation.CurrentUser)
+        {
+            services.AddTransient<ICertificateProvider>(_ => new StoreCertificateProvider(storeLocation));


The AddCertificateStoreKeyHandler method registers both ICertificateProvider and IPrivateKeyHandler as Transient, which means a new StoreCertificateProvider instance is created for each request. Since StoreCertificateProvider is stateless and accesses the certificate store each time, consider using Singleton lifetime instead to reduce allocations and improve performance.

Suggested change

services.AddTransient<ICertificateProvider>(_ => new StoreCertificateProvider(storeLocation));

services.AddSingleton<ICertificateProvider>(_ => new StoreCertificateProvider(storeLocation));

Copilot · 2025-12-17T11:47:01Z

tests/Fhi.Authentication.Extensions.UnitTests/Tokens/CertificateKeyHandler.UnitTests.cs

@@ -0,0 +1,171 @@
+using System.Security.Cryptography;


The file name "CertificateKeyHandler.UnitTests.cs" doesn't follow the conventional test file naming pattern. It should be "PrivateKeyHandlerTests.cs" or "PrivateKeyHandler.UnitTests.cs" to match the class being tested (PrivateKeyHandler) and be consistent with other test files in the project.

bvst · 2025-12-19T11:44:09Z

src/Fhi.Authentication.Extensions/Tokens/IPrivateKeyHandler.cs

+        {
+            if (certificate is null) throw new ArgumentNullException(nameof(certificate));
+
+            if (certificate.NotAfter < DateTime.Now)


Should probably use DateTime.UtcNow to avoid time zone issues

leknesh added 4 commits November 25, 2025 07:42

Added support for secret in pem format for client assertion

c8b884a

Add CertificateKeyHandler and related tests for private key retrieval

5cef4cd

Removed pseudocode comments

011df63

Cleanup of test class

6a5df36

leknesh requested review from Kattemat and jennyhougen November 26, 2025 07:29

leknesh self-assigned this Nov 26, 2025

leknesh commented Nov 26, 2025

View reviewed changes

src/Fhi.Authentication.Extensions/ClientCredentials/ClientCredentialsAssertionService.cs Outdated Show resolved Hide resolved

leknesh commented Nov 26, 2025

View reviewed changes

src/Fhi.Authentication.Extensions/ServiceCollectionExtensions.cs Outdated Show resolved Hide resolved

Jennys kluss

03df9ed

leknesh changed the title ~~Feature/313 client assertion pem~~ WIP: 313 client assertion pem Dec 1, 2025

leknesh added 3 commits December 5, 2025 09:29

Improved structure for certificate key handler, added support for PEM…

94b04d9

… certificate.

Fixed failing tests

8995886

leknesh changed the title ~~WIP: 313 client assertion pem~~ 313 client assertion pem Dec 5, 2025

Kattemat reviewed Dec 5, 2025

View reviewed changes

leknesh added 3 commits December 5, 2025 11:59

Added certificate auth to HelseID sample

8d5ea6f

Implemented factory pattern for secret store configuration

c26d5a0

Removed unnecessary comments

051e59a

leknesh added 3 commits December 5, 2025 16:26

Fixed failing test

f96657a

Rewrote certificate secret retrieval architecture yet again

d620247

Cleaned up some comments, committed missing files

72b7f8e

leknesh requested a review from Copilot December 17, 2025 08:59

Copilot started reviewing on behalf of leknesh December 17, 2025 09:00 View session

Merge branch 'main' into feature/313-client-assertion-PEM

9d0cf47

# Conflicts: # tests/Fhi.Authentication.Extensions.UnitTests/Tokens/ClientAssertionTokenHandlerTests.cs

Copilot AI reviewed Dec 17, 2025

View reviewed changes

Fixed errors after project sync

9653165

leknesh requested a review from Copilot December 17, 2025 09:36

Copilot started reviewing on behalf of leknesh December 17, 2025 09:36 View session

Copilot AI reviewed Dec 17, 2025

View reviewed changes

leknesh added 3 commits December 17, 2025 12:14

Minor changes after Copilot review

cbfba00

Improved cert dispose

74c88b7

Improved cert disposal

ca4b3e2

leknesh requested a review from Copilot December 17, 2025 11:31

Copilot started reviewing on behalf of leknesh December 17, 2025 11:31 View session

Copilot AI reviewed Dec 17, 2025

View reviewed changes

bvst reviewed Dec 19, 2025

View reviewed changes

bvst added 2 commits December 19, 2025 14:16

valides nested options objects

43a9d32

checks certificate dates with UTC dates

0974511

	public class CertificateKeyHandlerTests
	public class PrivateKeyHandlerTests

	Assert.That(jwkString, Is.Not.Null.Or.Empty);
	Assert.That(jwkString, Is.Not.Null.And.Not.Empty);

-        await app.StartAsync();
+        await app.StartAsync();
+        // The host is now started and HealthRecordService can be resolved from the DI container.
+        // This sample intentionally stops after startup to focus on configuration of client
+        // credentials authentication. Example code that calls the protected API using
+        // HealthRecordService is provided separately.

		if (string.IsNullOrEmpty(optionName))
		throw new ArgumentException("Option name cannot be null or empty", nameof(optionName));

	services.AddTransient<ICertificateProvider>(_ => new StoreCertificateProvider(storeLocation));
	services.AddSingleton<ICertificateProvider>(_ => new StoreCertificateProvider(storeLocation));

313 client assertion pem #42

Are you sure you want to change the base?

313 client assertion pem #42

Uh oh!

Conversation

leknesh commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kattemat left a comment

Choose a reason for hiding this comment

Uh oh!

leknesh commented Dec 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

leknesh commented Dec 17, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

bvst Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

leknesh commented Nov 26, 2025 •

edited

Loading