AddRejected method for ICertificateStore

mregen commented 3 months ago

Proposed changes

Add a light weight AddRejected certificate store method to ICertificateStore to ensure rejected cert history remains small and doesn't have a lot of overhead.
Number of history certificates is configurable using a new MaxRejectedCertificates entry.
Rejected certs are only saved to directory cert store, others do ignore the attempt
Fix the caching of CertificateValidator and flag some ÒpenStore methods deprecated which do not support the caching. Fixes across the board to support the methodology. Basically store.Close keeps the cache, store.Dispose clears the cache, so recommendation is to use var store = certStoreIdentifier.OpenStore(); try { ... }-- finally { store.Close }

Related Issues

Fixes #2693

Types of changes

What types of changes does your code introduce? Put an x in the boxes that apply. You can also fill these out after creating the PR.

[ ] Bugfix (non-breaking change which fixes an issue)
[x] Enhancement (non-breaking change which adds functionality)
[ ] Test enhancement (non-breaking change to increase test coverage)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected, requires version increase of Nuget packages)
[ ] Documentation Update (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

[x] I have read the CONTRIBUTING doc.
[x] I have signed the CLA.
[ ] I ran tests locally with my changes, all passed.
[ ] I fixed all failing tests in the CI pipelines.
[ ] I fixed all introduced issues with CodeQL and LGTM.
[ ] I have added tests that prove my fix is effective or that my feature works and increased code coverage.
[ ] I have added necessary documentation (if appropriate).
[ ] Any dependent changes have been merged and published in downstream modules.

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 82.57840% with 50 lines in your changes missing coverage. Please review.

Project coverage is 55.17%. Comparing base (6dcee7e) to head (aadd89a). Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...Security/Certificates/DirectoryCertificateStore.cs	84.72%	8 Missing and 3 partials :warning:
...raries/Opc.Ua.Configuration/ApplicationInstance.cs	0.00%	7 Missing :warning:
...ecurity/Certificates/CertificateStoreIdentifier.cs	72.00%	5 Missing and 2 partials :warning:
...aries/Opc.Ua.Gds.Server.Common/CertificateGroup.cs	87.17%	1 Missing and 4 partials :warning:
...ack/Opc.Ua.Core/Security/Certificates/X509Utils.cs	78.26%	2 Missing and 3 partials :warning:
...Core/Security/Certificates/CertificateValidator.cs	90.69%	3 Missing and 1 partial :warning:
...a.Configuration/ApplicationConfigurationBuilder.cs	33.33%	2 Missing :warning:
...a.Server/Configuration/ConfigurationNodeManager.cs	86.66%	0 Missing and 2 partials :warning:
Libraries/Opc.Ua.Server/Configuration/TrustList.cs	92.00%	0 Missing and 2 partials :warning:
...ore/Security/Certificates/CertificateIdentifier.cs	66.66%	2 Missing :warning:
... and 3 more

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #2720 +/- ## ========================================== + Coverage 55.12% 55.17% +0.04% ========================================== Files 349 349 Lines 65548 65683 +135 Branches 13398 13438 +40 ========================================== + Hits 36135 36238 +103 - Misses 25584 25608 +24 - Partials 3829 3837 +8 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ThomasNehring commented 3 months ago

does this make #2693 obsolete?

mregen commented 3 months ago

does this make #2693 obsolete?

yep, similar, but kind of trying to make it only work in a simple way for directory store. X509Store and others would not save rejected certs, as we already discussed.

mregen commented 2 months ago

Hi, sorry for the extra effort, but I reconsidered the previous approach to write to the rejected folder and fixed the caching instead to take advantage of it with an implementation that caches the rejected certificates. The root cause for the caching fix was the implementation only supported it for CertificateTrustList but not for the CertificateStoreIdentifier which is used in the CertificateValidator. To support the caching all OpenStore calls should use CertificateStoreIdentifier to which the cached store instance was moved. Some other OpenStore calls that do not support caching were marked deprecated and references fixed across the board. Only own certificates yet not use the caching.

OPCFoundation / UA-.NETStandard