auth: Make luci-go services trust signatures produced by the token server.

auth: Make luci-go services trust signatures produced by the token server.

Previously luci-go auth library ignored 'issued_by' field and just always
assumed tokens are signed by the auth service.

Now it fetches the certificates of both the auth service and the token server
and picks the correct key based on 'issued_by' field. It matches what luci-py
does.

It enables a migration path from "old" tokens signed by the auth service to
"new" tokens, signed by the token server.

R=maruel@chromium.org, nodir@chromium.org
BUG=

Committed: https://github.com/luci/luci-go/commit/59df90e41b87f38d73e5dde2baa0ed5ba139fe0c

[Vadim Sh., 2016-10-01 04:04:43]

Still need to write tests.

Also depends on
https://github.com/luci/luci-py/commit/6c03cbaa04dd2142326e6c59c9e67f785651e775
being deployed to chrome-infra-auth, or everything will blow up :) Will do it on
Monday.

https://codereview.chromium.org/2386643003/diff/1/server/auth/authdb/snapshot.go
File server/auth/authdb/snapshot.go (right):

https://codereview.chromium.org/2386643003/diff/1/server/auth/authdb/snapshot...
server/auth/authdb/snapshot.go:47: certs lazyslot.Slot
lazyslot.Slot takes care of initial lazy fetch and occasional refresh

https://codereview.chromium.org/2386643003/diff/1/server/auth/authdb/snapshot...
server/auth/authdb/snapshot.go:286: func (db *SnapshotDB) GetCertificates(c
context.Context, signerID identity.Identity) (*signing.PublicCertificates,
error) {
this is the main implementation of GetCertificates. The rest is various mocks.

[M-A Ruel, 2016-10-01 11:18:42]

lgtm

https://codereview.chromium.org/2386643003/diff/1/server/auth/authdb/snapshot.go
File server/auth/authdb/snapshot.go (right):

https://codereview.chromium.org/2386643003/diff/1/server/auth/authdb/snapshot...
server/auth/authdb/snapshot.go:286: func (db *SnapshotDB) GetCertificates(c
context.Context, signerID identity.Identity) (*signing.PublicCertificates,
error) {
On 2016/10/01 04:04:43, Vadim Sh. wrote:
> this is the main implementation of GetCertificates. The rest is various mocks.

Which brings me: why so many mocks?

https://codereview.chromium.org/2386643003/diff/1/server/auth/authdb/snapshot...
server/auth/authdb/snapshot.go:292: return trustedCertsMap[signerID], nil
Humm this will panic if the signerID is missing in the map. Could this happen?

https://codereview.chromium.org/2386643003/diff/1/server/auth/authdb/snapshot...
server/auth/authdb/snapshot.go:349: // Fetch in parallel.
I'm surprised there isn't a package to do this yet. :P

https://codereview.chromium.org/2386643003/diff/1/server/auth/authdb/snapshot...
server/auth/authdb/snapshot.go:352: i := i
optional nit: I prefer passing flags on the function but it's more typing.

I'd recommend to use urls[i] instead of aliasing url, this will likely result in
simpler code since that reduces aliasing.

https://codereview.chromium.org/2386643003/diff/1/server/auth/authdb/snapshot...
server/auth/authdb/snapshot.go:357: wg.Done()
optional nit: I generally prefer
defer wg.Done()
out of habit. Habits are good. :D

[Vadim Sh., 2016-10-03 20:47:12]

(still writing tests)

https://codereview.chromium.org/2386643003/diff/1/appengine/gaeauth/server/db.go
File appengine/gaeauth/server/db.go (right):

https://codereview.chromium.org/2386643003/diff/1/appengine/gaeauth/server/db...
appengine/gaeauth/server/db.go:98: func (devServerDB) SharedSecrets(c
context.Context) (secrets.Store, error) {
This is used on local devserver and only on devserver. It make 'IsMember' calls
return True for any group. Allows to not bother to configure groups on local dev
server.

https://codereview.chromium.org/2386643003/diff/1/server/auth/auth_test.go
File server/auth/auth_test.go (right):

https://codereview.chromium.org/2386643003/diff/1/server/auth/auth_test.go#ne...
server/auth/auth_test.go:151: func (db *fakeDB) IsMember(c context.Context, id
identity.Identity, group string) (bool, error) {
this is the main fake used in server/auth/* tests. It can't import the fake in
server/auth/authtest because of dependency cycle.

https://codereview.chromium.org/2386643003/diff/1/server/auth/authdb/erroring.go
File server/auth/authdb/erroring.go (right):

https://codereview.chromium.org/2386643003/diff/1/server/auth/authdb/erroring...
server/auth/authdb/erroring.go:49: func (db ErroringDB) GetCertificates(c
context.Context, id identity.Identity) (*signing.PublicCertificates, error) {
This is not a fake, but a primitive implementation that returns errors from all
methods.

Used on un-configured prod app. The error is "please configure the auth service
URL".

https://codereview.chromium.org/2386643003/diff/1/server/auth/authdb/snapshot.go
File server/auth/authdb/snapshot.go (right):

https://codereview.chromium.org/2386643003/diff/1/server/auth/authdb/snapshot...
server/auth/authdb/snapshot.go:67: func NewSnapshotDB(authDB *protocol.AuthDB,
authServiceURL string, rev int64) (*SnapshotDB, error) {
this is main prod implementation

https://codereview.chromium.org/2386643003/diff/1/server/auth/authdb/snapshot...
server/auth/authdb/snapshot.go:286: func (db *SnapshotDB) GetCertificates(c
context.Context, signerID identity.Identity) (*signing.PublicCertificates,
error) {
On 2016/10/01 11:18:41, M-A Ruel wrote:
> On 2016/10/01 04:04:43, Vadim Sh. wrote:
> > this is the main implementation of GetCertificates. The rest is various
mocks.
> 
> Which brings me: why so many mocks?

Replied in separate comments. They are all necessary :(

https://codereview.chromium.org/2386643003/diff/1/server/auth/authdb/snapshot...
server/auth/authdb/snapshot.go:292: return trustedCertsMap[signerID], nil
On 2016/10/01 11:18:41, M-A Ruel wrote:
> Humm this will panic if the signerID is missing in the map. Could this happen?

No, it will return 'nil'. Maps don't panic, they return zero-value.

And this can happen, and 'nil' is the correct response. It means "the given
signerID is not trusted". This is documented in DB.GetCertificates comment.

https://codereview.chromium.org/2386643003/diff/1/server/auth/authdb/snapshot...
server/auth/authdb/snapshot.go:349: // Fetch in parallel.
On 2016/10/01 11:18:41, M-A Ruel wrote:
> I'm surprised there isn't a package to do this yet. :P

There probably is one, but I find it simpler to use primitives like WaitGroup
:-/

https://codereview.chromium.org/2386643003/diff/1/server/auth/authdb/snapshot...
server/auth/authdb/snapshot.go:352: i := i
On 2016/10/01 11:18:41, M-A Ruel wrote:
> optional nit: I prefer passing flags on the function but it's more typing.
> 
> I'd recommend to use urls[i] instead of aliasing url, this will likely result
in
> simpler code since that reduces aliasing.

Done.

https://codereview.chromium.org/2386643003/diff/1/server/auth/authdb/snapshot...
server/auth/authdb/snapshot.go:357: wg.Done()
On 2016/10/01 11:18:41, M-A Ruel wrote:
> optional nit: I generally prefer
> defer wg.Done()
> out of habit. Habits are good. :D

Done.

https://codereview.chromium.org/2386643003/diff/1/server/auth/authtest/db.go
File server/auth/authtest/db.go (right):

https://codereview.chromium.org/2386643003/diff/1/server/auth/authtest/db.go#...
server/auth/authtest/db.go:62: func (db FakeDB) GetCertificates(c
context.Context, id identity.Identity) (*signing.PublicCertificates, error) {
this is a publicly usable fake in authtest/* package. It is used by various unit
tests in other packages that need to mock auth stuff. It is unusable from
auth_test.go due to module reference cycle.

[Vadim Sh., 2016-10-03 22:34:07]

The CQ bit was checked by vadimsh@chromium.org

[Vadim Sh., 2016-10-03 22:34:08]

The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org
Link to the patchset: https://codereview.chromium.org/2386643003/#ps40001
(title: "add tests")

[commit-bot: I haz the power, 2016-10-03 22:34:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 23:14:25]

Description was changed from

==========
auth: Make luci-go services trust signatures produced by the token server.

Previously luci-go auth library ignored 'issued_by' field and just always
assumed tokens are signed by the auth service.

Now it fetches the certificates of both the auth service and the token server
and picks the correct key based on 'issued_by' field. It matches what luci-py
does.

It enables a migration path from "old" tokens signed by the auth service to
"new" tokens, signed by the token server.

R=maruel@chromium.org, nodir@chromium.org
BUG=
==========

to

==========
auth: Make luci-go services trust signatures produced by the token server.

Previously luci-go auth library ignored 'issued_by' field and just always
assumed tokens are signed by the auth service.

Now it fetches the certificates of both the auth service and the token server
and picks the correct key based on 'issued_by' field. It matches what luci-py
does.

It enables a migration path from "old" tokens signed by the auth service to
"new" tokens, signed by the token server.

R=maruel@chromium.org, nodir@chromium.org
BUG=

Committed:
https://github.com/luci/luci-go/commit/59df90e41b87f38d73e5dde2baa0ed5ba139fe0c
==========

[commit-bot: I haz the power, 2016-10-03 23:14:26]

Committed patchset #3 (id:40001) as
https://github.com/luci/luci-go/commit/59df90e41b87f38d73e5dde2baa0ed5ba139fe0c