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 certificate provider #34

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jetersen
Copy link
Contributor

@jetersen jetersen commented Oct 13, 2021

Thought it might be useful to have an certificate provider 😁

Some improvements would be to create our own enum for store name as the symbol naming is not that great.
🤷

Would be nice to have the option for HTTP client to disable certificate checks because of bad certificate chain as seen with the recent DST Root CA X3 cert expiration 😅
Done

I could foresee it get somewhat complicated with password and etc but should be doable, this was an initial attempt at making sure root certificate could get patched.
Done

We have some systems that do not have general internet access.

Could potentially also be a separate nuget package if you feel it adds too much.

Haven't gotten around to tests, would like to pick your brain on how we could test the functionality.
Perhaps creating an interface for the certificate store so it could be mocked? As it often requires administrator.


public CertificateState Ensure { get; set; }

public string Thumbprint { get; set; } = string.Empty;
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure if I understand completely, but shouldn't this be nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Properly :)

@patriksvensson
Copy link
Owner

@jetersen Sorry for taking so long to get around to this.

@jetersen
Copy link
Contributor Author

@jetersen Sorry for taking so long to get around to this.

@patriksvensson No worries at all.

@jetersen jetersen force-pushed the add/certificateProvider branch from b43fc29 to b9d2a00 Compare November 9, 2024 08:28
@jetersen
Copy link
Contributor Author

jetersen commented Nov 9, 2024

@patriksvensson To add further testing would require changes to the ci.yaml to test on Windows as well 🤔

@patriksvensson
Copy link
Owner

@patriksvensson To add further testing would require changes to the ci.yaml to test on Windows as well 🤔

It's OK. Do you feel happy with the PR? In that case I can merge it now and we can fix tests later.

@jetersen
Copy link
Contributor Author

Ya the PR implementation has the features I had had in mind. Should allow for fetching certificates for multiple use cases. So if you're happy with the code and naming. Feel free to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants