Use BCL Base64Url implementation

[iamcarbon]

This PR removes our custom Base64Url class, and takes a dependency on the new BCL implementation. I'll update this next week with the RTM package.

NOTE: our current implementation was not strict and also decoded Base64 content. The new implementation throws when encountering content that is not properly encoded.

This PR fixes our test cases that were encoding Base64Url data as Base64.

[iamcarbon]

@abergs Ready for review

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 65.71429% with 12 lines in your changes missing coverage. Please review.

Project coverage is 76.33%. Comparing base (ce6374c) to head (7aebefd).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
Src/Fido2.Models/Converters/Base64UrlConverter.cs	68.96%	6 Missing and 3 partials ⚠️
...c/Fido2/Metadata/Fido2MetadataServiceRepository.cs	0.00%	2 Missing ⚠️
Src/Fido2/TokenBindingDto.cs	0.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #575      +/-   ##
==========================================
- Coverage   76.93%   76.33%   -0.60%     
==========================================
  Files         100       99       -1     
  Lines        2627     2590      -37     
  Branches      441      432       -9     
==========================================
- Hits         2021     1977      -44     
- Misses        498      503       +5     
- Partials      108      110       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[joegoldman2]

LGTM

[iamcarbon]

NOTE: this is not a valid id, which should a valid Base64Url encoded byte array.

[abergs]

NOTE: our current implementation was not strict and also decoded Base64 content. The new implementation throws when encountering content that is not properly encoded.

I'm a bit worried about the rough edges this will cause as it will undoubtedly cause in-production bugs for some payloads. I'm wondering if we could implement a fallback that causes less problems for consumers?

[iamcarbon]

I believe these "bugs" in production code should be fixed to be conformant with the WebAuthN specification. This is our last chance to fix this - and if we add a fallback, we may be allowing more clients to accidentally break compliance in the future.

Let me add some better error messaging in this PR for now. If you think we should continue to fallback to allow Base64 content to be decoded on Base64Url typed properties, we can add a fallback in our Base64UrlConverter.

We can also accept the break in 4.0, and add back the relaxed Base64 encoding behavior in 4.0.1, if this turns out to be a problem.

[abergs]

This is our last chance to fix this - and if we add a fallback, we may be allowing more clients to accidentally break compliance in the future.

We can also accept the break in 4.0, and add back the relaxed Base64 encoding behavior in 4.0.1, if this turns out to be a problem.

I agree with and appreciate your concern, but I'm worried about teams/devs adapting to all of our breaking changes only to end up with the surprise that some passkeys are broken in production - which ultimately causes dissatisfaction with passkeys for both devs and end users. The amount of changes between v3 and v4 might make a simple rollback hard for many teams.

I suggest we implement a temporary fallback configuration option for 4.0 rather than releasing it in a 4.0.1 patch, if possible?

Proposed approach:

• Add a simple config flag in 4.0 to enable the legacy relaxed encoding
• Teams can use this as an immediate fix if they encounter issues
• This buys them time to properly diagnose and fix the root cause
• We can remove this escape hatch in a future release once teams have migrated

This gives us the benefits of the breaking change while providing a safety net for teams who need it.

[iamcarbon]

Agreed on the flag:

How's this look:

Base64UrlConverter.AllowRelaxedDecoding = true;

as an escape hatch to fallback to the old decoding behavior. Encoding was always strict.

[abergs]

Guessing: Build is failing because of missing dotnet format

[iamcarbon]

@abergs OK, ready for a second review now!

[abergs]

Nicely done!