feat: add PS JWA support

[panva]

the PS family of JSON Web Algorithms is supported in node.js versions ^6.12.0, not 7, and then >=8

[ziluvatar]

Nice 👏

🤔 Even if this is only available for certain node versions, this is not a breaking change because all consumers using current functionality can continue working the same.

However, we have the problem of:

• Documentation, make it clear in what versions of node it is available
• Tests, since it would run for 4+ we would need to use something like jwa did in https://github.com/brianloveswords/node-jwa/blob/master/test/jwa.test.js#L75 to avoid run tests for them in v4 and v7

Could you take a look to those points? also to extend the current testing up to 11 (travis config)

[panva]

🤔 Even if this is only available for certain node versions, this is not a breaking change because all consumers using current functionality can continue working the same.

Not necessarily, to some extent PS verifying worked prior to this PR already (due to the RS/PS similarities). To my knowledge the behaviour before was undefined, where it is now well defined. That's a breaking change.

Could you take a look to those points? also to extend the current testing up to 11 (travis config)

from the PR's .travis.yml

  - 6.12.0 # lowest for PS support
  - lts/boron # lts/boron latest
  - 8.0.0 # 8 min
  - lts/carbon # lts/carbon latest
  - lts/dubnium # lts/dubnium latest
  - stable # stable

This is already testing 11 (current stable).

Documentation, make it clear in what versions of node it is available

Not sure how to best approach this. Node versions are strictly checked during yarn and warned during npm i, that should be enough of a warning.

[Nicholaiii]

First of, +1 on this, secondly:

Not necessarily, to some extent PS verifying worked prior to this PR already (due to the RS/PS similarities). To my knowledge the behaviour before was undefined, where it is now well defined. That's a breaking change.

Per SEMVER specifications:

MAJOR version when you make incompatible API changes,
MINOR version when you add functionality in a backwards-compatible manner

I strongly believe this is a case of a semver minor and not a breaking change. This will still work in Node version that do not support PS.
If a given application in eg. Node 4 uses node-jsonwebtoken it already does not use PS and updating will not break anything.

[ziluvatar]

I just tested your changes using Node 4 and Node 7, with these results:

• Node 4: Cannot read property 'RSA_PKCS1_PSS_PADDING' of undefined (from jwa on signing)
• Node 7: Key must be a buffer (from crypto on verifying, even passing a buffer, I haven't dug more)

Up to now (before your changes) if you try to sign using PS256 "algorithm" must be a valid string enum value. If you try to verify one signed with PS256 you would get JsonWebTokenError: invalid algorithm. So there is no way this could be breaking change.

What I meant is, since it is not a breaking change, just there is no need to modify the travis config to accurate the node versions, you can use the "ifs" that I pasted from jwa repo for testing.

[panva]

If a given application in eg. Node 4 uses node-jsonwebtoken it already does not use PS and updating will not break anything.

If an application receives a PS signed token and attempts to validate it in versions not supporting it there will be an uncaught exception (worst case) because of the constants not being available and PS all of a sudden passing the jsonwebtoken algorithm validations

if statements would therefore be needed in the code as well, not just the test suite

hence, a breaking change, no?

[Nicholaiii]

If you want to maintain backwards compatibility and include PS you could require it to be explicitly specified algorithm during verify rather than a default. Either that or throw a JsonWebTokenError with unsupported algo if unsupported. This should already be covered in existing applications.

[ziluvatar]

🤔 yeah you are right @panva @Nicholaiii the problem is the algorithm inference we have based on the key on verify(). The only way to release this without breaking is as @Nicholaiii said, avoid adding it to the inference, so the consumer has to make its usage explicit (for now).
In next major we could add it to the inference and remove the compatibility with 4, 5 and 7.

Thoughts?

[Macil]

To my knowledge the behaviour before was undefined, where it is now well defined. That's a breaking change.

If by "undefined" you meant "anything could happen", then the new version's behavior is still compatible with that. (If version 1 says doing X can cause anything to happen with no guarantees, and version 2 says doing X is guaranteed to do a specific thing, then that's not a breaking change. Code written against version 1 isn't following the API correctly if it expects doing X to reliably do a specific thing like throw an error.)

If an application receives a PS signed token and attempts to validate it in versions not supporting it there will be an uncaught exception (worst case) because of the constants not being available and PS all of a sudden passing the jsonwebtoken algorithm validations

I might be misunderstanding the situation, but won't the token only be successfully verified if the application specified the PS algorithm as being supported and the correct private key? Why would the developer in that case pass an unsupported private key, and then consider it a breaking change when that private key starts being supported? (How would the application user get a signed JWT for an unused private key if it wasn't intended to be used?)

Any new feature added to a library is in some sense a breaking change if there was already code in the wild trying to make use of the non-existent feature and then expecting a failure. If that was considered a breaking change by semver, then there wouldn't ever be anything that qualified as a non-breaking change for a minor version upgrade.

[Macil]

Oh, if adding PS support caused the PS algorithm to automatically be inferred in some cases, then I can imagine how that would be a problem if the new version ever inferred differently than older ones. I didn't know this library did that. I assumed that the algorithm had to be specified always. (Is that documented? I scanned through the readme a few times.) Personally I'd expect that inferring the algorithm could be a source of footguns after seeing this be a source of issues in various JWT libraries.

[panva]

If you want to maintain backwards compatibility and include PS you could require it to be explicitly specified algorithm during verify rather than a default. Either that or throw a JsonWebTokenError with unsupported algo if unsupported. This should already be covered in existing applications.

Unfortunately this would also be a breaking change as verify with PS algorithms already somewhat works in earlier versions. The thing is, since the use of constants isn't safeguarded in node-jws it will cause an exception after this code is merged.

🤔 yeah you are right @panva @Nicholaiii the problem is the algorithm inference we have based on the key on verify(). The only way to release this without breaking is as @Nicholaiii said, avoid adding it to the inference, so the consumer has to make its usage explicit (for now).

same point as above

I might be misunderstanding the situation, but won't the token only be successfully verified if the application specified the PS algorithm as being supported and the correct private key? Why would the developer in that case pass an unsupported private key, and then consider it a breaking change when that private key starts being supported? (How would the application user get a signed JWT for an unused private key if it wasn't intended to be used?)

You are assuming the library is only used in closed & controlled environments. That is not true. A common use of jsonwebtoken is to secure public facing APIs and you simply do not control the inputs. Given that a malicious party could've sent a PS signature before and it worked (produced expected library error) and doesn't now (in node 4, causes an uncaught) - if a developer is handling specific library error prototypes only.

Explicitly disallowing PS in the non applicable node versions breaks (altho probably not intended) someone verifying PS signatures already.

Unfortunately new installations are already affected and run into unspecified behaviours - the jws update was released as minor, disregarding the fact that PS verifications worked before.

[panva]

Bottom line, @ziluvatar it's your call, i can change the PR to whatever you want but I wanted to make it visible that trying to maintain backwards node compatibility is introducing new breaking changes. And we got those introduced already since node-jws doesn't safeguard the use of constants for PS when they're not available.

I'm not going to be affected by these breaking changes myself so ¯\_(ツ)_/¯

[panva]

updated as per @ziluvatar's wishes

[panva]

FWIW i retraced my steps and confirmed there's indeed no breaking change, if anything this PR will fix unexpected errors produced by the fact that jwa is now updated and doesn't produce the expected errors on unsupported versions anymore.

[ziluvatar]

Thanks @panva ! 👏 Released on v8.5.0

[omsmith]

@panva Can you describe how you were using PS* with jwa before that you felt it was broken? From the sounds of it, you were able to s/PS/RS/ and it would work for verifying?

Also from the sounds of it, are you suggesting the current PS* support could be updated to use a string constant locally instead of from crypto.constants and it will work for older node versions (the only difference in newer versions being that the constant is available?

I'm happy to accept an issue or a PR from you on either of jws or jwa.

[panva]

@omsmith

Also from the sounds of it, are you suggesting the current PS* support could be updated to use a string constant locally instead of from crypto.constants and it will work for older node versions (the only difference in newer versions being that the constant is available?

No, the constants are just convenience, before the node crypto API didn't have an option to pass anything else but a PEM cert.

Can you describe how you were using PS* with jwa before that you felt it was broken? From the sounds of it, you were able to s/PS/RS/ and it would work for verifying?

I realized ad acta that i already had a transient node-jwa dep version installed that supported PS. False 📢