Restore .Valid() Functionality Somehow

[jcburley]

Please restore the .Valid() method; export the new validation functions so we can reimplement it ourselves; or, (and this is currently my favored solution) support a WithNoSignatureValidation option for the .Parse* functions.

The goal here is to be able to validate just the (standard, and perhaps optionally any new custom) claims, while ignoring the signature, while parsing a jwt. (I.e. assume the context is one in which signature makes no sense, such as unit-test code; or is wasteful/difficult, such as in code within a "pipeline" where the signature validation has already occurred.)

This will allow us to port to v5.0.0 (from v4.5.0) without having to copy/duplicate validation code, use reflection, somehow disable signature checking (which I've been struggling with), fork our own version, etc.

(We've been blocked on this upgrade for months while dealing with other more-pressing issues, and hoped to knock this out in a day or so. That hasn't worked out due to this one problem.)

Background: We have a wrapper package that a lot of our code uses, and while I can't say I fully understand why the wrapping function that calls .Valid() is used everywhere, at least beyond test code, it does seem to have been working previously. We have a separate wrapping function that calls .ParseWithClaims(), which is the more-normal code path.

Would a PR be welcome? If so, which approach would you prefer?

[oxisto]

Please restore the .Valid() method, or export the new validation functions so we can reimplement it ourselves.

Could you maybe expand on your motivation to do so? Why would you re-implement the validation on your own? If you want to validate additional claims on top of the normal signature and mandatory claim validation, you can use the Validate function.

jwt/example_test.go

Lines 122 to 138 in 0cb4fa1

	type MyCustomClaims struct {
	Foo string `json:"foo"`
	jwt.RegisteredClaims
	}
	// Ensure we implement [jwt.ClaimsValidator] at compile time so we know our custom Validate method is used.
	var _ jwt.ClaimsValidator = (*MyCustomClaims)(nil)
	// Validate can be used to execute additional application-specific claims
	// validation.
	func (m MyCustomClaims) Validate() error {
	if m.Foo != "bar" {
	return errors.New("must be foobar")
	}
	return nil
	}

This will allow us to port to v5.0.0 (from v4.5.0) without having to copy/duplicate validation code, use reflection, somehow disable signature checking (which I've been struggling with), fork our own version, etc.

If you really want to disable signature checking (which is really not a good idea), you can use the function ParseUnverified, it only does base64 decoding and JSON unmarshalling of the token, without any signature verification and claims validation.

If you want to disable claims validation (e.g. checking for expiry, etc.), you can use the parser option WithoutClaimsValidation.

(We've been blocked on this upgrade for months while dealing with other more-pressing issues, and hoped to knock this out in a day or so. That hasn't worked out due to this one problem.)

Background: We have a wrapper package that a lot of our code uses, and while I can't say I fully understand why the wrapping function that calls .Valid() is used everywhere, at least beyond test code, it does seem to have been working previously. We have a separate wrapping function that calls .ParseWithClaims(), which is the more-normal code path.

If you just want to see whether the token is "valid" (after using ParseWithClaims), you can just use the property Valid instead of calling the function. But again, this depends on the use-case that you are trying to achieve, which is still not 100 % clear to me.

Would a PR be welcome? If so, which approach would you prefer? Beyond the two suggested in the title, I suppose another approach might be to add support for a "do not check signature" option.

Before discussing any PR, I would want to make sure that the problem cannot be already solved by the existing code base.

[jcburley]

The original .Valid() function validated the vanilla ("standard"?) claims, without doing signature verification. "Too much" of our code base depends on that either directly (by directly importing this package) or indirectly (by calling our wrapper package's function, which wraps .Valid()).

So, what we seem to need here is a way to validate just the standard claims — not the signature. (I suspect we could make good use of the new feature to validate custom claims, but that's out of scope of my current work.)

It would probably take us days or even weeks to go through the entire code base to find, understand, and especially change each such usage (of .Valid() or of our wrapper function for it) — even if, conceptually, the effort might be "worth it" from the perspective of having pristine code — and today's my last day working on this upgrade (of our wrapper package to v5) for awhile.

As I've gotten further in trying to bushwhack my way through changing our wrapper function (the one in our wrapper package that currently calls .Valid()) to call .ParseWithClaims() in a way that tries to ignore the inevitable invalid-signature failure, which unfortunately runs afoul of the fact that such a failure results in no further validation of the claims — so code we have that depends on such validation would no longer work — I've noticed we have code such as

• Unit and integration tests
• Token managers
• Interceptors

that are using this functionality. (To be clear, we have code that uses our other wrapper, which does do the signature verification — so it doesn't appear, to me, to be the case that the authors of this code, most/all of whom have since moved on, didn't understand the differences here. But I can't rule that out without further study which, again, is unlikely to be "funded" anytime soon, if ever.)

There's a very good argument for not calling the function .Valid(), by the way, which I appreciate: it's rather vaguely named and thus implies full validation, which is not the case. We should be able to easily handle renaming it to .ValidateClaims() or even .ValidateClaimsButNotTheSignature(), at least internally.

But it's the functionality we need, one way or another, given the timeframe and realities of our resource allocations.

Personally, though I haven't yet tried this myself, adding a WithNoSignatureValidation option (function) in the right place(s) seems the lowest-touch solution, and is likely what I'll try, or recommend someone on our team try, if we're indeed going to do the upgrade.

[jcburley]

As alluded to at the end of my previous comment, another tack on this issue is to ask why we should upgrade in the first place?

• Does v5 contain any fundamental security improvements (not just "APIs are better designed so people are less likely to end up writing insecure code") over v4.5.0?
• Will v4.5.0 be maintained as far as security improvements and required changes (to support new versions of Golang and/or dependencies) go? I.e. as v5 is fundamentally incompatible for some uses, will v4 be kept on an "even playing field" as far as maintenence goes?

These questions strike at the heart of problems we're having with more than just this package: when authors refactor APIs in non-backwards-compatible ways, and especially when they subsequently abandon older versions, they leave those with substantial investments in those older APIs in the dust.

This problem, as bad as it is, is greatly compounded when, while refactoring those APIs, the authors completely remove functionality (as seems to have happened here, in the sense that the ability to orthagonally validate claims, separate from validating a signature, has been removed).

This is not the first experience we've had that has led our upper-management team to ask us "Why don't we have this problem in <insert-another-language-ecosystem>?", to which we don't have answers. (There might be good answers; we just don't have them at hand.)

To them, it smacks of a lack of commitment to a solid, secure codebase across an ecosystem that changes are made that seem to not be justifiable on the face of it (even if, to us "low-level techies", we have our own justifications for them).

The compounding effect of these issues arising across multiple dependent packages — including even those (such as jwt) plainly maintained by developers who seem of the more-professional, more-experienced type, as well as those random things we happened to find lying around and (for better or worse) referenced in our code — is definitely contributing to an overall distaste for the Golang ecosystem. (I personally find this disappointing, as I really really like Golang, and haven't worked in one of those "other" ecosystems management keeps asking about for over 10 years, and in another popular one basically ever.)

I hope these perspectives are useful for me to have expressed, even if done so in classic rant form.

[jcburley]

Note that I updated the Summary/Description for clarity.

[oxisto]

TL;DR

I have created a test PR that exports the NewValidator function, which can be used to create a validator (intentionally private) and can be used to validate claims independently of parsing/verifying the token. It supports all the validation options, that also the Parser uses.

var v = jwt.NewValidator(jwt.WithLeeway(5*time.Second))
v.Validate(myClaims)

Personally, it is still a bit "weird" to only validate claims without the signature; even in unit tests you should at least have some kind of signature validation because this is the first thing you will get wrong when copying code from test to non-test 🤷. It was a little bit of bad design that the library previously "supported" that, but that's beside the point. The reason that I stress this point is that the intention of this library is to provide a complaint way to deal with JWTs and not checking the signature is not complaint, simple as that.

As alluded to at the end of my previous comment, another tack on this issue is to ask why we should upgrade in the first place?

• Does v5 contain any fundamental security improvements (not just "APIs are better designed so people are less likely to end up writing insecure code") over v4.5.0?

v5 was focused on customizing the validation of claims, which was one of the most requested features in some form or other (e.g., having a leeway in exp/iss, etc.). Unfortunately, this was not possible to introduce in a backwards compatible way and we decided to carefully re-organise the API.

• Will v4.5.0 be maintained as far as security improvements and required changes (to support new versions of Golang and/or dependencies) go? I.e. as v5 is fundamentally incompatible for some uses, will v4 be kept on an "even playing field" as far as maintenence goes?

Well, to be honest, I will not give any guarantee on maintaining v4 whatsoever. This project is currently maintained by two people in their free time and because we know of the importance of this project because of its wide usage, we are very careful in adding or removing features (see below). It does mean however, that we cannot have the luxury of maintaining different versions, at least not as a "strategy". If for some unforeseen reason, there are critical issues found in the v4 branch, we will probably still fix and backport them, but as I said, I will not give a guarantee on this.

BUT, that is the main reason for semantic versioning: You are guaranteed to not get any breaking changes in v4, but you will a) not get the features and b) you will at some point get "phased" out.

These questions strike at the heart of problems we're having with more than just this package: when authors refactor APIs in non-backwards-compatible ways, and especially when they subsequently abandon older versions, they leave those with substantial investments in those older APIs in the dust.

This problem, as bad as it is, is greatly compounded when, while refactoring those APIs, the authors completely remove functionality (as seems to have happened here, in the sense that the ability to orthagonally validate claims, separate from validating a signature, has been removed).

As mentioned before, we were extremely careful about doing backwards compatible, especially in the v4 version. When the need arose because of large demand of features from the community, we arrived at a point where we could not progress without breaking changes. The main issue was that the Valid() function was a poor design choice from the start because it did not give any option to make the validation configurable. Therefore we chose to externalize the validation in a new validator struct. We had a large discussion whether we want to export that struct (or the function newValidator) or not and chose to NOT export it for now, given that we want to see if a) there is the need for it and b) if the functions on it are somewhat stable. Now this point may have arrived.

Not trying to get defensive here, but you made it sound like we rashly removed this functionality in a day or to. That is definitely not the case. We had the first discussions on this in August 2022 (see original post of #234), a first and release candidate in Feburary/March 2023 and the final release in April 2023.

This is not the first experience we've had that has led our upper-management team to ask us "Why don't we have this problem in ?", to which we don't have answers. (There might be good answers; we just don't have them at hand.)

This may sound harsh, but the problem is in your code base, not the eco system ;)

To them, it smacks of a lack of commitment to a solid, secure codebase across an ecosystem that changes are made that seem to not be justifiable on the face of it (even if, to us "low-level techies", we have our own justifications for them).

The compounding effect of these issues arising across multiple dependent packages — including even those (such as jwt) plainly maintained by developers who seem of the more-professional, more-experienced type, as well as those random things we happened to find lying around and (for better or worse) referenced in our code — is definitely contributing to an overall distaste for the Golang ecosystem. (I personally find this disappointing, as I really really like Golang, and haven't worked in one of those "other" ecosystems management keeps asking about for over 10 years, and in another popular one basically ever.)

I hope these perspectives are useful for me to have expressed, even if done so in classic rant form.

[jcburley]

TL;DR

(Much appreciated, lol!)

I have created a test PR that exports the NewValidator function, which can be used to create a validator (intentionally private) and can be used to validate claims independently of parsing/verifying the token. It supports all the validation options, that also the Parser uses.

var v = jwt.NewValidator(jwt.WithLeeway(5*time.Second))
v.Validate(myClaims)

Wow, that "just works" for us, across the entire wrapping package (not just the wrapping module, but also other modules within the package that use the module); as well as in one other repo I tried, which pulls in that package; thanks!! And it took me about a half-hour to incorporate it. (I did rename the internal API name so it's no longer .Valid(); it's .ValidateClaimsOnly().)

Personally, it is still a bit "weird" to only validate claims without the signature; even in unit tests you should at least have some kind of signature validation because this is the first thing you will get wrong when copying code from test to non-test 🤷. It was a little bit of bad design that the library previously "supported" that, but that's beside the point. The reason that I stress this point is that the intention of this library is to provide a complaint way to deal with JWTs and not checking the signature is not complaint, simple as that.

This is helpful to me, though I'm still not sure why one would need (want?) to keep re-checking the signature while passing around a token from point A to point Z, all entirely within a single process, through various interceptors/loggers/etc.

But the "you will get wrong when copying code from test to non-test" point is valid all by itself, certainly!

(Our code base might simply have "gotten off on the wrong foot" with this package, as it probably has with others, due to a combination of being early adopters of such packages and the ecosystem generally and, well, the usual issues around feeling rushed to get something out that "just works". I think something like 98% of our Golang code base was written before I joined the company, so I have little to no personal stake in it; but don't want to criticize those who built it, because in at least a few ways it's fairly impressive.)

As alluded to at the end of my previous comment, another tack on this issue is to ask why we should upgrade in the first place?

• Does v5 contain any fundamental security improvements (not just "APIs are better designed so people are less likely to end up writing insecure code") over v4.5.0?

v5 was focused on customizing the validation of claims, which was one of the most requested features in some form or other (e.g., having a leeway in exp/iss, etc.). Unfortunately, this was not possible to introduce in a backwards compatible way and we decided to carefully re-organise the API.

That makes sense, and the care y'all took is quite evident (to me anyway). I've been doing similar refactorings here and there within our code base, inspired by packages such as yours, when I find the time and opportunity.

• Will v4.5.0 be maintained as far as security improvements and required changes (to support new versions of Golang and/or dependencies) go? I.e. as v5 is fundamentally incompatible for some uses, will v4 be kept on an "even playing field" as far as maintenence goes?

Well, to be honest, I will not give any guarantee on maintaining v4 whatsoever. This project is currently maintained by two people in their free time and because we know of the importance of this project because of its wide usage, we are very careful in adding or removing features (see below). It does mean however, that we cannot have the luxury of maintaining different versions, at least not as a "strategy". If for some unforeseen reason, there are critical issues found in the v4 branch, we will probably still fix and backport them, but as I said, I will not give a guarantee on this.

That's an expected and reasonable answer.

BUT, that is the main reason for semantic versioning: You are guaranteed to not get any breaking changes in v4, but you will a) not get the features and b) you will at some point get "phased" out.

Yup. (If we weren't using automated version/dependency tracking on our repos, we might not have noticed this issue for quite a bit longer.)

These questions strike at the heart of problems we're having with more than just this package: when authors refactor APIs in non-backwards-compatible ways, and especially when they subsequently abandon older versions, they leave those with substantial investments in those older APIs in the dust.
This problem, as bad as it is, is greatly compounded when, while refactoring those APIs, the authors completely remove functionality (as seems to have happened here, in the sense that the ability to orthagonally validate claims, separate from validating a signature, has been removed).

As mentioned before, we were extremely careful about doing backwards compatible, especially in the v4 version. When the need arose because of large demand of features from the community, we arrived at a point where we could not progress without breaking changes. The main issue was that the Valid() function was a poor design choice from the start because it did not give any option to make the validation configurable. Therefore we chose to externalize the validation in a new validator struct. We had a large discussion whether we want to export that struct (or the function newValidator) or not and chose to NOT export it for now, given that we want to see if a) there is the need for it and b) if the functions on it are somewhat stable. Now this point may have arrived.

It does seem helpful to expose it at this point, and I do like (though don't yet 100% understand) the new API better than the old (which I understand even less, tbh).

Not trying to get defensive here, but you made it sound like we rashly removed this functionality in a day or to. That is definitely not the case. We had the first discussions on this in August 2022 (see original post of #234), a first and release candidate in Feburary/March 2023 and the final release in April 2023.

That wasn't my intent (to make it sound like y'all acted rashly); I was, however, attempting to highlight what effects these sorts of decisions have when they lead to roadblocks that are surfaced to management, at which point the thoughtfulness that went into making those decisions isn't particularly evident.

I'm curious, though as to why the .Valid() function wasn't marked as deprecated in a v4 Release? Maybe it should be now, for those who are (intentionally or otherwise) "behind the curve"? The deprecation message could suggest a workaround, like the one I just wrote to use your NewValidator() function.

This is not the first experience we've had that has led our upper-management team to ask us "Why don't we have this problem in ?", to which we don't have answers. (There might be good answers; we just don't have them at hand.)

This may sound harsh, but the problem is in your code base, not the eco system ;)

Could you elaborate? (I assume you're talking about some wider problem than just "our code base was using an API that was later deleted".)

I mean, I have (plenty of) my own opinions and could speculate, but if you have any particular insights stemming from my (certainly inadequate and incomplete) characterizations of the codebase so far, that might be helpful if/when it comes to advocating for making non-customer-facing improvements to it in the future.

(Note that, overall, our code base encompasses a fairly wide range of ecosystems, though not much of anything older than 15-20 years. I got this job mainly due to my then-recent Golang experience and expertise; but that part of our ecosystem is barely client-facing, mostly back-end, and shares back-end duties with code in other ecosystems as well. So our management has, besides their own professional experience often including being developers themselves, a reasonably broad understanding of those ecosystems and the experiences we have with them. Hence those sorts of questions, that they might ask, are to be expected.)

[oxisto]

TL;DR

(Much appreciated, lol!)

I have created a test PR that exports the NewValidator function, which can be used to create a validator (intentionally private) and can be used to validate claims independently of parsing/verifying the token. It supports all the validation options, that also the Parser uses.

var v = jwt.NewValidator(jwt.WithLeeway(5*time.Second))
v.Validate(myClaims)

Wow, that "just works" for us, across the entire wrapping package (not just the wrapping module, but also other modules within the package that use the module); as well as in one other repo I tried, which pulls in that package; thanks!! And it took me about a half-hour to incorporate it. (I did rename the internal API name so it's no longer .Valid(); it's .ValidateClaimsOnly().)

Good to hear!

Personally, it is still a bit "weird" to only validate claims without the signature; even in unit tests you should at least have some kind of signature validation because this is the first thing you will get wrong when copying code from test to non-test 🤷. It was a little bit of bad design that the library previously "supported" that, but that's beside the point. The reason that I stress this point is that the intention of this library is to provide a complaint way to deal with JWTs and not checking the signature is not complaint, simple as that.

This is helpful to me, though I'm still not sure why one would need (want?) to keep re-checking the signature while passing around a token from point A to point Z, all entirely within a single process, through various interceptors/loggers/etc.

But the "you will get wrong when copying code from test to non-test" point is valid all by itself, certainly!

(Our code base might simply have "gotten off on the wrong foot" with this package, as it probably has with others, due to a combination of being early adopters of such packages and the ecosystem generally and, well, the usual issues around feeling rushed to get something out that "just works". I think something like 98% of our Golang code base was written before I joined the company, so I have little to no personal stake in it; but don't want to criticize those who built it, because in at least a few ways it's fairly impressive.)

As alluded to at the end of my previous comment, another tack on this issue is to ask why we should upgrade in the first place?

• Does v5 contain any fundamental security improvements (not just "APIs are better designed so people are less likely to end up writing insecure code") over v4.5.0?

v5 was focused on customizing the validation of claims, which was one of the most requested features in some form or other (e.g., having a leeway in exp/iss, etc.). Unfortunately, this was not possible to introduce in a backwards compatible way and we decided to carefully re-organise the API.

That makes sense, and the care y'all took is quite evident (to me anyway). I've been doing similar refactorings here and there within our code base, inspired by packages such as yours, when I find the time and opportunity.

• Will v4.5.0 be maintained as far as security improvements and required changes (to support new versions of Golang and/or dependencies) go? I.e. as v5 is fundamentally incompatible for some uses, will v4 be kept on an "even playing field" as far as maintenence goes?

Well, to be honest, I will not give any guarantee on maintaining v4 whatsoever. This project is currently maintained by two people in their free time and because we know of the importance of this project because of its wide usage, we are very careful in adding or removing features (see below). It does mean however, that we cannot have the luxury of maintaining different versions, at least not as a "strategy". If for some unforeseen reason, there are critical issues found in the v4 branch, we will probably still fix and backport them, but as I said, I will not give a guarantee on this.

That's an expected and reasonable answer.

BUT, that is the main reason for semantic versioning: You are guaranteed to not get any breaking changes in v4, but you will a) not get the features and b) you will at some point get "phased" out.

Yup. (If we weren't using automated version/dependency tracking on our repos, we might not have noticed this issue for quite a bit longer.)

These questions strike at the heart of problems we're having with more than just this package: when authors refactor APIs in non-backwards-compatible ways, and especially when they subsequently abandon older versions, they leave those with substantial investments in those older APIs in the dust.
This problem, as bad as it is, is greatly compounded when, while refactoring those APIs, the authors completely remove functionality (as seems to have happened here, in the sense that the ability to orthagonally validate claims, separate from validating a signature, has been removed).

As mentioned before, we were extremely careful about doing backwards compatible, especially in the v4 version. When the need arose because of large demand of features from the community, we arrived at a point where we could not progress without breaking changes. The main issue was that the Valid() function was a poor design choice from the start because it did not give any option to make the validation configurable. Therefore we chose to externalize the validation in a new validator struct. We had a large discussion whether we want to export that struct (or the function newValidator) or not and chose to NOT export it for now, given that we want to see if a) there is the need for it and b) if the functions on it are somewhat stable. Now this point may have arrived.

It does seem helpful to expose it at this point, and I do like (though don't yet 100% understand) the new API better than the old (which I understand even less, tbh).

Not trying to get defensive here, but you made it sound like we rashly removed this functionality in a day or to. That is definitely not the case. We had the first discussions on this in August 2022 (see original post of #234), a first and release candidate in Feburary/March 2023 and the final release in April 2023.

That wasn't my intent (to make it sound like y'all acted rashly); I was, however, attempting to highlight what effects these sorts of decisions have when they lead to roadblocks that are surfaced to management, at which point the thoughtfulness that went into making those decisions isn't particularly evident.

I'm curious, though as to why the .Valid() function wasn't marked as deprecated in a v4 Release? Maybe it should be now, for those who are (intentionally or otherwise) "behind the curve"? The deprecation message could suggest a workaround, like the one I just wrote to use your NewValidator() function.

That is a fair point. The challenge is that we cannot back port the validator to v4 without introducing breaking changes. We could however discourage the use of the Valid() function to be called directly and instead refer to the still usable Valid property of the token. The Valid method was really only ever meant as a way to satisfy the interface and not as a function to be called by the user. We can stress that part again in the documentation and back port this as v4.5.1.

This is not the first experience we've had that has led our upper-management team to ask us "Why don't we have this problem in ?", to which we don't have answers. (There might be good answers; we just don't have them at hand.)

This may sound harsh, but the problem is in your code base, not the eco system ;)

Could you elaborate? (I assume you're talking about some wider problem than just "our code base was using an API that was later deleted".)

It was more related to the "unlucky" fact that your code base happened to call a function which was never really supposed to be called; but yes it's part of the public API so I cannot blame you for calling it :D

[jcburley]

I'm curious, though as to why the .Valid() function wasn't marked as deprecated in a v4 Release? Maybe it should be now, for those who are (intentionally or otherwise) "behind the curve"? The deprecation message could suggest a workaround, like the one I just wrote to use your NewValidator() function.

That is a fair point. The challenge is that we cannot back port the validator to v4 without introducing breaking changes. We could however discourage the use of the Valid() function to be called directly and instead refer to the still usable Valid property of the token. The Valid method was really only ever meant as a way to satisfy the interface and not as a function to be called by the user. We can stress that part again in the documentation and back port this as v4.5.1.

Sorry, I wasn't clear: the deprecation message would point to the workaround when upgrading to v5 (assuming your PR gets merged/released), not something available in v4 (though that might be a nice thing to provide as well, except it'd not be trivial to do so in the v4 API.)

[oxisto]

I'm curious, though as to why the .Valid() function wasn't marked as deprecated in a v4 Release? Maybe it should be now, for those who are (intentionally or otherwise) "behind the curve"? The deprecation message could suggest a workaround, like the one I just wrote to use your NewValidator() function.

That is a fair point. The challenge is that we cannot back port the validator to v4 without introducing breaking changes. We could however discourage the use of the Valid() function to be called directly and instead refer to the still usable Valid property of the token. The Valid method was really only ever meant as a way to satisfy the interface and not as a function to be called by the user. We can stress that part again in the documentation and back port this as v4.5.1.

Sorry, I wasn't clear: the deprecation message would point to the workaround when upgrading to v5 (assuming your PR gets merged/released), not something available in v4 (though that might be a nice thing to provide as well, except it'd not be trivial to do so in the v4 API.)

Ah! Got it. Yes we should upgrade the migration guide in the v5 docs in the above PR. Furthermore, I added a deprecation notice to Valid() in a separate v4 PR to warn users to not directly call Valid() because it will go away once they upgrade to v5.