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

RSA public/private key #19

Closed
tanner0101 opened this issue Feb 8, 2017 · 4 comments
Closed

RSA public/private key #19

tanner0101 opened this issue Feb 8, 2017 · 4 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@tanner0101
Copy link
Member

Unless I'm mistaken, the RSA signers should have a separate public and private key. The private key is used for signing and the public key is used for verifying the signature.

Right now you can only add one key that is used for both.

@tanner0101 tanner0101 added the bug Something isn't working label Feb 8, 2017
@vzsg
Copy link
Member

vzsg commented Feb 8, 2017

I'd like to add two remarks, if you don't mind:

  1. The current behavior is not uncommon, for example the auth0/java-jwt library requires the developer to make a second instance of their JWT class, using the public key. I'm not implying that this experience cannot or should not be improved.

  2. Fun fact: an RSA private key implicitly contains the public key too. If you combine this with the way how RSA validation is implemented in LibreSSL (hint: both keys are decoded into the same RSA C struct), it's almost trivially simple to both sign and verify tokens with the private key only. The only thing that needs to be changed is the call to d2i_RSA_PUBKEY in RSASigner.swift.
    (I confirmed it by replacing this call with d2i_RSAPrivateKey as above.)

So an ideal implementation of RSASigner IMHO would detect if the provided key is a private or public key. If it's a private key, both functions can work directly using it. If it's a public key, sign should throw an error and verify should work as expected.

@tanner0101
Copy link
Member Author

tanner0101 commented Feb 9, 2017

Ah, being able to use d2i_RSAPrivateKey in the verify function is the piece I was missing. Awesome.

If it's that easy to use the private key for verification then we should definitely avoid asking the user to input both keys. That's another user input step which I'm happy to avoid.

I'm thinking the best solution here is to have the RSA signers take a Key enum:

enum Key {
    case public(value: Bytes)
    case private(value: Bytes)
}

In an attempt to create a JWT with a Key.public we can throw an error along the lines of JWTError.privateKeyRequired

By not requiring the private key we can allow this package to be used in client code.

The Config/jwt.json in the jwt-provider package could then look something like

{
    "signer": {
        "type": "rsa",
        "algorithm": "rs256",
        "key": {
            "type": "public",
            "value": "..."
        }
    }
}

@vzsg
Copy link
Member

vzsg commented Feb 9, 2017

I think introducing a public enum or otherwise changing the current public interface is not necessary.

The RSASigner could try to parse the raw bytes in both formats and see which function returns success, throwing an error only in sign mode with a public key, or if neither decoding worked.

Everything else should Just Work transparently.

(Also, if RSASigner was a class instead of a protocol, it would be possible to decode the key only once in the initializer instead of in each request. It's an immutable bag of big integers, and as DER-encoded bytes are held in memory anyway, keeping the RSA in too is not less secure.)

@tanner0101
Copy link
Member Author

tanner0101 commented Feb 9, 2017

+1 to the idea of parsing the key once and holding onto that.

However, I like the clarity around providing either a public or private key at the configuration level. If someone reading the code sees Key.private it's clear this is something sensitive.
I do like the idea of "just working", though.

I'll throw together a PR for this and see if there are any underwater rocks with either option that might make the other more attractive.

@tanner0101 tanner0101 self-assigned this Feb 9, 2017
@tanner0101 tanner0101 added this to the 0.8 milestone Feb 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants