auth: support "authorization" token for grpc-gateway #7999

hexfusion · 2017-05-28T22:01:37Z

The allows native use of grpc-gateway authorization header, providing direct access to metadata context while using intuitive naming.

Fixes #6643

heyitsanthony · 2017-05-30T16:40:36Z

auth/store.go

-	if !uok {
-		plog.Warningf("invalid auth token: %s", token)
-		return nil, ErrInvalidAuthToken
+	for k := range md {


avoid looping over all the metadata?

token := "" if token, ok = md["token"]; !ok { token = md["authorization"] } if token == "" { return nil, nil } authInfo, uok := as.authInfoFromToken(ctx, token) ...

@heyitsanthony 👍

heyitsanthony · 2017-05-30T16:41:14Z

Documentation/dev-guide/apispec/swagger/rpc.swagger.json

    "http",
    "https"
  ],
+  "securityDefinitions" : {


will this be clobbered after running scripts/genproto.sh?

@heyitsanthony yes, please excuse my ignorance I am just learning how all of this tooling works together. I will research proper solution further.

xiang90 · 2017-05-30T23:02:07Z

auth/store.go

+				plog.Warningf("invalid auth token: %s", token)
+				return nil, ErrInvalidAuthToken
+			}
+			plog.Debugf("checking value in loop %s", k)


if we do want the debugging logging here, it needs to contain more context. checking value in a loop is too vague.

@xiang90 yes sir. I will clean this up on my next run, I appreciate the notes.

mitake · 2017-06-01T22:55:00Z

auth/store.go

-		plog.Warningf("invalid auth token: %s", token)
-		return nil, ErrInvalidAuthToken
+	for k := range md {
+		if k == "token" || k == "authorization" {


The already existing key "token" is used because I defined it in here: https://github.com/coreos/etcd/blob/master/clientv3/client.go#L168
If "authorization" cannot be changed because of swagger, I think changing the existing "token" would be good in the future. It breaks compatibility so it cannot be done immediately. Possibly putting a comment of todo would be fine?

@mitake grpc-gateway for now only allows direct access to authorization header. This may change if you follow issue. Otherwise I think that token could be used if you passed header Grpc-Metadata-Token but this is not very intuitive. grpc-ecosystem/grpc-gateway#311

So perhaps time will resolve this question. In the meantime I will add your comment

Yes, I don't think unifying the key names immediately. Just putting a comment or opening an issue for a remainder is ok.

mitake · 2017-06-01T22:56:04Z

@hexfusion thanks a lot for your PR. I think your change can support jwt tokens so the commit title shouldn't say about simple token explicitly.

hexfusion · 2017-06-01T23:45:44Z

@mitake well I can see that adding these security definitions to the root of swagger is a complicated matter to automate, Currently proto files do not support the definition directly. So using the methods offered by grpc-gateway will not facilitate.

I have found ways of merging swagger json or defining a json/yml stub using go-swagger but this is not ideal either. I would like this to be seamless and not a hack of genproto.sh but that maybe the only option until you decide to move to OpenApi 3 and perhaps protobuf adds support for security definitions compatibe with swagger. My final solution involved using https://goswagger.io/generate/spec/meta.html. This seems interesting concept for automation but again not optimal.

But this is only my discovery if you have other ways please let me know. I will keep digging for now :)

heyitsanthony · 2017-06-02T02:32:25Z

@hexfusion a hack in genproto.sh that inserts the swagger snippet into the generated json should be OK, the script is already doing some gross stuff to get around inadequate/impossible tooling so the seamless ship has sailed on that one.

Could this also have a curl e2e test like the ones in https://github.com/coreos/etcd/blob/master/e2e/v3_curl_test.go? It'd help as an example for how to use auth with grpc/json in addition to checking it all works via CI.

hexfusion · 2017-06-06T21:07:14Z

@mitake I created a Go script to append swagger security stubs using https://github.com/go-openapi tools in a programmatic way. Although I could not find a clean way to do this directly using proto files, I feel this is a fair compromise. The downside is the spec tools use a different order then method used by grpc-gateway so the diff is quite large. But this order should stay consistent on future iterations. If this seems reasonable I will finish the curl tests. Finally to deal with missing descriptions as a result of the process I resulted to appending (empty) vs "".

heyitsanthony · 2017-06-08T02:47:41Z

@hexfusion please rebase this patchset? It's difficult to see how the code differs from master and review.

The approach looks OK but the schwag package should probably checkout to a fixed commit SHA like the other go get packages in the script.

mitake · 2017-06-08T04:49:33Z

@hexfusion I see, now the script/genproto.sh is updated so I think your way would be acceptable. Do you have any opinions about the change? @xiang90 @heyitsanthony

heyitsanthony · 2017-06-09T22:13:13Z

@hexfusion OK, this is shaping up. Two last nits:

This would be good as just two separate patches:

auth: support "authorization" token for grpc-gateway (with the auth/store.go changes)
scripts: generate swagger with authorization support (with the scripts/* and swagger json files)

Need a test case in e2e/v3_curl_test.go that tries to access etcd using auth through curl.

Thanks!

hexfusion · 2017-06-10T16:36:44Z

@heyitsanthony PTAL, thanks!

heyitsanthony · 2017-06-12T21:32:50Z

e2e/v3_curl_test.go

+	check(t, err)
+
+	cURLRes, err := proc.ExpectFunc(lineFunc)
+	check(t, err)


testutil.AssertNil(t, err)

heyitsanthony · 2017-06-12T21:33:24Z

e2e/v3_curl_test.go

+	check(t, err)
+
+	jerr := json.Unmarshal([]byte(cURLRes), &authRes)
+	check(t, jerr)


testutil.AssertNil(t, json.Unmarshall([]byte(cURLRes), &authRes))

@heyitsanthony thanks for notes, will resolve.

heyitsanthony · 2017-06-12T21:33:54Z

e2e/v3_curl_test.go

+
+	// put "bar" into "foo"
+	putreq, err := json.Marshal(&pb.PutRequest{Key: []byte("foo"), Value: []byte("bar")})
+	check(t, err)


testutil.AssertNil(t, err)

heyitsanthony · 2017-06-12T21:34:23Z

e2e/v3_curl_test.go

+
+	// create root user
+	userreq, err := json.Marshal(&pb.AuthUserAddRequest{Name: string("root"), Password: string("toor")})
+	check(t, err)


testutil.AssertNil(t, err)

heyitsanthony · 2017-06-12T21:34:38Z

e2e/v3_curl_test.go

+
+	// create root role
+	rolereq, err := json.Marshal(&pb.AuthRoleAddRequest{Name: string("root")})
+	check(t, err)


testutil.AssertNil(t, err)

heyitsanthony · 2017-06-12T21:35:34Z

e2e/v3_curl_test.go

+	)
+	cmdArgs = cURLPrefixArgs(epc, "POST", cURLReq{endpoint: "/v3alpha/auth/authenticate", value: string(authreq)})
+	proc, err := spawnCmd(cmdArgs)
+	check(t, err)


testutil.AssertNil(t, err)

heyitsanthony · 2017-06-12T21:35:42Z

e2e/v3_curl_test.go

+
+	// auth request
+	authreq, err := json.Marshal(&pb.AuthenticateRequest{Name: string("root"), Password: string("toor")})
+	check(t, err)


testutil.AssertNil(t, err)

heyitsanthony · 2017-06-12T21:37:38Z

e2e/v3_curl_test.go

 	}
 }
+
+type AuthResponse struct {


use etcdserverpb.AuthenticationResponse instead of redeclaring?

heyitsanthony · 2017-06-12T21:38:41Z

e2e/v3_curl_test.go

 // You may obtain a copy of the License at
 //
-//     http://www.apache.org/licenses/LICENSE-2.0
+//	   http://www.apache.org/licenses/LICENSE-2.0


heyitsanthony · 2017-06-12T21:38:56Z

e2e/v3_curl_test.go

 	"github.com/grpc-ecosystem/grpc-gateway/runtime"
 )

+func check(t *testing.T, err error) {


can replace with testutil.AssertNil

hexfusion · 2017-06-13T12:21:32Z

@heyitsanthony could you please help me better understand the root of this problem?

https://play.golang.org/p/o-s0_LI9Tr

gyuho · 2017-06-13T15:55:22Z

@hexfusion Try without double-quotes on numbers inside JSON?

e.g.

{"header":{"cluster_id":8146361247912014845,"member_id":14895263327052482431,...

hexfusion · 2017-06-13T18:17:12Z

@gyuho right but this is the JSON format returned by curl in the test. I don't believe I can control this format it returns this same quoting if I were to perform the curl command manually.

Appending json:",string" allows this usage, but this I believe would need to be a manual grooming as I don't believe it is supported by proto.

https://play.golang.org/p/W0KBfT_bM5

I guess the main issue is https://developers.google.com/protocol-buffers/docs/proto3#json

The uint64 and int64 types are json strings not numbers.

This is very frustrating :)

heyitsanthony · 2017-06-13T19:06:31Z

there was some discussion about json output w/r/t 64-bit values on #7396; the decision at the time was to stick to the defaults

hexfusion · 2017-06-13T19:17:21Z

@heyitsanthony I understand, so could I groom the JSON manually for the test or how to proceeded?

hexfusion · 2017-06-13T19:54:56Z

perhaps something like this?

    cURLRes, err := proc.ExpectFunc(lineFunc)
    testutil.AssertNil(t, err)

    authRes := make(map[string]interface{})
    testutil.AssertNil(t, json.Unmarshal([]byte(cURLRes), &authRes))

    token, ok := authRes["token"].(string)
    if !ok {
        t.Fatalf("failed invalid token in authenticate response with curl")
    }

heyitsanthony · 2017-06-13T20:06:41Z

@hexfusion yeah that looks OK

hexfusion · 2017-06-14T22:36:22Z

@heyitsanthony PTAL, let me know if you need anything else. Thanks!

heyitsanthony

lgtm thanks!

heyitsanthony · 2017-06-14T22:49:02Z

@hexfusion can that Merge branch 'master' into grpc-gateway-auth commit be removed? Then this should be good to merge.

Defer to @mitake

hexfusion · 2017-06-14T23:15:22Z

@heyitsanthony @mitake ok I rebased but it shows a basic conflict that I resolved causing the merge message last time. So maybe that can be resolved during merge? The change is very obvious. Anyways thanks to all for the assistance and input, very exciting!

heyitsanthony · 2017-06-14T23:57:36Z

@hexfusion can you rebase on the current master branch? You should be able to resolve the conflict from there (the git SHAs were updated for a newer protobuf version)

codecov-io · 2017-06-15T00:18:58Z

Codecov Report

❗ No coverage uploaded for pull request base (master@ee0c805). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master   #7999   +/-   ##
========================================
  Coverage          ?   76.8%           
========================================
  Files             ?     342           
  Lines             ?   26701           
  Branches          ?       0           
========================================
  Hits              ?   20508           
  Misses            ?    4751           
  Partials          ?    1442

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee0c805...b19e2fa. Read the comment docs.

hexfusion · 2017-06-15T00:22:29Z

@heyitsanthony you sir are correct, resolved.

heyitsanthony · 2017-06-15T17:35:02Z

scripts/genproto.sh

-GRPC_GATEWAY_SHA="18d159699f2e83fc5bb9ef2f79465ca3f3122676"
+# exact version of packages to build
+GOGO_PROTO_SHA="8d70fb3182befc465c4a1eac8ad4d38ff49778e2"
+GRPC_GATEWAY_SHA="84398b94e188ee336f307779b57b3aa91af7063c"


GOGO_PROTO_SHA="100ba4e885062801d56799d78530b73b178a78f3" GRPC_GATEWAY_SHA="18d159699f2e83fc5bb9ef2f79465ca3f3122676"

otherwise clobbers update from 4ebeba0

hexfusion · 2017-06-15T17:44:19Z

@heyitsanthony yes I updated, PTAL, thank you!

xiang90 · 2017-06-16T02:21:51Z

@hexfusion thanks. merging.

davissp14 · 2017-08-22T16:01:16Z

Hey guys, are you guys planning on rolling this into 3.2.7?

heyitsanthony · 2017-08-22T16:05:06Z

@davissp14 it's planned for 3.3; backports are tagged with kind/need-backport

hexfusion mentioned this pull request May 28, 2017

Question: grpc-gateway non gRPC auth. #6643

Closed

heyitsanthony suggested changes May 30, 2017

View reviewed changes

xiang90 reviewed May 30, 2017

View reviewed changes

mitake reviewed Jun 1, 2017

View reviewed changes

hexfusion changed the title ~~grpc-gateway: Support simple token via authorization header.~~ grpc-gateway: Support token via authorization header. Jun 2, 2017

gyuho force-pushed the master branch 2 times, most recently from 44ca396 to 4301f49 Compare June 2, 2017 15:53

hexfusion force-pushed the grpc-gateway-auth branch 2 times, most recently from eff9df1 to 67de94b Compare June 9, 2017 02:18

hexfusion force-pushed the grpc-gateway-auth branch 2 times, most recently from 14d2326 to 6c4dfff Compare June 10, 2017 16:35

hexfusion changed the title ~~grpc-gateway: Support token via authorization header.~~ auth: support "authorization" token for grpc-gateway Jun 10, 2017

heyitsanthony suggested changes Jun 12, 2017

View reviewed changes

hexfusion force-pushed the grpc-gateway-auth branch 2 times, most recently from a8a2e20 to 5b1a74f Compare June 13, 2017 20:38

heyitsanthony approved these changes Jun 14, 2017

View reviewed changes

hexfusion force-pushed the grpc-gateway-auth branch from b4ea954 to 5b1a74f Compare June 14, 2017 23:12

hexfusion force-pushed the grpc-gateway-auth branch from 15c6baa to 5b1a74f Compare June 15, 2017 00:09

auth: support "authorization" token for grpc-gateway

0caab26

hexfusion force-pushed the grpc-gateway-auth branch from 13bca87 to b19e2fa Compare June 15, 2017 00:20

hexfusion force-pushed the grpc-gateway-auth branch from b19e2fa to 5d869e0 Compare June 15, 2017 10:54

heyitsanthony reviewed Jun 15, 2017

View reviewed changes

hexfusion added 2 commits June 15, 2017 13:41

scripts: generate swagger with authorization support

e5aa938

e2e: test auth over grpc json

c27634c

hexfusion force-pushed the grpc-gateway-auth branch from 5d869e0 to c27634c Compare June 15, 2017 17:42

xiang90 merged commit 44a6c21 into etcd-io:master Jun 16, 2017

hexfusion mentioned this pull request Jun 17, 2017

Add full Auth support hexfusion/perl-net-etcd#18

Closed

heyitsanthony mentioned this pull request Aug 14, 2017

Can't use http api to get data from a authorized cluster #8398

Closed

hexfusion mentioned this pull request Jul 27, 2018

permission denied using grpc gateway through authentication (etcd version 3.0.10) #9959

Closed

pacoxu mentioned this pull request Mar 28, 2019

add (python2.7 and python3.5+) Python client for etcd v3, using gRPC-… #10593

Merged

18 tasks

fuweid mentioned this pull request Apr 26, 2023

chore: deprecate github.com/hexfusion/schwag #15779

Merged

auth: support "authorization" token for grpc-gateway #7999

auth: support "authorization" token for grpc-gateway #7999

Uh oh!

Conversation

hexfusion commented May 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hexfusion Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitake commented Jun 1, 2017

Uh oh!

hexfusion commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

heyitsanthony commented Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hexfusion commented Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

heyitsanthony commented Jun 8, 2017

Uh oh!

mitake commented Jun 8, 2017

Uh oh!

heyitsanthony commented Jun 9, 2017

Uh oh!

hexfusion commented Jun 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hexfusion commented Jun 13, 2017

Uh oh!

gyuho commented Jun 13, 2017

Uh oh!

hexfusion commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

heyitsanthony commented Jun 13, 2017

Uh oh!

hexfusion commented Jun 13, 2017

Uh oh!

hexfusion commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

hexfusion Jun 1, 2017 •

edited

Loading

hexfusion commented Jun 1, 2017 •

edited

Loading

heyitsanthony commented Jun 2, 2017 •

edited

Loading

hexfusion commented Jun 6, 2017 •

edited

Loading

hexfusion commented Jun 13, 2017 •

edited

Loading

hexfusion commented Jun 13, 2017 •

edited

Loading

heyitsanthony commented Jun 14, 2017 •

edited

Loading

codecov-io commented Jun 15, 2017 •

edited

Loading

hexfusion commented Jun 15, 2017 •

edited

Loading

hexfusion commented Jun 15, 2017 •

edited

Loading

heyitsanthony commented Aug 22, 2017 •

edited

Loading