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

Supporting RSA-SHA256 #3

Closed
akramer opened this issue Mar 26, 2016 · 11 comments
Closed

Supporting RSA-SHA256 #3

akramer opened this issue Mar 26, 2016 · 11 comments

Comments

@akramer
Copy link

akramer commented Mar 26, 2016

Wonderful middleware! It would be even better with public key algorithm support. I can give it a shot and send a PR in a week or two.

@BTBurke
Copy link
Owner

BTBurke commented Mar 26, 2016

Oh you're right, I hard coded the algorithm based on my usage. Implementing RS256 is supported by the underlying library this is using for validation so it shouldn't be a tough change.

Is it correct to assume that Caddy would still have prior knowledge of the public key it's supposed to use to validate the token such that it can be stored in JWT_SECRET for all requests? If not, then it becomes slightly more complicated because it would have to support both discovering the public key and having a configuration parameter in the Caddyfile that would specify the signing algorithm, otherwise it would be vulnerable to the attack discovered last year by altering the "alg" header.

@akramer
Copy link
Author

akramer commented Mar 26, 2016

My assumption was that the key would be available via an environment variable like JWT_PUBLIC_KEY. You've already nicely broken out the validation function so it could switch which variable it uses based on which algorithm is passed to the function.

@BTBurke
Copy link
Owner

BTBurke commented Mar 27, 2016

Shouldn't be too tricky. I think as long as we use two different environment variables and warn people in the docs not to put their public key in JWT_SECRET it should be ok. The problem is if they mistakenly put their public key in JWT_SECRET, an attacker can forge a token that would be accepted by only knowing the public key. They would use the public key to sign a new token and specify HS256 instead of RS256 in the token header. The JWT middleware currently would have no way of knowing that it's expecting only RS256 tokens and so would use the public key as the secret in HS256. That would be a serious vulnerability.

Another avenue is to modify the Caddyfile to force everyone to specify which algorithm they are using. This would make it more secure but also a bit harder to parse and for people to configure.

It could look something like this:

jwt alg hs256
jwt /path1

or get rid of the one line configs and have everything look like

jwt {
   alg hs256
   path /path1
   allow user someone
}

@akramer
Copy link
Author

akramer commented Mar 27, 2016

The cert needs to be in PEM format for the JWT library. It should be safe to verify that JWT_SECRET doesn't start with "-----BEGIN CERTIFICATE" and error out if it does.

Is that a gross hack? I can't tell, but it should keep the configs simple and still allow use of both hmac and rsa safely.

@BTBurke
Copy link
Owner

BTBurke commented Apr 9, 2016

I'm still working on this. It's actually a bit harder than I thought it would be to fit it into the current auth flow.

@coolaj86
Copy link

coolaj86 commented Apr 20, 2016

Shouldn't there be a way to specify the public key used for validation?

Or maybe there is and I'm just missing it. I would expect it to be a key in the jwt object in the Caddyfile

@BTBurke
Copy link
Owner

BTBurke commented Apr 20, 2016

There will be once I finish the RSA support. Right now the current middleware supports HS256/384/512.

The RSA version will use JWT_PUBLIC_KEY for validation when it encounters an RSA-SHA encrypted token.

It's not ready for prime time yet but I'm going to work on it this weekend.

On Apr 20, 2016, at 9:04 AM, AJ ONeal notifications@github.com wrote:

Shouldn't there be a way to specify the public key used for validation?


You are receiving this because you commented.
Reply to this email directly or view it on GitHub

@namsral
Copy link
Contributor

namsral commented Jul 12, 2016

How about using the kid header parameter documented in RFC 7515 to look up the key and its corresponding algorithm to support multiple algorithms and solve the signature verification method attack.

Caddyfile example:

jwt {
    path [path]

    # key [id] [type] [env*]
    key a oct KEY_A
    key b oct KEY_B
    key c rsa KEY_C
    key "d e" ec KEY_DE
}

*environment variable containing passphrase or pem encoded public key

Related JOSE header which matches the first key parameter in the Caddyfile:

{
    "kid": "a",
    "alg": "hs256"
}

@BTBurke
Copy link
Owner

BTBurke commented Jul 19, 2016

I think it becomes a little complicated to handle so many possible key combinations in the Caddy file. The way I was going about it now was to just declare two possible ENV states, you're either using the JWT_SECRET or JWT_PUBLIC_KEY and it returns 401 if either both are set or the public key doesn't look like PEM format. By determining the configuration based on the environment variables you're only in RSA mode in the event that you have one public key set and no conflicting information. From then on it will assert that the "alg" header is always RS family to prevent the attack.

I think once you get to the point where you need to be able to rotate keys, use multiple algorithms, invalidate keys, etc you're beyond what Caddy was designed for and in the custom middleware realm in your downstream app.

Using ENV at least lets you change keys from the outside should you need to immediately invalidate the public key without reloading Caddy.

This isn't a huge change, I just haven't had time or inclination to work on it lately since I'm not using the RS algorithms.

@namsral
Copy link
Contributor

namsral commented Jul 19, 2016

I concur, supporting more complex JWT configurations wouldn't fit the Caddy mindset.

@lsjostro
Copy link
Contributor

PR #15 adds RSA support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants