add simple identity registry and registry-based authorizer#1988
add simple identity registry and registry-based authorizer#1988crazytan merged 2 commits intoistio:masterfrom crazytan:registry
Conversation
wattli
left a comment
There was a problem hiding this comment.
Thanks, a few minor comments.
security/pkg/registry/registry.go
Outdated
| } | ||
|
|
||
| // IdentityRegistry is a naive registry that maintains a mapping between | ||
| // identities (as strings) |
There was a problem hiding this comment.
Explains a little bit more about the mapping between identites?
security/pkg/registry/registry.go
Outdated
| return reg | ||
| } | ||
|
|
||
| // K8SServiceAdded ... |
There was a problem hiding this comment.
Finish the comment? Same below
There was a problem hiding this comment.
sorry I forgot... Will add comments
| mapped, ok := reg.Map[id1] | ||
| reg.RUnlock() | ||
| if !ok { | ||
| return false |
There was a problem hiding this comment.
Can we log some info here for debugging purpose? And if possible, please also log the case that mapped existed, but not equal to id2.
| } | ||
|
|
||
| // registryAuthorizor uses an underlying identity registry to make authorization decisions | ||
| type registryAuthorizor struct { |
There was a problem hiding this comment.
So there will be another PR to hook up the Control Plane with this registryAuthorizor, right?
There was a problem hiding this comment.
I'm not sure whether to add a cmd option to let user select which authorizor to use, or just switch to registryAuthorizor by default (for this I need to ensure that existing mesh expansion script works fine).
There was a problem hiding this comment.
The current one is pretty dummy and we should replace it. It would be great to provide an option for user to select once we have more complete authorizors. :)
There was a problem hiding this comment.
Can be done in a follow up PR :)
Codecov Report
@@ Coverage Diff @@
## master #1988 +/- ##
==========================================
- Coverage 81.81% 81.69% -0.13%
==========================================
Files 190 195 +5
Lines 15675 16020 +345
==========================================
+ Hits 12825 13087 +262
- Misses 2414 2459 +45
- Partials 436 474 +38
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| // registryAuthorizor uses an underlying identity registry to make authorization decisions | ||
| type registryAuthorizor struct { |
There was a problem hiding this comment.
The current one is pretty dummy and we should replace it. It would be great to provide an option for user to select once we have more complete authorizors. :)
| } | ||
|
|
||
| // registryAuthorizor uses an underlying identity registry to make authorization decisions | ||
| type registryAuthorizor struct { |
There was a problem hiding this comment.
Can be done in a follow up PR :)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wattli The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
What this PR does / why we need it:
This PR adds identity registry support for service accounts registered via
istioctl registercommand.istioctl registerhas-soption to specify the service account to be linked to the service. By adding a service controller to listen to such changes (see #1951), the identity registry automatically adds new service accounts to the mapping (effectively a whitelist).It also adds
registryAuthorizorthat consults identity registry for authorization decisions. When used by CA, the identity in the CSR sent by node agent needs to be in the registry to be signed by CA.Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #1976Special notes for your reviewer: There's no integration tests for this. Need to wait for #625. Also it's not enabled by default yet.
Release note: