[milo] better ACL system for masters.

[milo] better ACL system for masters.

This way we can check to see if:
  * a user can see internal masters; OR
  * the master is public

Without revealing the existance of internal masters via timing attacks.

Plus it's simpler :)

R=dnj@chromium.org, hinoka@chromium.org, vadimsh@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2974263002
Committed: https://github.com/luci/luci-go/commit/eba3a76e7406cec606db3c8964428c059250110b

[iannucci, 2017-07-11 18:01:48]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-11 18:01:52]

Dry run: CQ is trying da patch.

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

[iannucci, 2017-07-11 18:06:15]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-11 18:06:21]

Dry run: 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, 2017-07-11 18:11:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-11 18:11:11]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Trusty 32-on-64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/374ba64eb7e13f10)
  Luci-go Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/374ba64e0a409410)

[Ryan Tseng, 2017-07-11 18:37:15]

hinoka@google.com changed reviewers:
+ hinoka@google.com

[Ryan Tseng, 2017-07-11 18:37:16]

https://codereview.chromium.org/2974263002/diff/1/milo/buildsource/buildbot/m...
File milo/buildsource/buildbot/master.go (right):

https://codereview.chromium.org/2974263002/diff/1/milo/buildsource/buildbot/m...
milo/buildsource/buildbot/master.go:41: if cu.Identity !=
identity.AnonymousIdentity {
This should go after L51, otherwise a logged in user (eg
happyperson@chromium.org) would lose access to public masters after logging in.

https://codereview.chromium.org/2974263002/diff/1/milo/buildsource/buildbot/p...
File milo/buildsource/buildbot/pubsub.go (right):

https://codereview.chromium.org/2974263002/diff/1/milo/buildsource/buildbot/p...
milo/buildsource/buildbot/pubsub.go:107: toPut := []interface{}{&entry}
why not []*entry{}?

[dnj, 2017-07-11 18:46:34]

https://codereview.chromium.org/2974263002/diff/1/milo/buildsource/buildbot/m...
File milo/buildsource/buildbot/master.go (right):

https://codereview.chromium.org/2974263002/diff/1/milo/buildsource/buildbot/m...
milo/buildsource/buildbot/master.go:42: // if we're logged in, and we can
internal stuff, return nil. getMasterEntry
nit: "we can see internal stuff"

https://codereview.chromium.org/2974263002/diff/1/milo/buildsource/buildbot/p...
File milo/buildsource/buildbot/pubsub.go (right):

https://codereview.chromium.org/2974263002/diff/1/milo/buildsource/buildbot/p...
milo/buildsource/buildbot/pubsub.go:85: type buildbotMasterPublic struct {
Is there a potential problem with already-ingested builds becoming inaccessible
to the public?

https://codereview.chromium.org/2974263002/diff/1/milo/buildsource/buildbot/p...
milo/buildsource/buildbot/pubsub.go:107: toPut := []interface{}{&entry}
On 2017/07/11 18:37:16, Ryan Tseng wrote:
> why not []*entry{}?

b/c this contains both a "buildbotMasterEntry" and potentially a
"buildbotMasterPublic".

https://codereview.chromium.org/2974263002/diff/1/milo/buildsource/buildbot/p...
milo/buildsource/buildbot/pubsub.go:111: if err := ds.Delete(c,
&buildbotMasterPublic{master.Name}); err != nil && err != ds.ErrNoSuchEntity {
nit: declare the public variable outside of this conditional so we only have to
specify it once.

[Ryan Tseng, 2017-07-11 19:25:17]

lgtm

[iannucci, 2017-07-11 21:01:28]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-11 21:01:31]

Dry run: CQ is trying da patch.

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

[iannucci, 2017-07-11 21:01:39]

https://codereview.chromium.org/2974263002/diff/1/milo/buildsource/buildbot/m...
File milo/buildsource/buildbot/master.go (right):

https://codereview.chromium.org/2974263002/diff/1/milo/buildsource/buildbot/m...
milo/buildsource/buildbot/master.go:41: if cu.Identity !=
identity.AnonymousIdentity {
On 2017/07/11 18:37:16, Ryan Tseng wrote:
> This should go after L51, otherwise a logged in user (eg
> mailto:happyperson@chromium.org) would lose access to public masters after
logging in.

chatted, we think this logic is actually correct (non-internal logged in users
should fall through to the anonymous access check below)

https://codereview.chromium.org/2974263002/diff/1/milo/buildsource/buildbot/m...
milo/buildsource/buildbot/master.go:42: // if we're logged in, and we can
internal stuff, return nil. getMasterEntry
On 2017/07/11 18:46:33, dnj wrote:
> nit: "we can see internal stuff"

Done.

https://codereview.chromium.org/2974263002/diff/1/milo/buildsource/buildbot/p...
File milo/buildsource/buildbot/pubsub.go (right):

https://codereview.chromium.org/2974263002/diff/1/milo/buildsource/buildbot/p...
milo/buildsource/buildbot/pubsub.go:85: type buildbotMasterPublic struct {
On 2017/07/11 18:46:34, dnj wrote:
> Is there a potential problem with already-ingested builds becoming
inaccessible
> to the public?

We'll deploy the pubsub module first and make sure all the public masters are
tagged first.

https://codereview.chromium.org/2974263002/diff/1/milo/buildsource/buildbot/p...
milo/buildsource/buildbot/pubsub.go:107: toPut := []interface{}{&entry}
On 2017/07/11 18:46:34, dnj wrote:
> On 2017/07/11 18:37:16, Ryan Tseng wrote:
> > why not []*entry{}?
> 
> b/c this contains both a "buildbotMasterEntry" and potentially a
> "buildbotMasterPublic".

yep

https://codereview.chromium.org/2974263002/diff/1/milo/buildsource/buildbot/p...
milo/buildsource/buildbot/pubsub.go:111: if err := ds.Delete(c,
&buildbotMasterPublic{master.Name}); err != nil && err != ds.ErrNoSuchEntity {
On 2017/07/11 18:46:34, dnj wrote:
> nit: declare the public variable outside of this conditional so we only have
to
> specify it once.

Done.

[Vadim Sh., 2017-07-11 21:04:59]

lgtm in general

https://codereview.chromium.org/2974263002/diff/40001/milo/buildsource/buildb...
File milo/buildsource/buildbot/pubsub.go (right):

https://codereview.chromium.org/2974263002/diff/40001/milo/buildsource/buildb...
milo/buildsource/buildbot/pubsub.go:64: Internal bool
will this be removed?

[commit-bot: I haz the power, 2017-07-11 21:07:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-11 21:07:42]

Dry run: This issue passed the CQ dry run.

[iannucci, 2017-07-11 21:10:28]

https://codereview.chromium.org/2974263002/diff/40001/milo/buildsource/buildb...
File milo/buildsource/buildbot/pubsub.go (right):

https://codereview.chromium.org/2974263002/diff/40001/milo/buildsource/buildb...
milo/buildsource/buildbot/pubsub.go:64: Internal bool
On 2017/07/11 21:04:59, Vadim Sh. wrote:
> will this be removed?
 
later CL

[iannucci, 2017-07-11 21:10:44]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2017-07-11 21:10:45]

The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@google.com
Link to the patchset: https://codereview.chromium.org/2974263002/#ps40001
(title: "fix tests")

[commit-bot: I haz the power, 2017-07-11 21:10:52]

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, 2017-07-11 21:15:35]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1499807444581100,
"parent_rev": "494156a450069cafd4bee1bbd43f327a148cb698", "commit_rev":
"eba3a76e7406cec606db3c8964428c059250110b"}

[commit-bot: I haz the power, 2017-07-11 21:15:38]

Description was changed from

==========
[milo] better ACL system for masters.

This way we can check to see if:
  * a user can see internal masters; OR
  * the master is public

Without revealing the existance of internal masters via timing attacks.

Plus it's simpler :)

R=dnj@chromium.org, hinoka@chromium.org, vadimsh@chromium.org
BUG=
==========

to

==========
[milo] better ACL system for masters.

This way we can check to see if:
  * a user can see internal masters; OR
  * the master is public

Without revealing the existance of internal masters via timing attacks.

Plus it's simpler :)

R=dnj@chromium.org, hinoka@chromium.org, vadimsh@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2974263002
Committed:
https://github.com/luci/luci-go/commit/eba3a76e7406cec606db3c8964428c059250110b
==========

[commit-bot: I haz the power, 2017-07-11 21:15:39]

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