Document RBAC set up for ingress key cert protection. #755

JimmyCYJ · 2017-12-05T01:01:03Z

Which issue this PR fixes: fixes istio/old_auth_repo#313

wattli

Thanks for working on this. Generally looks good with a few comments.

wattli · 2017-12-05T18:08:43Z

_docs/tasks/traffic-management/ingress.md

@@ -269,6 +269,344 @@ rules.
   curl -I -k https://$INGRESS_HOST/status/200
   ```

+## Configuring RBAC for ingress key/cert


The key/cert is only used in above section, would it make more sense to put the new content in that section?

wattli · 2017-12-05T18:10:57Z

_docs/tasks/traffic-management/ingress.md

@@ -269,6 +269,344 @@ rules.
   curl -I -k https://$INGRESS_HOST/status/200
   ```

+## Configuring RBAC for ingress key/cert
+There are service accounts which can access this ingress key/cert, and the ingress key/cert is not 
+encrypted. This leads to risks of leaking key/cert. We can set up Role-Based Access Control ("RBAC") 


This is not accurate. K8s supports encrypted secret already, see here: https://kubernetes.io/docs/tasks/administer-cluster/encrypt-data/ . It would be great if we could update this as well. But maybe in a follow up PR.

Thanks for pointing that out. I didn't know that we can encrypt secret. I will document encrypting secrets in a follow up PR.

wattli · 2017-12-05T18:14:45Z

_docs/tasks/traffic-management/ingress.md

+istio-ingress-service-account. We need to update or replace these RBAC set up to only allow 
+istio-ingress-service-account to access ingress key/cert.       
+
+1. Update RBAC set up for istio-pilot-service-account and istio-mixer-istio-service-account.


Please help to keep the new content succinct and only display things of interest.

I have made the content shorter now.

wattli · 2017-12-05T18:15:30Z

_docs/tasks/traffic-management/ingress.md

+   ```
+   This produces the following output:
+
+   ```bash


We don't have to display everything, please focus on ingress related one.

Done. Thanks!

wattli · 2017-12-05T18:16:01Z

_docs/tasks/traffic-management/ingress.md

+   metadata:
+     name: istio-pilot-mixer-istio-system
+   rules:
+   - apiGroups: [""] # "" indicates the core API group


Same here and below. Please avoid showing unrelated content.

JimmyCYJ · 2017-12-05T19:46:06Z

I have revised the document, please take a look. Thanks.

wattli · 2017-12-06T17:03:34Z

_docs/tasks/traffic-management/ingress.md

+   ```bash
+   kubectl delete ClusterRoleBinding istio-pilot-admin-role-binding-istio-system
+   kubectl delete ClusterRoleBinding istio-mixer-admin-role-binding-istio-system
+   kubectl delete ClusterRole istio-mixer-istio-system


Is there a similar role for istio-pilot-istio-system?

Is there any way to recover the deletion here at the end of this file? The idea is to clean up the change in this file after everything is done.

And what privileges will pilot/mixer lose after this change? I am afraid this will break existing system. Let me add some pilot and mixer guy.

istio-pilot-istio-system is also bound to account istio-ingress-service-account by istio-ingress-admin-role-binding-istio-system. We will delete istio-pilot-istio-system in next step, after deleting that ClusterRoleBinding object.

According to https://github.com/istio/istio/blob/11dc2fb937a23157215fffa06921cf507d27f757/install/kubernetes/istio.yaml#L22
istio-pilot-istio-system grants access permission to many resources. We need to bind those rules to istio-pilot-service-account. We should apply similar role binding process to account istio-mixer-service-account and istio-ca-service-account

wattli · 2017-12-06T17:07:30Z

_docs/tasks/traffic-management/ingress.md

+   NAME                                        TYPE                                  DATA      AGE
+   istio-ca-secret                             istio.io/ca-root                      2         7d
+   istio-ingress-certs                         kubernetes.io/tls                     2         7d
+   istio-ingress-service-account-token-kwr85   kubernetes.io/service-account-token   3         7d


Can we remove this one as well? And istio-ca-secret?

lookuptable · 2017-12-06T17:27:34Z

_docs/tasks/traffic-management/ingress.md

+   rules:
+   - apiGroups: [""] # "" indicates the core API group
+     resources: ["secrets"]
+     resourceNames: ["istio-ca-service-account-token-rl4xm"]


It seems quite dangerous to hard-code names with randomized tokens (rl4xm).

This is a kubernetes.io/service-account-token, and it is removed.

lookuptable · 2017-12-06T17:33:01Z

_docs/tasks/traffic-management/ingress.md

+     namespace: istio-system
+   - kind: ServiceAccount
+     name: istio-mixer-service-account
+     namespace: istio-system


So we're binding the same role to both mixer SA and pilot SA. Is this desirable? IIRC mixer and pilot need different permissions.

Yes, they need different permissions. I have create separate ClusterRoles for each of them.

lookuptable · 2017-12-06T17:35:09Z

_docs/tasks/traffic-management/ingress.md

+   ```
+
+   Create istio-pilot-mixer-istio-system.yaml, which allows istio-pilot-service-account and 
+   istio-mixer-service-account to read all kubernetes.io/service-account-token, 


I don't think anyone needs to read kubernetes.io/service-account-token, no?

Agree. That line is removed.

ldemailly · 2018-01-25T05:13:34Z

can we move this forward, it's been open a while ? (and there is a conflict to be resolved now too)

JimmyCYJ · 2018-01-25T05:15:33Z

Sure, I will update this PR.

ldemailly · 2018-02-03T00:08:33Z

ping ?

JimmyCYJ · 2018-02-03T01:31:47Z

Sorry I was working on other stuff, will update this PR soon.

ldemailly

I wonder if we should update the installation rather than document workarounds ?

ldemailly · 2018-02-05T19:49:05Z

_docs/tasks/traffic-management/ingress.md

@@ -214,11 +214,6 @@ The following are the known limitations of Istio ingress:
     kubectl get ingress secure-ingress -o wide
     ```

-     ```bash
-     NAME             HOSTS     ADDRESS                 PORTS     AGE
-     secure-ingress   *         130.211.10.121          80        1d


why is this sample output removed?

That part is added back. Thanks.

This PR is for the ingress controller task only. Maybe we can update the installation with a more general solution in future work?

wattli · 2018-02-06T18:39:40Z

Please validate it before submission. Thanks for working on this, Jimmy.

wattli

/lgtm

wattli reviewed Dec 5, 2017

View reviewed changes

JimmyCYJ added 3 commits December 5, 2017 11:34

Document RBAC set up for ingress key/cert

07add4d

Document revision.

3c91c35

Addressed some review comments.

bd00f69

JimmyCYJ force-pushed the rbac-ingress-key-cert branch from 3228b41 to bd00f69 Compare December 5, 2017 19:35

JimmyCYJ added 2 commits December 5, 2017 11:39

Add some revision.

02f66cb

Add some revision.

722943b

wattli reviewed Dec 6, 2017

View reviewed changes

wattli requested review from ayj, mandarjog and andraxylia December 6, 2017 17:09

lookuptable reviewed Dec 6, 2017

View reviewed changes

ldemailly reviewed Feb 5, 2018

View reviewed changes

Revise per comment.

5795f37

JimmyCYJ force-pushed the rbac-ingress-key-cert branch from ea6c3b3 to 5795f37 Compare February 6, 2018 03:07

JimmyCYJ added 3 commits February 5, 2018 22:16

Merge branch 'master' into rbac-ingress-key-cert

ff6b74c

Merge branch 'master' into rbac-ingress-key-cert

abd1066

Update ingress.md

5992c3d

wattli approved these changes Feb 6, 2018

View reviewed changes

ldemailly merged commit ea11fcf into istio:master Feb 6, 2018

JimmyCYJ deleted the rbac-ingress-key-cert branch February 6, 2018 21:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document RBAC set up for ingress key cert protection. #755

Document RBAC set up for ingress key cert protection. #755

JimmyCYJ commented Dec 5, 2017 •

edited by ldemailly

Loading

wattli left a comment

wattli Dec 5, 2017

wattli Dec 5, 2017

JimmyCYJ Dec 5, 2017

wattli Dec 5, 2017

JimmyCYJ Dec 5, 2017

wattli Dec 5, 2017

JimmyCYJ Dec 5, 2017

wattli Dec 5, 2017

JimmyCYJ Dec 5, 2017

JimmyCYJ commented Dec 5, 2017

wattli Dec 6, 2017

wattli Dec 6, 2017

wattli Dec 6, 2017

JimmyCYJ Feb 6, 2018

wattli Dec 6, 2017

JimmyCYJ Dec 8, 2017

lookuptable Dec 6, 2017

JimmyCYJ Feb 6, 2018

lookuptable Dec 6, 2017 •

edited

Loading

JimmyCYJ Feb 6, 2018

lookuptable Dec 6, 2017

JimmyCYJ Feb 6, 2018

ldemailly commented Jan 25, 2018

JimmyCYJ commented Jan 25, 2018

ldemailly commented Feb 3, 2018

JimmyCYJ commented Feb 3, 2018

ldemailly left a comment

ldemailly Feb 5, 2018

JimmyCYJ Feb 6, 2018

JimmyCYJ Feb 6, 2018

wattli commented Feb 6, 2018

wattli left a comment

Document RBAC set up for ingress key cert protection. #755

Document RBAC set up for ingress key cert protection. #755

Conversation

JimmyCYJ commented Dec 5, 2017 • edited by ldemailly Loading

wattli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimmyCYJ commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lookuptable Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldemailly commented Jan 25, 2018

JimmyCYJ commented Jan 25, 2018

ldemailly commented Feb 3, 2018

JimmyCYJ commented Feb 3, 2018

ldemailly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wattli commented Feb 6, 2018

wattli left a comment

Choose a reason for hiding this comment

JimmyCYJ commented Dec 5, 2017 •

edited by ldemailly

Loading

lookuptable Dec 6, 2017 •

edited

Loading