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

Replacing Yubico.DotnetPolyFills with PolySharp. Bumping LanguageVersion to C#12. #132

Draft
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

DennisDyallo
Copy link
Collaborator

@DennisDyallo DennisDyallo commented Jul 30, 2024

Description

This update removes the need for the Yubico.DotnetPolyfills-project. This project was adding a couple of methods, such as CryptographicOperations.ZeroMemory and the Index operator. Now, since adding Polysharp, this is no longer needed as Polysharp generates polyfills for newer C# language features and APIs at compile-time. This also allows us to use C# language version 12 instead of 7.3.

It's true that bringing in a dependency into any library should not be taken in lightly. This suggestion has been with the team for some time, and while we haven't committed to it yet, we want to explore the possibility of making such a change. We do believe measures can be taken to ensure stability and expected behavior. Rest assured, although the third party dependency in question appears to be well regarded, useful and stable, it won't be added without going through a due dilligence process.

To you and anyone else reading this, please feel free to add your comments or any alternatives to move the SDK forward in this regard.

Fixes: YESDK1316

Type of change

How has this been tested?

Unit tests pass and builds pass.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have run dotnet format to format my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@DennisDyallo DennisDyallo changed the title Replaceing Yubico.DotnetPolyFills with PolySharp Replacing Yubico.DotnetPolyFills with PolySharp Jul 30, 2024
Copy link

github-actions bot commented Jul 30, 2024

Test Results: Windows

    2 files      2 suites   5s ⏱️
3 700 tests 3 700 ✅ 0 💤 0 ❌
3 702 runs  3 702 ✅ 0 💤 0 ❌

Results for commit 85a888e.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jul 30, 2024

Test Results: Ubuntu

    2 files      2 suites   9s ⏱️
3 692 tests 3 692 ✅ 0 💤 0 ❌
3 694 runs  3 694 ✅ 0 💤 0 ❌

Results for commit 85a888e.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jul 30, 2024

Test Results: MacOS

    2 files      2 suites   3s ⏱️
3 692 tests 3 692 ✅ 0 💤 0 ❌
3 694 runs  3 694 ✅ 0 💤 0 ❌

Results for commit 85a888e.

♻️ This comment has been updated with latest results.

Copy link

Code Coverage

Package Line Rate Branch Rate Complexity Health
Yubico.Core 42% 32% 4278
Yubico.YubiKey 50% 47% 19122
Summary 48% (32015 / 66351) 44% (8160 / 18540) 23400

Minimum allowed line rate is 40%

@DennisDyallo DennisDyallo changed the title Replacing Yubico.DotnetPolyFills with PolySharp Replacing Yubico.DotnetPolyFills with PolySharp. Bumping LanguageVersion to C#12. Jul 30, 2024
@iamcarbon
Copy link

This introduces an untrusted third party dependency into a security critical library. It would be better to selectively port the individual Polyfills into core, then introduce a new dependency (and extend the chain of trust).

@DennisDyallo
Copy link
Collaborator Author

DennisDyallo commented Aug 27, 2024

This introduces an untrusted third party dependency into a security critical library. It would be better to selectively port the individual Polyfills into core, then introduce a new dependency (and extend the chain of trust).

Hi @iamcarbon and thanks for your raised concern. It's true that bringing in a dependency into any library should not be taken in lightly. This suggestion has been with the team for some time, and while we haven't committed to it yet, we want to explore the possibility of making such a change. We do believe measures can be taken to ensure stability and expected behavior. Rest assured, although the third party dependency in question appears to be well regarded, useful and stable, it won't be added without going through a due dilligence process.

To you and anyone else reading this, please feel free to add your comments or any alternatives to move the SDK forward in this regard.

@dainnilsson @GregDomzalski FYI

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

Successfully merging this pull request may close these issues.

2 participants