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

Adding custom algorithm #429

Closed
wants to merge 4 commits into from
Closed

Adding custom algorithm #429

wants to merge 4 commits into from

Conversation

Teukros
Copy link

@Teukros Teukros commented Dec 6, 2017

This pull request introduce possibility to use your own algorithm with jsonwebtoken library. If you pass proper options you can use sign, verify and decode methods together with chosen algorithm function.

sign.js Outdated
header: { isValid: isPlainObject, message: '"header" must be an object' },
encoding: { isValid: isString, message: '"encoding" must be a string' },
issuer: { isValid: isString, message: '"issuer" must be a string' },
subject: { isValid: isString, message: '"subject" must be a string' },
jwtid: { isValid: isString, message: '"jwtid" must be a string' },
noTimestamp: { isValid: isBoolean, message: '"noTimestamp" must be a boolean' },
keyid: { isValid: isString, message: '"keyid" must be a string' }
keyid: { isValid: isString, message: '"keyid" must be a string' },
customAlgorithmFunction: {isValid: isFunction, message: "custom algorithm must provide it's own sign function"}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's -> its
" -> '

sign.js Outdated
.once('done', function (signature) {
callback(null, signature);
if (isCustomAlgorithmUsed) {
options.customAlgorithmFunction(payload, secretOrPrivateKey, options, function(err, result) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting.

verify.js Outdated
try {
decodedToken = options.customAlgorithmFunction(jwtString, secretOrPublicKey, options);
return done(null, decodedToken);
} catch(err) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting.

README.md Outdated
@@ -203,6 +234,8 @@ __Warning:__ This will __not__ verify whether the signature is valid. You should

* `json`: force JSON.parse on the payload even if the header doesn't contain `"typ":"JWT"`.
* `complete`: return an object with the decoded payload and header.
* `isCustomAlgorithmUsed`: boolean that declares if you want to use your own algorithm for decoding.
* `customAlgorithmFunction`: function which will be used for decoding, if isCustomAlgorithmUsed is true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing ".".

@rochdev
Copy link

rochdev commented Jan 10, 2018

I think it should be possible to use a custom signing function even when using a predefined algorithm.

My use case: #427 (comment)

@ziluvatar
Copy link
Contributor

Sorry, I still have some concerns about this, what is the benefit of adding custom signing to this library? (at least how it is in the PR so far)

  • On signing: I can see a little: some payload generation. Signer ends up even filling the header part. The function could even avoid adding any header or anything it could bypass the JWT format and build any other format.
  • On verifying: It just get a string and return the object.

It seems to me like a simple serializer / deserializer. We should think how to offer this as simple as possible to the consumer and be sure that we are still in JWT playground.

If you take a look to jws package, what you want is to use your own algo: https://github.com/brianloveswords/node-jws/blob/master/lib/sign-stream.js#L21, so to generate the last part of the JWT (header.payload.signature). Is that right?

There is an open ticket there about your own use case: auth0/node-jws#34. I think it would have more sense to have it supported in there.

If we still wanted to support it here we would need to write, or bring, most of that to this library, having here two signers with same interface custom and jws and leaving the final custom signing function for the consumer similar to what they do in jws: algo(securedInput, secretOrKey) where securedInput is "header.payload" in base64 with encoding in mind.
If we don't do all of that, consumer would need to build jws basically to do it right.

What if you try to bump up the topic on jws repo and we pause this for now? Thoughts?

@novemberborn
Copy link

what is the benefit of adding custom signing to this library?

I'm currently investigating custom algorithms. In my use case I have a secp256k1 private key I want to use to sign a JWT. The JWT would contain a claim of the corresponding public key.

Interface wise I think the algorithms option here should be an array that accepts strings and objects. For objects, keys would be the custom algorithm name and the value either a signer or verifier:

jwt.sign(payload, secretOrPrivateKey, {
  algorithm: {
    'urn:foo:bar:custom:yay' (input) {
      return sign(input)
    }
  }
})

jwt.verify(token, publicKey, {
  algorithms: [
    {
      'urn:foo:bar:custom:yay' (input) {
        return verify(input)
      }
    }
  ]
})

This library could select the correct algorithm when verifying. I agree that it makes sense for jws to accept a custom function. Though that project doesn't look particularly active.

@ziluvatar
Copy link
Contributor

There was a PR about it in the past: auth0/node-jws#35 I bet it was closed since some feedback was provided but no reaction was made by the PR creator.
Maybe someone can re-create it applying the feedback.

@panva
Copy link
Contributor

panva commented Jun 25, 2019

JWT algorithms are well defined and we're committed to support the defined ones, as soon as the jws dependency supports them too.

secp256k1 curve EC keys and alg name registration is only just being standardized and is still very much in flux

The only other missing defined DSA JWA is EdDSA which is available in node 12 and will eventually make it to jsonwebtoken too.

Also as a general recommendation you should not roll your own crypto, which this very much looks like. And for the sake of interoperability we should guide the users of this lib to stay within the defined JWAs.

@panva panva closed this Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants