Move authentication to grpc interceptor by gdbelvin · Pull Request #973 · google/keytransparency

gdbelvin · 2018-05-10T12:33:52Z

This PR supports greater flexibility for deployment specific authentication schemes by moving authentication from the core/ code base to an impl/ specific grpc interceptor which can easily be swapped out with more appropriate authentication logic in specific deployments.

Changes:

core/authentication moved to impl/authentication
ValidateCreds moved to AuthFunc to match the grpc_auth.AuthFunc interface.
- SecurityContext moved to ValidatedSecurity and is now stored in context.Context
impl/google/authentication merged with impl/authentication to use the new ValidatedSecurity
Removed auth from core/keyserver 😄
Wrote a simple auth interceptor to support per-method authentication configurations.

Now that authentication is happening in a grpc interceptor, we don't need any authentication code in core/. We also get the added benefit of keeping the `validatedSecurityKey` private to the authentication package.

codecov-io · 2018-05-10T12:40:46Z

Codecov Report

Merging #973 into master will increase coverage by 0.17%.
The diff coverage is 38.46%.

@@            Coverage Diff             @@
##           master     #973      +/-   ##
==========================================
+ Coverage   49.55%   49.73%   +0.17%     
==========================================
  Files          28       29       +1     
  Lines        2030     2041      +11     
==========================================
+ Hits         1006     1015       +9     
- Misses        843      846       +3     
+ Partials      181      180       -1

Impacted Files	Coverage Δ
impl/authentication/interceptor.go	`0% <0%> (ø)`
core/keyserver/keyserver.go	`34.89% <0%> (+1.72%)`	⬆️
impl/authentication/google.go	`18.75% <0%> (ø)`
impl/authentication/context.go	`100% <100%> (ø)`
impl/integration/env.go	`73.07% <100%> (+1.64%)`	⬆️
impl/authentication/fake.go	`88.88% <100%> (ø)`
impl/authorization/authorization.go	`82.35% <73.33%> (-9.96%)`	⬇️

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 ae6690b...9e2bcf5. Read the comment docs.

AlCutter · 2018-05-10T13:20:39Z

impl/authentication/google.go

-func (a *GAuth) ValidateCreds(ctx context.Context) (*authentication.SecurityContext, error) {
-	// Get Tokeninfo from credentials.
-	tokenInfo, err := a.validateToken(ctx)
+// AuthFunc authenticate the information present in ctx.


authenticates

AlCutter · 2018-05-10T13:22:33Z

impl/authentication/interceptor.go

+	grpc_auth "github.com/grpc-ecosystem/go-grpc-middleware/auth"
+)
+
+// UnaryServerInterceptor returns a new unary server interceptors that performs per-request auth.


"interceptor that"

AlCutter · 2018-05-10T13:23:11Z

impl/authentication/interceptor.go

+// UnaryServerInterceptor returns a new unary server interceptors that performs per-request auth.
+func UnaryServerInterceptor(authFuncs map[string]grpc_auth.AuthFunc) grpc.UnaryServerInterceptor {
+	return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
+		glog.Infof("In authentication interceptor")


Should this be a V(...).Infof?

This was a debug. I'll remove

AlCutter · 2018-05-10T13:23:27Z

impl/authentication/interceptor.go

+		glog.Infof("In authentication interceptor")
+		authFunc, ok := authFuncs[info.FullMethod]
+		if !ok {
+			glog.Infof("auth interceptor: no hander for %v", info.FullMethod)


Warning / V?

AlCutter · 2018-05-10T13:24:03Z

impl/authentication/interceptor.go

+	}
+}
+
+// StreamServerInterceptor returns a new unary server interceptors that performs per-request auth.


"interceptor that"

AlCutter · 2018-05-10T13:24:55Z

impl/authorization/authorization.go

@@ -38,30 +42,37 @@ func New() authorization.Authorization {
 // authorized to carry the given permission. A call is authorized if:
 //  1. userID matches the identity in sctx,


Does sctx need to change now that SecurityContext is not used?

Updated comment

AlCutter · 2018-05-10T13:26:47Z

impl/authorization/authorization.go

 func resourceLabel(domainID, appID string) string {
-	return fmt.Sprintf("%v|%v", domainID, appID)
+	return fmt.Sprintf("domains/%v/apps/%v",
+		strings.Replace(domainID, "/", "_", -1),


this implies that two resources with differing domains (and/or apps) "blah/blah" and "blah_blah" are treated identically, is that ok?

The / character is not allowed in domainIDs or appIDs. If they were allowed, a specially crafted domainID or appID could cross boundaries. eg domainID = "blah/apps/apptakeover"

Sure, but my point is that there are now multiple "preimages" which result in identical resource labels - this function doesn't enforce the rule that "/"s aren't allowed, it just quietly papers over that situation resulting in the multiple "preimage" thing.
Maybe that's OK, I don't know :)

Is the rule about the contents of domainID and appID enforced elsewhere?
if so this function possibly doesn't need to Sprintf, and if it isn't enforced elsewhere then maybe this function should return an error in the case where a passed-in ID contains the character?

Excellent point. I'll make this return an error.

Martin2112 · 2018-05-10T13:31:56Z

cmd/keytransparency-server/main.go

+		entry.New(), authz, domains, queue, mutations)
 	grpcServer := grpc.NewServer(
 		grpc.Creds(creds),
+		// All streaming methods are unauthenticated for now.


That looks like a TODO to me.

I don't anticipate adding authenticated streaming methods right now.
This comment is a reminder to add the interceptor if we do add an authenticated streaming rpc.

OK, as you're the one who'll have to write the postmortem when that doesn't happen.

I'll add a reference to the streaming interceptor with an empty map to make things clearer.

Martin2112 · 2018-05-10T13:32:41Z

cmd/keytransparency-server/main.go

+		grpc.UnaryInterceptor(grpc_middleware.ChainUnaryServer(
+			grpc_prometheus.UnaryServerInterceptor,
+			authentication.UnaryServerInterceptor(map[string]grpc_auth.AuthFunc{
+				"/google.keytransparency.v1.KeyTransparency/UpdateEntry": authFunc,


Can we avoid hardcoding this? Presumably there will be a bunch of methods under the same prefix x/y in future.

We could replace this with a regex in the future if we need to, but simpler seems easier to test and maintain for the moment.

I thought I saw another copy of the same string. Literals in multiple places often get overlooked.

Martin2112 · 2018-05-10T13:35:42Z

impl/authentication/context.go

-	ErrMissingAuth = errors.New("auth: missing authentication header")
-)
+// ValidatedSecurity is the auth value stored in the Contexts.
+type ValidatedSecurity struct {


Maybe this should be called SecurityContext, or the key renamed so they match.

Martin2112 · 2018-05-10T13:36:09Z

impl/authentication/fake_test.go

@@ -0,0 +1,56 @@
+// Copyright 2016 Google Inc. All Rights Reserved.


Time passes ... Thorin sings about gold.

Martin2112 · 2018-05-10T13:37:09Z

impl/authentication/fake_test.go

+		wantCode  codes.Code
+	}{
+		{desc: "missing authentication", ctx: ctx, wantCode: codes.Unauthenticated},
+		{desc: "working case", ctx: WithOutgoingFakeAuth(ctx, "foo"), wantEmail: "foo"},


Aren't there more than two cases e.g. corrupt token? There are multiple errors defined below, which makes me think so.

FakeAuthFunc is fairly simple. I'm getting 100% code coverage.

OK we shouldn't really be testing fakes though. Are there tests for the multiple possible errors defined or is that elsewhere?

TestGoogleAuthn tests those code paths.

Martin2112 · 2018-05-10T13:38:42Z

impl/authentication/google.go

 	return diff
 }

+func parseToken(accessToken string) *oauth2.Token {


Does this add much value?

Not really, except that it nicely separates the ideas of parsing and authenticating.

Martin2112 · 2018-05-10T13:39:15Z

impl/authentication/interceptor.go

+// UnaryServerInterceptor returns a new unary server interceptors that performs per-request auth.
+func UnaryServerInterceptor(authFuncs map[string]grpc_auth.AuthFunc) grpc.UnaryServerInterceptor {
+	return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
+		glog.Infof("In authentication interceptor")


Martin2112 · 2018-05-10T13:40:51Z

impl/authorization/authorization.go

 	domainID, appID, userID string, permission authzpb.Permission) error {
+	sctx, ok := authentication.FromContext(ctx)
+	if !ok {
+		return status.Errorf(codes.Unauthenticated, "Request does not contain a ValidatedSecurity object")


The other code returns fixed errors but this doesn't. Should this do the same?

The other fixed errors weren't really needed. I've removed them now.

OK, this possibly addresses my point above.

Martin2112 · 2018-05-10T13:43:27Z

impl/authorization/authorization_test.go

+		sctx, err := authentication.FakeAuthFunc(inCtx)
+		if err != nil {
+			t.Errorf("FakeAuthFunc(): %v", err)
+			continue


maybe t.FailNow() ?

We want to run the rest of the tests :-)
I'll put this in a t.Run()

r.Run() is better.

Martin2112 · 2018-05-10T13:43:55Z

impl/integration/env.go

+	gsvr := grpc.NewServer(
+		grpc.UnaryInterceptor(
+			authentication.UnaryServerInterceptor(map[string]grpc_auth.AuthFunc{
+				"/google.keytransparency.v1.KeyTransparency/UpdateEntry": authFunc,


The integration tests act like a sort of "main", wiring together libraries much like main does.
As a result, this intentionally lives in impl/ and allows other environments to setup their own custom envs

OK if it doesn't rely on those strings matching (my earlier point) then it might not be a problem.

I don't see a place in the generated proto files where these strings are publicly exported. Not sure what the best solution is.

gdbelvin · 2018-05-10T15:53:07Z

PTAL

Martin2112 · 2018-05-11T08:44:18Z

I think the potential only blocker is the point Al made about the preimages.

gdbelvin · 2018-05-11T10:06:00Z

Fixed the resource label pre-image issue.

gdbelvin · 2018-05-11T13:54:53Z

PTAL

* master: Move authentication to grpc interceptor (google#973) Add UnitTest for PaginateHistory (google#968) Remove unused `UserProfile` message (google#972) Update default paths (google#910)

gdbelvin added 5 commits May 10, 2018 10:47

Store ValidatedSecurity inside Context

f68f8e3

Merge core and impl authentication

04b0c5a

Now that authentication is happening in a grpc interceptor, we don't need any authentication code in core/. We also get the added benefit of keeping the `validatedSecurityKey` private to the authentication package.

Use grpc auth interceptor in cmd

0188b98

Use grpc auth interceptor in tests

636ecaf

Create a per-method authentication interceptor

5c4d35d

Apply polish

3333d3f

gdbelvin requested review from AlCutter and Martin2112 May 10, 2018 13:09

AlCutter reviewed May 10, 2018

View reviewed changes

Martin2112 reviewed May 10, 2018

View reviewed changes

Polish with a finer grain

4b56339

gdbelvin mentioned this pull request May 11, 2018

Also move authorization to the grpc interceptor #974

Merged

Disallow '/' in resource labels

d262034

bump go version

5735d41

AlCutter approved these changes May 11, 2018

View reviewed changes

gdbelvin added 2 commits May 11, 2018 19:34

Merge branch 'master' into f/auth/2

2e219ad

Merge branch 'master' into f/auth/2

9e2bcf5

gdbelvin merged commit d1762ed into google:master May 11, 2018

gdbelvin mentioned this pull request May 14, 2018

Move userCreds func to cmd/client #975

Merged

gdbelvin deleted the f/auth/2 branch January 9, 2019 18:08

		@@ -38,30 +42,37 @@ func New() authorization.Authorization {
		// authorized to carry the given permission. A call is authorized if:
		// 1. userID matches the identity in sctx,

		@@ -0,0 +1,56 @@
		// Copyright 2016 Google Inc. All Rights Reserved.

Conversation

gdbelvin commented May 10, 2018

Uh oh!

codecov-io commented May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

gdbelvin May 10, 2018 • 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!

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!

gdbelvin May 10, 2018 • 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!

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!

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

codecov-io commented May 10, 2018 •

edited

Loading

gdbelvin May 10, 2018 •

edited

Loading

gdbelvin May 10, 2018 •

edited

Loading