Add jwtauth middleware

[c0ze]

This adds JWT authentication for all API routes, adding as a middleware.

One problem, when doing a route call, two tokens will clash. @kunihiko-t any ideas for this ? Maybe we can set a specific Realm ? Bearer ?

todos :

• deal with routes API auth token clash (? maybe no need ?)
• add full test suite with auth
• add full test suite with broken auth
• add some kind of integration testing with fn
• add some kind of integration testing with fn with auth

[vasilev]

Please apply gofmt to this file.

[c0ze]

ok thanks, done !

[kunihiko-t]

Hi, Arda!

Yes, we need "Bearer" at the beginning of Authorization header.
Please refer following line.

functions/fn/commands/routes.go

Line 265 in c3d25a9

req.Header.Set("Authorization", fmt.Sprintf("Bearer %v", ss))

Regard to client side, how about having JWT_AUTH_KEY in the configuration file?

e.g. ~/.ironfconfig.yml when the user has ~/.ironfconfig.yml override the former.

[kunihiko-t]

Hi, @c0ze.

I think I had misunderstood about what you asked.

As for two token clash, middleware authenticates /v1/* access using with “JWT_AUTH_KEY” env variable.
For app authentication, it works using with “jwt_key” in YAML file specified by user, and authenticate the request which doesn’t have /v1/ prefix.
So I think it won’t clash. But to avoid confusion, we need to consider an env variable name.

In addition What do you think about removing IRON_TOKEN?
It will clash with jwt token because both of them using same header.
I searched source files, but I cannot find any function using IRON_TOKEN.

I also sent a message in Slack :)

[c0ze]

@kunihiko-t thanks for the feedback !

Hmm, yes in that case it won't clash, so that's good news ! I am still looking for a good way to test this...

Yeah, I think IRON_TOKEN was somekind of experimental feature which we can remove.

[c0ze]

@kunihiko-t I modified FullStackTest to test jwt authentication (with valid and invalid tokens).

Now I'm looking for some kind of integration testing with the fn tool.

[c0ze]

hello @vasilev @kunihiko-t ,

I added integration tests to this PR. I would appreciate if you could take a look !

This is necessary for #663 because I am planning to use a similar testing harness, so would like to merge this in first.

Thank you !

[kunihiko-t]

Hello, @c0ze.
Great!!

I've tried to run some test with make test-tag command and I caught errors.

I think /api/server/server_test.go should have
// +build server full_stack
instead of

// +build full_stack
// +build server

According to this document, multiple lines represent AND condition
https://golang.org/pkg/go/build/#hdr-Build_Constraints

Regarding test with server tag, /api/server/server_auth_test.go needs server tag for required functions.
Also /api/server/apps_test.go needs full_stack tag for include setLogBuffer function.

So I think following files should have // +build server full_stack on top.

api/server/apps_test.go
api/server/runner_test.go
api/server/server_auth_test.go
api/server/server_test.go

But If we change those files, make test TAG=server will run some full stack tests.
So should we move

functions/api/server/server_auth_test.go

Line 59 in 9315060

func setJwtAuth(req *http.Request) {

to another file?

Thanks :)

[c0ze]

@kunihiko-t

Thank you for the review !

Instead of writing a big review, can you please mention individual problems inline with the changes ? I think it is the usual github review procedure. I will give you an example.

[c0ze]

@kunihiko-t

for example, like this,

According to this document, multiple lines represent AND condition
https://golang.org/pkg/go/build/#hdr-Build_Constraints

It is easier to follow context this way.

Thank you !

[c0ze]

About test tags, actually, I think I will remove server tag. I introduced full_stack and integration tags to test quickly, but I don't think there is need for a server tag.

[c0ze]

@kunihiko-t I removed full_stack tag instead of server tag. Now you can run both and also with no tags and everything should be passing. What do you think ?

[c0ze]

there is a leftover file at /tmp/func_test_bolt.db after tests are run. It is not a big problem , but little bit annoying.

1. If you run make test file is there.
2. If you run with server tag, it is not there (I modified teardown functions to destroy it)
3. If you run with integration tag, it is also there.

probably the teardown function is not being called somewhere (or overwritten ? but it is not possible in Go I think) I'm not sure what's going on, hmmm ...

[vasilev]

Thank you for tests. Code looks good to me.

[kunihiko-t]

@c0ze It works! Thanks!
Regarding /tmp/func_test_bolt.db, teardown func which has os.Remove is here (

functions/api/server/server_test.go

Line 131 in b562394

os.Remove(tmpBolt)

) and it has server tag.
Therefore it is called when we run make test-tag TAG=server .

When we run with make test-tag TAG=integration or make test it won't be included on test.
So I think os.Remove should be put on the another file.

[c0ze]

@kunihiko-t I see, thank you for the analysis.

However, I don't agree with your proposal. tmpBolt is local to server tests, so it's creation and disposal should be handled in the same file, server_test.

Instead, I will try to find why it is being created in the first place, when tests are called without server tag. I think it is better approach.

[c0ze]

ok, I found it. It was being created in one of the datastore tests.