tokenserver: Validate and parse service_accounts.cfg rules.

tokenserver: Validate and parse service_accounts.cfg rules.

R=smut@google.com
BUG=731843
Review-Url: https://codereview.chromium.org/2997433002
Committed: https://github.com/luci/luci-go/commit/4fa5261ebba20e82a97ae573c6dcfb4c32541a8b

[smut, 2017-08-04 23:07:37]

https://codereview.chromium.org/2997433002/diff/20001/tokenserver/appengine/i...
File tokenserver/appengine/impl/serviceaccounts/config_test.go (right):

https://codereview.chromium.org/2997433002/diff/20001/tokenserver/appengine/i...
tokenserver/appengine/impl/serviceaccounts/config_test.go:44: proxy: "group:zzz"
Can you make this group:proxy-group or something, just so someone looking at the
tests doesn't have to remember at assertion time that group:zzz is a proxy group
and group:group-name is an end user group?

https://codereview.chromium.org/2997433002/diff/20001/tokenserver/appengine/i...
File tokenserver/appengine/impl/serviceaccounts/config_validation.go (right):

https://codereview.chromium.org/2997433002/diff/20001/tokenserver/appengine/i...
tokenserver/appengine/impl/serviceaccounts/config_validation.go:48: }
Add comment that we don't bother handling the else because rule.Name == "" is an
error validated by validateRule.

https://codereview.chromium.org/2997433002/diff/20001/tokenserver/appengine/i...
tokenserver/appengine/impl/serviceaccounts/config_validation.go:84: case
r.MaxGrantValidityDuration > 7*24*3600:
Can this be defined somewhere as maximum allowable grant validity duration?
Local to validateRule is fine, I just want it reused in the ctx.Error.

https://codereview.chromium.org/2997433002/diff/20001/tokenserver/appengine/i...
tokenserver/appengine/impl/serviceaccounts/config_validation.go:85:
ctx.Error(`"max_grant_validity_duration" must be smaller than 604801`)
Once maxixmum allowable grant validity duration is defined, change to say
"max_grant_validity_duration" must not exceed %d.

As it is 7*24*3600 and 604801 have to be manually kept in sync.

[Vadim Sh., 2017-08-04 23:37:52]

https://codereview.chromium.org/2997433002/diff/20001/tokenserver/appengine/i...
File tokenserver/appengine/impl/serviceaccounts/config_test.go (right):

https://codereview.chromium.org/2997433002/diff/20001/tokenserver/appengine/i...
tokenserver/appengine/impl/serviceaccounts/config_test.go:44: proxy: "group:zzz"
On 2017/08/04 23:07:36, smut wrote:
> Can you make this group:proxy-group or something, just so someone looking at
the
> tests doesn't have to remember at assertion time that group:zzz is a proxy
group
> and group:group-name is an end user group?

Done.

https://codereview.chromium.org/2997433002/diff/20001/tokenserver/appengine/i...
File tokenserver/appengine/impl/serviceaccounts/config_validation.go (right):

https://codereview.chromium.org/2997433002/diff/20001/tokenserver/appengine/i...
tokenserver/appengine/impl/serviceaccounts/config_validation.go:48: }
On 2017/08/04 23:07:37, smut wrote:
> Add comment that we don't bother handling the else because rule.Name == "" is
an
> error validated by validateRule.

Done.

https://codereview.chromium.org/2997433002/diff/20001/tokenserver/appengine/i...
tokenserver/appengine/impl/serviceaccounts/config_validation.go:84: case
r.MaxGrantValidityDuration > 7*24*3600:
On 2017/08/04 23:07:37, smut wrote:
> Can this be defined somewhere as maximum allowable grant validity duration?
> Local to validateRule is fine, I just want it reused in the ctx.Error.

Done.

Though I have a nagging suspicion that in reality 1 week is probably not going
to work anyway because GAE rotates signing keys faster than that. We'll see.

https://codereview.chromium.org/2997433002/diff/20001/tokenserver/appengine/i...
tokenserver/appengine/impl/serviceaccounts/config_validation.go:85:
ctx.Error(`"max_grant_validity_duration" must be smaller than 604801`)
On 2017/08/04 23:07:37, smut wrote:
> Once maxixmum allowable grant validity duration is defined, change to say
> "max_grant_validity_duration" must not exceed %d.
> 
> As it is 7*24*3600 and 604801 have to be manually kept in sync.

Done.

[smut, 2017-08-04 23:42:01]

lgtm

[Vadim Sh., 2017-08-04 23:46:11]

The CQ bit was checked by vadimsh@chromium.org

[commit-bot: I haz the power, 2017-08-04 23:46:15]

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-08-04 23:51:30]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1501890371553800,
"parent_rev": "5059169d54c9243ad7234ce9174ff604b91857a9", "commit_rev":
"4fa5261ebba20e82a97ae573c6dcfb4c32541a8b"}

[commit-bot: I haz the power, 2017-08-04 23:51:34]

Description was changed from

==========
tokenserver: Validate and parse service_accounts.cfg rules.

R=smut@google.com
BUG=731843
==========

to

==========
tokenserver: Validate and parse service_accounts.cfg rules.

R=smut@google.com
BUG=731843

Review-Url: https://codereview.chromium.org/2997433002
Committed:
https://github.com/luci/luci-go/commit/4fa5261ebba20e82a97ae573c6dcfb4c32541a8b
==========

[commit-bot: I haz the power, 2017-08-04 23:51:35]

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