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

[Bug] custom SignatureProvider breaks after upgrading .NET 8 #2418

Closed
superyyrrzz opened this issue Nov 24, 2023 · 7 comments
Closed

[Bug] custom SignatureProvider breaks after upgrading .NET 8 #2418

superyyrrzz opened this issue Nov 24, 2023 · 7 comments
Assignees
Labels
IdentityModel8x Future breaking issues/features for IdentityModel 8x P1 More important, prioritize highly Regression:6xto7x regressions when moving from 6x to 7x
Milestone

Comments

@superyyrrzz
Copy link

Repro

Our services met an outage due to a breaking change in .NET 8 (https://learn.microsoft.com/en-us/dotnet/core/compatibility/aspnet-core/8.0/securitytoken-events) related to SignatureProvider Change.

In our service's existing code in .NET 6, we inherite SignatureProvider and override public abstract bool Verify(byte[] input, byte[] signature).

However due to the breaking change in .NET 8, this method is not invoked. Instead, the new JsonWebTokenHandler calls the new method public virtual bool Verify(byte[] input, int inputOffset, int inputLength, byte[] signature, int signatureOffset, int signatureLength). This new method throws NotImplementedException as we are not aware to implement it.

Expected behavior

I propose to update the signature from virtual to abstract. Usually this is not friendly by forcing all the inheritance to implement a new method. However in this case, a compilation error to devs is more preferred over a runtime exception affecting our customers. If we can notice in advance when upgrading .NET 8, we may possiblely turn on UseSecurityTokenValidators to avoid breaking.

public abstract bool Verify(byte[] input, int inputOffset, int inputLength, byte[] signature, int signatureOffset, int signatureLength)

Actual behavior
The new virtual method is invoked by default in .NET 8's JsonWebToken, which throws NotImplementedException.

@yodasad
Copy link

yodasad commented Jan 15, 2024

The same issue with

public abstract byte[] Sign(byte[] input);

This method is no longer called by JsonWebTokenHandler targeting .NET 6 and higher, the method

internal virtual bool Sign(ReadOnlySpan<byte> data, Span<byte> destination, out int bytesWritten)

is called instead and there is no way one can override it since it is internal.

@KelDazan
Copy link

Experiencing same issue with Sign method.
I am using a custom CryptoProviderFactory with custom ICryptoProvider, that provides custom implementation of SignatureProvider.
Rolling back to Microsoft.IdentityModel.JsonWebTokens 7.1.2 helps, but i would like to have all security packages up to date, for obvious reasons.

@juunas11
Copy link

Ran into the same issue as well. Will have to keep my sample on 7.1.2 to allow it to function.

@brentschmaltz brentschmaltz added Regression:6xto7x regressions when moving from 6x to 7x and removed Needs attention Untriaged labels Feb 21, 2024
@brentschmaltz
Copy link
Member

brentschmaltz commented Feb 21, 2024

@juunas11 @KelDazan @yodasad @superyyrrzz i see the issue.
I marked it as a Regression6xto7x to get some attention.

We will have to open up that method.

@keegan-caruso keegan-caruso added P1 More important, prioritize highly IdentityModel8x Future breaking issues/features for IdentityModel 8x labels Feb 22, 2024
@keegan-caruso
Copy link
Contributor

keegan-caruso commented Feb 22, 2024

We can't make the method abstract in 7.x as it is breaking. We understand this is inconvenient / not a great experience as it is.

We need to make that change in 8.x for the reasons mentioned above.

We can at least open up the virtual

@jennyf19
Copy link
Collaborator

fixed in 7.4.0

@keegan-caruso
Copy link
Contributor

@jennyf19 we allowed for extensibility, I don't see an item with IM8 tag to make these methods abstract like they should be.

Do we want to reopen this or open a new issue to track?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IdentityModel8x Future breaking issues/features for IdentityModel 8x P1 More important, prioritize highly Regression:6xto7x regressions when moving from 6x to 7x
Projects
None yet
Development

No branches or pull requests

7 participants