Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2236163002:
auth: Low-level API for minting delegation tokens. (Closed)
|
Created:
4 years, 4 months ago by Vadim Sh. Modified:
4 years, 4 months ago Reviewers:
iannucci CC:
andrew.wang, chromium-reviews, infra-reviews+luci-go_chromium.org, M-A Ruel, tandrii+luci-go_chromium.org, todd Base URL:
https://chromium.googlesource.com/external/github.com/luci/luci-go@store-int-in-cache Target Ref:
refs/heads/master Project:
luci-go Visibility:
Public. |
Descriptionauth: Low-level API for minting delegation tokens.
It just wraps corresponding Auth Service endpoint.
R=iannucci@chromium.org
BUG=
Committed: https://github.com/luci/luci-go/commit/6be677d31881e22e488a7ab284d4d2ecad86af03
Patch Set 1 #
Total comments: 14
Patch Set 2 : auth: Low-level API for minting delegation tokens. #Patch Set 3 : add tests #
Messages
Total messages: 11 (3 generated)
PTAL Mostly comments. (And still need to write some dumb unit test for it).
https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/doc.go File server/auth/delegation/doc.go (right): https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/doc.... server/auth/delegation/doc.go:10: // If you don't know what it is and how to use it, you probably don't need it. I dunno about 'how to use it'... wouldn't that advise no one to look at the docs? :D Maybe "If you don't know what it is, or why you would use it, you probably...." https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... File server/auth/delegation/minter.go (right): https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... server/auth/delegation/minter.go:21: // AuthServiceURL is root URL (e.g. https://<host>) of the service to use for I think I almost asked this elsewhere, but deleted the comment, but: Why prefer "https://<host>" instead of just "<host>"? What if someone writes "http://<host>" by accident? https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... server/auth/delegation/minter.go:30: // Audience is to whom caller's identity is delegated. IIUC, the Audience set (all identities in this slice, plus all the transient identities in AudienceGroups) is the set of identities to which the bearer must belong in order to use the token? If so, what do you think about AuthorizedIdentities and AuthorizedGroups, defined as "the identities which will be authorized to use the delegation token"? https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... server/auth/delegation/minter.go:36: // usable by any bearer. that's a bit awkward; if you somehow strip both of these restrictions, then the token becomes unlimited? Maybe that's OK, but it seems like a potential opportunity for a bug. Maybe an explicit AudienceUnlimited boolean? https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... server/auth/delegation/minter.go:54: Services []identity.Identity Yeah I'm not super happy with go's zero-value for the struct equalling the most powerful token :/. I feel like we should make these structs fail-safe in the traditional sense (i.e. if you don't specify something, you will get the most restricted, and probably useless, token) This convention would also help us if we add another field like this; if we miss a caller, we'd see it start failing. If we fail-open, then we'd see that service accidentally start requesting (and maybe getting) super-powerful tokens that it doesn't need. https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... server/auth/delegation/minter.go:60: // (e.g. OAuth access token). could we ever run into a situation where this extracts an impersonated identity? Would that be a problem? https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... server/auth/delegation/minter.go:80: // be a short identifier-like string. Will it be publicly visible, or could you put private or pseudo-private identifiers in it?
https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/doc.go File server/auth/delegation/doc.go (right): https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/doc.... server/auth/delegation/doc.go:10: // If you don't know what it is and how to use it, you probably don't need it. On 2016/08/11 22:52:35, iannucci wrote: > I dunno about 'how to use it'... wouldn't that advise no one to look at the > docs? :D > > Maybe "If you don't know what it is, or why you would use it, you probably...." Done. https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... File server/auth/delegation/minter.go (right): https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... server/auth/delegation/minter.go:21: // AuthServiceURL is root URL (e.g. https://<host>) of the service to use for On 2016/08/11 22:52:35, iannucci wrote: > I think I almost asked this elsewhere, but deleted the comment, but: > > Why prefer "https://<host>" instead of just "<host>"? What if someone writes > "http://<host>" by accident? Usually using https://<host> is less strings manipulations in general, and it allows to test stuff locally. https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... server/auth/delegation/minter.go:30: // Audience is to whom caller's identity is delegated. On 2016/08/11 22:52:35, iannucci wrote: > IIUC, the Audience set (all identities in this slice, plus all the transient > identities in AudienceGroups) is the set of identities to which the bearer must > belong in order to use the token? > Correct. > If so, what do you think about AuthorizedIdentities and AuthorizedGroups, > defined as "the identities which will be authorized to use the delegation > token"? "Audience" is a term used in protos and in auth_service code. I prefer to keep it. https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... server/auth/delegation/minter.go:36: // usable by any bearer. On 2016/08/11 22:52:35, iannucci wrote: > that's a bit awkward; if you somehow strip both of these restrictions, then the > token becomes unlimited? Maybe that's OK, but it seems like a potential > opportunity for a bug. > > Maybe an explicit AudienceUnlimited boolean? Done. https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... server/auth/delegation/minter.go:54: Services []identity.Identity On 2016/08/11 22:52:35, iannucci wrote: > Yeah I'm not super happy with go's zero-value for the struct equalling the most > powerful token :/. > > I feel like we should make these structs fail-safe in the traditional sense > (i.e. if you don't specify something, you will get the most restricted, and > probably useless, token) > > This convention would also help us if we add another field like this; if we miss > a caller, we'd see it start failing. If we fail-open, then we'd see that service > accidentally start requesting (and maybe getting) super-powerful tokens that it > doesn't need. Done. https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... server/auth/delegation/minter.go:60: // (e.g. OAuth access token). On 2016/08/11 22:52:35, iannucci wrote: > could we ever run into a situation where this extracts an impersonated identity? No, auth_service enforces that user is not using delegation: https://github.com/luci/luci-py/blob/master/appengine/auth_service/delegation... I have some ideas how to properly implement redelegation and extending token's lifetime, but we don't need them now. https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... server/auth/delegation/minter.go:80: // be a short identifier-like string. On 2016/08/11 22:52:35, iannucci wrote: > Will it be publicly visible, or could you put private or pseudo-private > identifiers in it? It is not encoded in the token body. It ends up in auth_service token audit log (that currently exists as a bunch of entities in Datastore with no UI around them). So anything can go there. But it's not a vessel to carry information in the token. As it says, it just for logging purposes. Consumers of the token won't see it.
On 2016/08/11 at 23:25:38, vadimsh wrote: > https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/doc.go > File server/auth/delegation/doc.go (right): > > https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/doc.... > server/auth/delegation/doc.go:10: // If you don't know what it is and how to use it, you probably don't need it. > On 2016/08/11 22:52:35, iannucci wrote: > > I dunno about 'how to use it'... wouldn't that advise no one to look at the > > docs? :D > > > > Maybe "If you don't know what it is, or why you would use it, you probably...." > > Done. > > https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... > File server/auth/delegation/minter.go (right): > > https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... > server/auth/delegation/minter.go:21: // AuthServiceURL is root URL (e.g. https://<host>) of the service to use for > On 2016/08/11 22:52:35, iannucci wrote: > > I think I almost asked this elsewhere, but deleted the comment, but: > > > > Why prefer "https://<host>" instead of just "<host>"? What if someone writes > > "http://<host>" by accident? > > Usually using https://<host> is less strings manipulations in general, and it allows to test stuff locally. > > https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... > server/auth/delegation/minter.go:30: // Audience is to whom caller's identity is delegated. > On 2016/08/11 22:52:35, iannucci wrote: > > IIUC, the Audience set (all identities in this slice, plus all the transient > > identities in AudienceGroups) is the set of identities to which the bearer must > > belong in order to use the token? > > > Correct. > > > If so, what do you think about AuthorizedIdentities and AuthorizedGroups, > > defined as "the identities which will be authorized to use the delegation > > token"? > "Audience" is a term used in protos and in auth_service code. I prefer to keep it. > > https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... > server/auth/delegation/minter.go:36: // usable by any bearer. > On 2016/08/11 22:52:35, iannucci wrote: > > that's a bit awkward; if you somehow strip both of these restrictions, then the > > token becomes unlimited? Maybe that's OK, but it seems like a potential > > opportunity for a bug. > > > > Maybe an explicit AudienceUnlimited boolean? > > Done. > > https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... > server/auth/delegation/minter.go:54: Services []identity.Identity > On 2016/08/11 22:52:35, iannucci wrote: > > Yeah I'm not super happy with go's zero-value for the struct equalling the most > > powerful token :/. > > > > I feel like we should make these structs fail-safe in the traditional sense > > (i.e. if you don't specify something, you will get the most restricted, and > > probably useless, token) > > > > This convention would also help us if we add another field like this; if we miss > > a caller, we'd see it start failing. If we fail-open, then we'd see that service > > accidentally start requesting (and maybe getting) super-powerful tokens that it > > doesn't need. > > Done. > > https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... > server/auth/delegation/minter.go:60: // (e.g. OAuth access token). > On 2016/08/11 22:52:35, iannucci wrote: > > could we ever run into a situation where this extracts an impersonated identity? > No, auth_service enforces that user is not using delegation: > https://github.com/luci/luci-py/blob/master/appengine/auth_service/delegation... > > I have some ideas how to properly implement redelegation and extending token's lifetime, but we don't need them now. > > https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... > server/auth/delegation/minter.go:80: // be a short identifier-like string. > On 2016/08/11 22:52:35, iannucci wrote: > > Will it be publicly visible, or could you put private or pseudo-private > > identifiers in it? > > It is not encoded in the token body. It ends up in auth_service token audit log (that currently exists as a bunch of entities in Datastore with no UI around them). > > So anything can go there. But it's not a vessel to carry information in the token. As it says, it just for logging purposes. Consumers of the token won't see it. Patch 2?
On 2016/08/11 at 23:29:16, iannucci wrote: > On 2016/08/11 at 23:25:38, vadimsh wrote: > > https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/doc.go > > File server/auth/delegation/doc.go (right): > > > > https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/doc.... > > server/auth/delegation/doc.go:10: // If you don't know what it is and how to use it, you probably don't need it. > > On 2016/08/11 22:52:35, iannucci wrote: > > > I dunno about 'how to use it'... wouldn't that advise no one to look at the > > > docs? :D > > > > > > Maybe "If you don't know what it is, or why you would use it, you probably...." > > > > Done. > > > > https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... > > File server/auth/delegation/minter.go (right): > > > > https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... > > server/auth/delegation/minter.go:21: // AuthServiceURL is root URL (e.g. https://<host>) of the service to use for > > On 2016/08/11 22:52:35, iannucci wrote: > > > I think I almost asked this elsewhere, but deleted the comment, but: > > > > > > Why prefer "https://<host>" instead of just "<host>"? What if someone writes > > > "http://<host>" by accident? > > > > Usually using https://<host> is less strings manipulations in general, and it allows to test stuff locally. > > > > https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... > > server/auth/delegation/minter.go:30: // Audience is to whom caller's identity is delegated. > > On 2016/08/11 22:52:35, iannucci wrote: > > > IIUC, the Audience set (all identities in this slice, plus all the transient > > > identities in AudienceGroups) is the set of identities to which the bearer must > > > belong in order to use the token? > > > > > Correct. > > > > > If so, what do you think about AuthorizedIdentities and AuthorizedGroups, > > > defined as "the identities which will be authorized to use the delegation > > > token"? > > "Audience" is a term used in protos and in auth_service code. I prefer to keep it. > > > > https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... > > server/auth/delegation/minter.go:36: // usable by any bearer. > > On 2016/08/11 22:52:35, iannucci wrote: > > > that's a bit awkward; if you somehow strip both of these restrictions, then the > > > token becomes unlimited? Maybe that's OK, but it seems like a potential > > > opportunity for a bug. > > > > > > Maybe an explicit AudienceUnlimited boolean? > > > > Done. > > > > https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... > > server/auth/delegation/minter.go:54: Services []identity.Identity > > On 2016/08/11 22:52:35, iannucci wrote: > > > Yeah I'm not super happy with go's zero-value for the struct equalling the most > > > powerful token :/. > > > > > > I feel like we should make these structs fail-safe in the traditional sense > > > (i.e. if you don't specify something, you will get the most restricted, and > > > probably useless, token) > > > > > > This convention would also help us if we add another field like this; if we miss > > > a caller, we'd see it start failing. If we fail-open, then we'd see that service > > > accidentally start requesting (and maybe getting) super-powerful tokens that it > > > doesn't need. > > > > Done. > > > > https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... > > server/auth/delegation/minter.go:60: // (e.g. OAuth access token). > > On 2016/08/11 22:52:35, iannucci wrote: > > > could we ever run into a situation where this extracts an impersonated identity? > > No, auth_service enforces that user is not using delegation: > > https://github.com/luci/luci-py/blob/master/appengine/auth_service/delegation... > > > > I have some ideas how to properly implement redelegation and extending token's lifetime, but we don't need them now. > > > > https://codereview.chromium.org/2236163002/diff/1/server/auth/delegation/mint... > > server/auth/delegation/minter.go:80: // be a short identifier-like string. > > On 2016/08/11 22:52:35, iannucci wrote: > > > Will it be publicly visible, or could you put private or pseudo-private > > > identifiers in it? > > > > It is not encoded in the token body. It ends up in auth_service token audit log (that currently exists as a bunch of entities in Datastore with no UI around them). > > > > So anything can go there. But it's not a vessel to carry information in the token. As it says, it just for logging purposes. Consumers of the token won't see it. > > Patch 2? oops, spoke too soon
lgtm
The CQ bit was checked by vadimsh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iannucci@chromium.org Link to the patchset: https://codereview.chromium.org/2236163002/#ps40001 (title: "add tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== auth: Low-level API for minting delegation tokens. It just wraps corresponding Auth Service endpoint. R=iannucci@chromium.org BUG= ========== to ========== auth: Low-level API for minting delegation tokens. It just wraps corresponding Auth Service endpoint. R=iannucci@chromium.org BUG= Committed: https://github.com/luci/luci-go/commit/6be677d31881e22e488a7ab284d4d2ecad86af03 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://github.com/luci/luci-go/commit/6be677d31881e22e488a7ab284d4d2ecad86af03 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
