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

Updates for getIssuer(s) in the Certificate Validator class #1632

Merged

Conversation

ThomasNehring
Copy link
Contributor

  • Adapted getIssuer and getIssuers so that all findings get aggregated for later processing in the validation methods.
  • Ensured that other methods calling getIssuer(s) will experience unchanged behavior
  • Adapted unit tests

…ended and adapted some Unit Tests to take into account Issuer Certificate Revocation and mutliple errors when revocation lists are missing.
@lgtm-com
Copy link

lgtm-com bot commented Dec 10, 2021

This pull request introduces 2 alerts when merging 5588555 into 58bac5a - view on LGTM.com

new alerts:

  • 1 for Container contents are never accessed
  • 1 for Useless assignment to local variable

@mregen mregen self-requested a review December 11, 2021 10:22
@mregen mregen added this to the 1.4.368 milestone Dec 11, 2021
@mregen mregen requested a review from mrsuciu December 17, 2021 10:09
@mregen
Copy link
Contributor

mregen commented Dec 29, 2021

@ThomasNehring please merge master again, I can't push the merged version becuase some github actions were updated..

here is the final version you could merge: https://github.com/mregen/UA-.NetStandardLibrary/tree/local/get_issuer_noexception

Copy link
Contributor

@mregen mregen left a comment

Choose a reason for hiding this comment

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

@mregen
Copy link
Contributor

mregen commented Dec 31, 2021

@ThomasNehring testing CTT, found a few unrelated issues, so regarding these improvements looks all good.
However, test 02 causes the specific tests to fail with BadCertificateInvalid instead of BadIssuerRevocationUnknown.
The root cause is that X509Chain doesn't accept the root cert created in the CTT, so the chain is incomplete.
Now investigating why a cert chain used in CTT is not accepted in windows, It may have to do with bad serial number or SKI being invalid.

@mregen
Copy link
Contributor

mregen commented Dec 31, 2021

CTT passes once proper root CA cert is created by CTT --> https://mantis.opcfoundation.org/view.php?id=7509

@lgtm-com
Copy link

lgtm-com bot commented Jan 11, 2022

This pull request introduces 2 alerts and fixes 1 when merging b4c1467 into 9188dcf - view on LGTM.com

new alerts:

  • 1 for Container contents are never accessed
  • 1 for Useless assignment to local variable

fixed alerts:

  • 1 for Missing Dispose call on local IDisposable

@lgtm-com
Copy link

lgtm-com bot commented Jan 11, 2022

This pull request introduces 2 alerts and fixes 1 when merging 6001d08 into 9188dcf - view on LGTM.com

new alerts:

  • 1 for Container contents are never accessed
  • 1 for Useless assignment to local variable

fixed alerts:

  • 1 for Missing Dispose call on local IDisposable

Copy link
Contributor

@mregen mregen left a comment

Choose a reason for hiding this comment

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

great work 🚀 @ThomasNehring. This looks now good - during CTT testing I ran into some other issues that may have been unrelated but are now creeping in with this PR. But most were running the cert tests...

@lgtm-com
Copy link

lgtm-com bot commented Jan 11, 2022

This pull request introduces 2 alerts and fixes 1 when merging b57588d into b41fb36 - view on LGTM.com

new alerts:

  • 1 for Container contents are never accessed
  • 1 for Useless assignment to local variable

fixed alerts:

  • 1 for Missing Dispose call on local IDisposable

@codecov
Copy link

codecov bot commented Jan 11, 2022

Codecov Report

Merging #1632 (6747b47) into master (6747b47) will not change coverage.
The diff coverage is n/a.

❗ Current head 6747b47 differs from pull request most recent head 7890cf6. Consider uploading reports for the commit 7890cf6 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1632   +/-   ##
=======================================
  Coverage   53.88%   53.88%           
=======================================
  Files         319      319           
  Lines       57776    57776           
=======================================
  Hits        31134    31134           
  Misses      26642    26642           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6747b47...7890cf6. Read the comment docs.

@mregen
Copy link
Contributor

mregen commented Jan 12, 2022

Hi @ThomasNehring , the code works as expected on windows/macOS, but apparently there is still an issue with linux cert validation. I'm debugging it but to no success yet. Please stay tuned...

@lgtm-com
Copy link

lgtm-com bot commented Jan 12, 2022

This pull request introduces 2 alerts and fixes 1 when merging 0f3d6e4 into fa6099b - view on LGTM.com

new alerts:

  • 1 for Container contents are never accessed
  • 1 for Useless assignment to local variable

fixed alerts:

  • 1 for Missing Dispose call on local IDisposable

@mregen
Copy link
Contributor

mregen commented Jan 14, 2022

Hi @ThomasNehring , #1665 fixes the linux cert issue, once merged, should be ready to go...

@mregen
Copy link
Contributor

mregen commented Jan 14, 2022

a couple of CTT fixes ended up here which are unrelated -- will be removed

@lgtm-com
Copy link

lgtm-com bot commented Jan 14, 2022

This pull request introduces 5 alerts and fixes 1 when merging d295539 into 6747b47 - view on LGTM.com

new alerts:

  • 3 for Constant condition
  • 1 for Container contents are never accessed
  • 1 for Useless assignment to local variable

fixed alerts:

  • 1 for Missing Dispose call on local IDisposable

@lgtm-com
Copy link

lgtm-com bot commented Jan 14, 2022

This pull request introduces 4 alerts and fixes 1 when merging 7890cf6 into 6747b47 - view on LGTM.com

new alerts:

  • 3 for Constant condition
  • 1 for Container contents are never accessed

fixed alerts:

  • 1 for Missing Dispose call on local IDisposable

@mregen
Copy link
Contributor

mregen commented Jan 14, 2022

almost ready, one more CTT pass then 🚀

@mregen mregen merged commit dd6f4a4 into OPCFoundation:master Jan 14, 2022
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.

A certificate which does not present a revocation list endpoint should not be refused for this reason.
3 participants