Add support for JWT authentication #732

krzysztof-indyk · 2018-10-25T23:53:19Z

Changes proposed in this pull request:

For swagger 2.0 add support for x-authentication-scheme=bearer in apiKey method.
For OpenAPI 3.0 add support for scheme=bearer in http method
Use x-bearerInfoFunc in security definition or BEARERINFO_FUNC env variable to pass a reference to token validation function

dtkav · 2018-10-27T00:47:46Z

examples/openapi3/jwt/app.py

+from jose import JWTError, jwt
+
+JWT_ISSUER = 'com.zalando.connexion'
+JWT_SECRET = '872f65270a71ca913766335276fb2c1b76b8ccdef749cdfc9a1598ac36d77c99'


Can you make this something like not_really_secret? I can imagine some poor security engineer thinking these are leaked creds.

dtkav · 2018-10-27T00:50:45Z

examples/openapi3/jwt/app.py

+    try:
+        return jwt.decode(token, JWT_SECRET, algorithms=[JWT_ALGORITHM])
+    except JWTError as e:
+        raise Unauthorized from e


I don't think raise ... from is python 2.7 compatible.
You probably need to use six.raise_from(...)

@dtkav It's not supposed to run on 2.7, please look at shebang.
BTW end of python 2.7 is coming :)

It's not a matter of my personal preference (I'm also on python 3). Connexion explicitly supports python 2.7, so I think making the examples backwards compatible is the right thing to do.

If you don't want to do it, I'm happy to. I'd prefer to follow the rule of least surprise now, because otherwise I'll have to respond to a ticket about this example being "broken" later.

I have made it python 2.7 compatible.

But it looks like helloworld example is also broken on 2.7

dtkav · 2018-10-27T00:53:42Z

tests/fixtures/secure_endpoint/openapi.yaml

+      type: http
+      scheme: bearer
+      bearerFormat: JWT
+      x-bearerInfoFunc: fakeapi.hello.jwt_info


github is complaining about missing newline at end of file

dtkav · 2018-10-27T00:56:18Z

great work! I had a few minor comments, but otherwise this is looking really good :)

krzysztof-indyk · 2018-10-27T21:49:43Z

@dtkav I have fixed the issues.

dtkav · 2018-10-28T00:41:21Z

Thanks @krise3k Good call, I'm happy to fix the helloworld example in a separate diff.

…

On Sat, Oct 27, 2018, 4:37 PM krise3k ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In examples/openapi3/jwt/app.py <#732 (comment)>: > + timestamp = _current_timestamp() + payload = { + "iss": JWT_ISSUER, + "iat": int(timestamp), + "exp": int(timestamp + JWT_LIFETIME_SECONDS), + "sub": str(user_id), + } + + return jwt.encode(payload, JWT_SECRET, algorithm=JWT_ALGORITHM) + + +def decode_token(token): + try: + return jwt.decode(token, JWT_SECRET, algorithms=[JWT_ALGORITHM]) + except JWTError as e: + raise Unauthorized from e I have made it python 2.7 compatible. But it looks like helloworld example <https://github.com/zalando/connexion/blob/dev-2.0/examples/openapi3/helloworld/hello.py> is also broken on 2.7 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#732 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlPSTybvfh5EV27pI4rnD_Zkx_5mq1pks5upO47gaJpZM4X7TdC> .

dtkav

Nice work @krise3k !

dtkav · 2018-10-28T19:16:56Z

@jmcs do you want to take a look as well, or are you happy for me to merge this?

Stranger6667 · 2018-10-29T07:20:50Z

connexion/decorators/security.py

+        try:
+            auth_type, token = authorization.split(None, 1)
+        except ValueError:
+            raise OAuthProblem(description='Invalid authorization header')


Is OAuthProblem class appropriate here? The code seems more generic and not only about OAuth

Previously connexion only supported oauth. It's more generic now. We could rename the Exception, but it would be backward incompatible if someone has set up a custom error_handler.

You could also inherit from OAuthProblem in order to rename it and keep it backwards compatible. (and leave a note to change this on the next major version bump).

cziebuhr · 2018-10-29T16:13:23Z

Not sure if I missed something. Isn't this the same as oauth2 with x-tokenInfoFunc set to decode_token and an empty scope?

krzysztof-indyk · 2018-10-31T22:00:33Z

@cziebuhr When I have implemented JWT I found that verify function is a closure and guessed that it would be hard to avoid code duplication.
Today I take a second look at the code and extract common function to verify oauth and bearer tokens.

cziebuhr · 2018-11-01T10:43:29Z

Looks better now, but I still don't get the difference. IMHO the only difference is, that scope is not validated. Can't you achieve the same result when specifying oauth2 as type of the security scheme and an empty array as scope for security requirement?

dtkav · 2018-11-05T21:58:02Z

I'm a bit out of the loop on this one, but It seems valuable to be able to define the correct security scheme in the spec, and not have to make everything work through oauth2. ~~In addition, there may be a use case for someone supporting multiple security schemes, so they may need a handler for each one.~~
@cziebuhr thoughts?

cziebuhr · 2018-11-05T22:09:53Z

One can't configure bearer method and oauth2 together, because if the first validator finds an Authorization header of type Bearer and can't validate its value, it will fail hard and not continue with the next validator.

dtkav · 2018-11-06T00:28:46Z

@cziebuhr I'm not sure if you have blocking feedback for the PR, and if so how @krise3k can address your feedback.

I now get your point about oauth2 and http:bearer both using the Bearer keyword in the header. Thanks for clarifying that.

Still, the 3.x.x spec [0] supports http:bearer, so it seems wise to support it as well, even if it's just for the principle of least surprise (people wouldn't expect that they need to change auth methods in their spec to make auth work, even if they are similar/subsets/supersets).

AFAICT this diff adds support for the http:bearer options in the spec, and seems pretty DRY.

[0] https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#security-scheme-object

dtkav · 2018-11-06T00:43:00Z

I tried to update this PR to be against master (as connexion 2.0 was released), but it didn't work as cleanly as I expected (sorry!). @krise3k do you mind rebasing your changes onto master?

cziebuhr · 2018-11-06T20:10:55Z

@cziebuhr I'm not sure if you have blocking feedback for the PR, and if so how @krise3k can address your feedback.

I'm fine now.

krzysztof-indyk · 2018-11-06T22:44:35Z

@dtkav indeed, rebase was not so obvious. It was easier to start from current master and cherry pick the commits, I also squash them.

dtkav · 2018-11-07T00:00:42Z

Great - thanks @krise3k and sorry about that.
I think the last eyes are probably @jmcs then.

jmcs · 2018-11-07T06:59:15Z

Can you expand the documentation and add doctstrings to the new functions?

dtkav · 2018-11-09T22:27:04Z

Hey @krise3k - do you think you'll have time to update the docs?
Let me know if I can help.

krzysztof-indyk · 2018-11-10T07:54:33Z

@dtkav I will do it this weekend

krzysztof-indyk · 2018-11-11T18:40:37Z

@jmcs, @dtkav I push the changes.

jmcs · 2018-11-12T07:41:36Z

👍

zout · 2019-09-16T08:08:50Z

Hi @krise3k,

We are using your JWT code, and it works great, many thanks for your work. However, we are using scopes in the JWT token payload. In your example code you have set a scope that does nothing. We would like to contribute code for scope checking. Is there any reason you have skipped this? And is there any particular problem with that that we need to solve?

krzysztof-indyk · 2019-10-11T14:09:39Z

@zout it's because I didn't need to use scopes, also I wanted to make this example as simple as possible.

bjoluc · 2020-03-24T11:44:02Z

@zout Do you still want to approach this? "Scope" (or should we say "claim"?) checking would be really helpful for us too. I can take a look at this otherwise.

zout · 2020-03-24T13:28:53Z

@bjoluc We have build it in our project now, but as separate functions that decode de JWT to check for scope in the handling function. I could give you that code. But it is quite challenging to get to the scopes that are listed in the yaml file. So we decided not to do that.

bjoluc · 2020-03-24T15:31:38Z

@zout Thanks for the quick reply! Yeah, we ended up implementing a custom solution too. Just thought it would be really nice to support a claims validation function (would have saved me a lot of time). I will try to figure it out and mention you in the PR in case I succeed...

jmcs mentioned this pull request Oct 26, 2018

Connexion 2.0 #619

Merged

dtkav reviewed Oct 27, 2018

View reviewed changes

krzysztof-indyk force-pushed the jwt branch from 0737420 to 8e1629b Compare October 27, 2018 21:40

spec-first deleted a comment Oct 27, 2018

dtkav mentioned this pull request Oct 27, 2018

Finalize 2.0 Release #678

Closed

krzysztof-indyk force-pushed the jwt branch from 6169caf to f797708 Compare October 27, 2018 23:34

spec-first deleted a comment Oct 28, 2018

dtkav approved these changes Oct 28, 2018

View reviewed changes

dtkav requested a review from jmcs October 28, 2018 19:17

Stranger6667 reviewed Oct 29, 2018

View reviewed changes

spec-first deleted a comment Nov 1, 2018

dtkav changed the base branch from dev-2.0 to master November 6, 2018 00:29

luizirber mentioned this pull request Nov 6, 2018

Describe token auth in the API once connexion supports it sourmash-bio/wort#10

Closed

spec-first deleted a comment Nov 6, 2018

krzysztof-indyk added 2 commits November 6, 2018 23:23

Add support for JWT

04216e6

Add example for JWT

b5c9d98

Fix linter errors

bf59ef0

krzysztof-indyk force-pushed the jwt branch from 69103ef to bf59ef0 Compare November 6, 2018 22:29

spec-first deleted a comment Nov 6, 2018

krzysztof-indyk added 2 commits November 11, 2018 19:36

Add minimal JWT documentation

ff3baa3

Add docstrings

281d4c6

spec-first deleted a comment Nov 11, 2018

jmcs and others added 2 commits November 12, 2018 08:38

Merge branch 'master' into jwt

417dc10

Fix title in docs

47c1d3c

jmcs approved these changes Nov 12, 2018

View reviewed changes

jmcs merged commit 6ec1182 into spec-first:master Nov 12, 2018

krzysztof-indyk deleted the jwt branch November 12, 2018 20:48

dtkav mentioned this pull request Dec 17, 2018

Connexion jwt support #607

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for JWT authentication #732

Add support for JWT authentication #732

krzysztof-indyk commented Oct 25, 2018

dtkav Oct 27, 2018

dtkav Oct 27, 2018

krzysztof-indyk Oct 27, 2018

dtkav Oct 27, 2018

krzysztof-indyk Oct 27, 2018

dtkav Oct 27, 2018

dtkav commented Oct 27, 2018

krzysztof-indyk commented Oct 27, 2018

dtkav commented Oct 28, 2018 via email

dtkav left a comment

dtkav commented Oct 28, 2018

Stranger6667 Oct 29, 2018

cziebuhr Oct 29, 2018

dtkav Nov 6, 2018

cziebuhr commented Oct 29, 2018 •

edited

Loading

krzysztof-indyk commented Oct 31, 2018

cziebuhr commented Nov 1, 2018

dtkav commented Nov 5, 2018 •

edited

Loading

cziebuhr commented Nov 5, 2018

dtkav commented Nov 6, 2018

dtkav commented Nov 6, 2018

cziebuhr commented Nov 6, 2018

krzysztof-indyk commented Nov 6, 2018

dtkav commented Nov 7, 2018

jmcs commented Nov 7, 2018

dtkav commented Nov 9, 2018

krzysztof-indyk commented Nov 10, 2018

krzysztof-indyk commented Nov 11, 2018

jmcs commented Nov 12, 2018

zout commented Sep 16, 2019

krzysztof-indyk commented Oct 11, 2019

bjoluc commented Mar 24, 2020

zout commented Mar 24, 2020

bjoluc commented Mar 24, 2020

Add support for JWT authentication #732

Add support for JWT authentication #732

Conversation

krzysztof-indyk commented Oct 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtkav commented Oct 27, 2018

krzysztof-indyk commented Oct 27, 2018

dtkav commented Oct 28, 2018 via email

dtkav left a comment

Choose a reason for hiding this comment

dtkav commented Oct 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cziebuhr commented Oct 29, 2018 • edited Loading

krzysztof-indyk commented Oct 31, 2018

cziebuhr commented Nov 1, 2018

dtkav commented Nov 5, 2018 • edited Loading

cziebuhr commented Nov 5, 2018

dtkav commented Nov 6, 2018

dtkav commented Nov 6, 2018

cziebuhr commented Nov 6, 2018

krzysztof-indyk commented Nov 6, 2018

dtkav commented Nov 7, 2018

jmcs commented Nov 7, 2018

dtkav commented Nov 9, 2018

krzysztof-indyk commented Nov 10, 2018

krzysztof-indyk commented Nov 11, 2018

jmcs commented Nov 12, 2018

zout commented Sep 16, 2019

krzysztof-indyk commented Oct 11, 2019

bjoluc commented Mar 24, 2020

zout commented Mar 24, 2020

bjoluc commented Mar 24, 2020

cziebuhr commented Oct 29, 2018 •

edited

Loading

dtkav commented Nov 5, 2018 •

edited

Loading