Skip to content

Conversation

@amcasey
Copy link
Member

@amcasey amcasey commented May 3, 2023

Introduce ListenOptions.GetServerCertificate

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

An API to retrieve the cert for an endpoint

Description

Introduce ListenOptions.GetServerCertificate as a way to retrieve the server certificate eagerly (assuming the configuration has been loaded), rather than having to wait until bind-time.

Fixes #28120
Unblocks #45801

...as a way to retrieve the server certificate eagerly (assuming the configuration has been loaded), rather than having to wait until bind-time.

Fixes dotnet#28120
@amcasey
Copy link
Member Author

amcasey commented May 3, 2023

Waiting for API approval (and PR approval, obviously).

Copy link
Member

@mitchdenny mitchdenny left a comment

Choose a reason for hiding this comment

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

LGTM

@amcasey amcasey added the blocked The work on this issue is blocked due to some dependency label May 3, 2023
@JamesNK
Copy link
Member

JamesNK commented May 3, 2023

Is there an API review for this?

@amcasey
Copy link
Member Author

amcasey commented May 3, 2023

Is there an API review for this?

#48055

{
ran1 = true;
var serverCertificate = listenOptions.GetServerCertificate();
Assert.Equal(serverCertificate.SerialNumber, certificate.SerialNumber);
Copy link
Member

Choose a reason for hiding this comment

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

Some of the asserts are in the wrong order. It's generally Assert.Something(expected, actual)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, but I was following the pattern of the nearby tests.

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

I think this needs a different design. I've left notes on the API review.

@amcasey
Copy link
Member Author

amcasey commented May 8, 2023

We're going to try to fix things without a new API.

@amcasey amcasey closed this May 8, 2023
@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 blocked The work on this issue is blocked due to some dependency

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Preload default Kestrel config before running Kestrel's configureOptions callback

5 participants