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

Make CertificateConfig linker friendly #25485

Closed
wants to merge 2 commits into from

Conversation

benaadams
Copy link
Member

@benaadams benaadams commented Sep 1, 2020

Currently can't use Certificates from config files if you link ASP.NET as all the setters for the config section are removed.

This directly references the properties rather than nebulously reflecting them via object so the linker will preserve them as they are directly used.

Resolves #25482

@ghost ghost added the area-servers label Sep 1, 2020
@benaadams benaadams closed this Sep 1, 2020
@benaadams benaadams reopened this Sep 1, 2020
@benaadams
Copy link
Member Author

@Tratcher any way to get this in for 5.0 (area-owners.md looks out of date so not sure who to ping); as certs are kinda common as is setting them via config?

Raised issue for the .Bind method in runtime dotnet/runtime#41658 however doesn't look like its an easy fix dotnet/runtime#40551

@@ -353,7 +353,18 @@ internal class CertificateConfig
public CertificateConfig(IConfigurationSection configSection)
{
ConfigSection = configSection;
ConfigSection.Bind(this);

Path = configSection[nameof(Path)];
Copy link
Member

Choose a reason for hiding this comment

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

Comment that this is to preserve linkability.

@Tratcher
Copy link
Member

Tratcher commented Sep 1, 2020

It's plausible if you rebase this on release/5.0.

Is this the only bind in Kestrel config? I know bind is used extensively for other options types. Kestrel is already the odd one out for doing most of its own bindings.

Put another way, doesn't this linking issue with Bind cripple most Options types? Does fixing Kestrel really help make your app linkable if none of the other options types work?

@benaadams
Copy link
Member Author

Is this the only bind in Kestrel config? I know bind is used extensively for other options types.

This particular Bind method is only used here and in IdentityServer.Configuration.KeyDefinition which I added another PR for #25487

image

https://source.dot.net/#Microsoft.Extensions.Configuration.Binder/ConfigurationBinder.cs,b3d80335ffffa242,references

@Tratcher
Copy link
Member

Tratcher commented Sep 1, 2020

Hmm, I guess the other bind calls I'm used to seeing are in the templates.

.AddAzureAD(options => Configuration.Bind("AzureAd", options));

Note these are moving in 5.0:

.AddMicrosoftIdentityWebApp(Configuration.GetSection("AzureAd"))

To:
https://github.com/AzureAD/microsoft-identity-web/blob/eb8af9c1bd6dd0014420dd6d06fd4083bda1f8b9/src/Microsoft.Identity.Web/WebApiExtensions/MicrosoftIdentityWebApiAuthenticationBuilderExtensions.cs#L89

@benaadams
Copy link
Member Author

Opened 5.0 targeted version #25493

Hmm, I guess the other bind calls I'm used to seeing are in the templates

Probably would need runtime fix in Configuration for that to recognise preserve the properties for the object's type that is passed in

@Tratcher
Copy link
Member

Tratcher commented Sep 1, 2020

If #25493 gets merged then we can close this PR, 5.0 auto merges forward to master.

@benaadams
Copy link
Member Author

Superseded by #25515

@benaadams benaadams closed this Sep 2, 2020
@benaadams benaadams deleted the Linkable-CertConfig branch September 2, 2020 16:25
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linker removes all setters for CertificateConfig in Kestrel
3 participants