Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KIALI-2148 Adding mTLS status into status endpoint #803

Merged
merged 9 commits into from Feb 6, 2019

Conversation

xeviknal
Copy link
Member

@xeviknal xeviknal commented Jan 24, 2019

** Describe the change **

We need to show whether or not mTLS is globally enabled or not. This PR adds that status in /api/status endpoint.

The idea is that the ui takes it and prints a padlock into the masthead.

** Issue reference **
https://issues.jboss.org/browse/KIALI-2148

** Backwards incompatible? **
yep.

screenshot of https___kiali-istio-system 192 168 42 226 nip io_api_status

screenshot of https___kiali-istio-system 192 168 42 226 nip io_api_status 1

screenshot of https___kiali-istio-system 192 168 42 226 nip io_api_status 2

@xeviknal xeviknal added do not merge A PR is not ready to merge requires UI PR A PR sent to the backend kiali/kiali requires changes on frontend kiali/kiali-ui labels Jan 24, 2019
@jmazzitelli
Copy link
Collaborator

"Kiali mTLS" - that doesn't seem right. This has nothing to do with Kiali right? You are talking about mTLS enabled globally in the Istio mesh. I think the name of this should be changed to better reflect what it means. Otherwise, people will see it and think Kiali has turned on mTLS in some way when I think this really means "Istio has mTLS enabled across the global mesh".

@xeviknal
Copy link
Member Author

@jmazzitelli yep, this was one thing I was worried about. I will move it to Istio mTLS.
@lucasponce @aljesusg adding in this info in the status endpoint seems right to you guys?

@rhqci
Copy link

rhqci commented Jan 24, 2019

Jenkins CI: kiali-core-pr-e2e-test #443

@xeviknal xeviknal force-pushed the KIALI-2148 branch 2 times, most recently from d1df034 to 1b89709 Compare January 25, 2019 17:44
@rhqci
Copy link

rhqci commented Jan 25, 2019

Jenkins CI: kiali-core-pr-e2e-test #463

  • ✔️ run-kiali-e2e-tests #[1077]

@rhqci
Copy link

rhqci commented Jan 28, 2019

Jenkins CI: kiali-core-pr-e2e-test #472

  • ✔️ run-kiali-e2e-tests #[1092]

status/mtls_status.go Outdated Show resolved Hide resolved
status/status.go Outdated Show resolved Hide resolved
Copy link
Member Author

@xeviknal xeviknal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes applied!

deploy/openshift/clusterrole.yaml Show resolved Hide resolved
status/mtls_status.go Outdated Show resolved Hide resolved
status/status.go Outdated Show resolved Hide resolved
@rhqci
Copy link

rhqci commented Jan 28, 2019

Jenkins CI: kiali-core-pr-e2e-test #473

  • ✔️ run-kiali-e2e-tests #[1093]

@rhqci
Copy link

rhqci commented Jan 29, 2019

Jenkins CI: kiali-core-pr-e2e-test #476

  • ✔️ run-kiali-e2e-tests #[1098]

@rhqci
Copy link

rhqci commented Jan 29, 2019

Jenkins CI: kiali-core-pr-e2e-test #477

  • ✔️ run-kiali-e2e-tests #[1099]

@abonas
Copy link
Contributor

abonas commented Jan 30, 2019

@xeviknal what are the different status options? right now I see GLOBALLY_ENABLED and NOT_GLOBALLY_ENABLED. if those are the only 2 statuses, why not just use boolean true/false? the field can be called something like MTLS_ENABLED.
also iirc there was a discussion about the word "global" that it could be confusing, perhaps it was on the UI PR.

@rhqci
Copy link

rhqci commented Jan 30, 2019

Jenkins CI: kiali-core-pr-e2e-test #482

  • ✔️ run-kiali-e2e-tests #[1109]

@xeviknal
Copy link
Member Author

xeviknal commented Jan 30, 2019

@abonas we have 3 scenarios so far, as I understood from heiko. I just have mentioned them in here: kiali/kiali-ui#963 (comment)
Just updated this PR description's screenshots.

Regarding the naming, I will change that to mesh-y wording instead.

@rhqci
Copy link

rhqci commented Jan 30, 2019

Jenkins CI: kiali-core-pr-e2e-test #483

  • ✔️ run-kiali-e2e-tests #[1111]

@xeviknal xeviknal removed the do not merge A PR is not ready to merge label Jan 30, 2019
@xeviknal
Copy link
Member Author

xeviknal commented Jan 30, 2019

Instruccions to verify this Pull Request (on top of bookinfo):

To achieve ENABLED status:

  1. Install MeshPolicy[1]
  2. Install DR[2]

To achieve PARTIALLY_ENABLED status

  1. Install only the MP[1]
    OR
  2. Install only the DR[2]
  • Try both options to fully verify the feature.

To achieve NOT_ENABLED status:
Remove both 1) and 2) in case you have installed already.

MP[1] - Mesh policy enabling mesh-wide mTLS

apiVersion: "authentication.istio.io/v1alpha1"
kind: "MeshPolicy"
metadata:
  name: "default"
spec:
  peers:
  - mtls: {}

DR[2] - Destination Rule with mesh-wide enabling

apiVersion: "networking.istio.io/v1alpha3"
kind: "DestinationRule"
metadata:
  name: "default"
  namespace: "default"
spec:
  host: "*.local"
  trafficPolicy:
    tls:
      mode: ISTIO_MUTUAL

Mesh-wide installation: https://istio.io/docs/tasks/security/authn-policy/#globally-enabling-istio-mutual-tls
Namespace-wide installation: https://istio.io/docs/tasks/security/authn-policy/#namespace-wide-policy

@hhovsepy
Copy link
Contributor

2 question here:

  1. When Istio itself is installed in 'auth' mode, it should be considered as ENABLED without any MP or DR creation, right?
  2. When mTLS mode is enabled for Namespace wide, or Service specific, where the secure icon should be shown?

@xeviknal
Copy link
Member Author

Regarding to 1), I just realized that the DR might be installed in different namespaces than the default as is in the example in istio webpage. It works whenever the NS is installed: i.e. istio-system, myproject, bookinfo, etc. (always talking about the mesh-wide mtls installation). Should we check all the DR? should we force the user to install it in istio-system?

@rhqci
Copy link

rhqci commented Jan 30, 2019

Jenkins CI: kiali-core-pr-e2e-test #485

  • ✔️ run-kiali-e2e-tests #[1114]

@xeviknal
Copy link
Member Author

xeviknal commented Jan 30, 2019

@hhovsepy added a commit to show green lock for istio-demo-auth.yaml installation. Can you check i out?
@pilhuhn could you please check the doubts here out and verify the feature here is align with what you suggest in previous comments?

@pilhuhn
Copy link
Contributor

pilhuhn commented Jan 30, 2019

@xeviknal the combo MeshPolicy+DR in default is what enables mesh-wide mTLS.
Other *.DRs in other namespaces are used for namespace wide enabling. Here the counterpart is a Policy and not a MeshPolicy. See also
https://itnext.io/musings-about-istio-with-mtls-c64b551fe104

@xeviknal
Copy link
Member Author

xeviknal commented Jan 30, 2019

@pilhuhn if you check the istio-demo-auth.yaml installation file, the DR is installed in istio-system namespace. That is why @hhovsepy spot that oddness.

# Authentication policy to enable mutual TLS for all services (that have sidecar) in the mesh.
    apiVersion: "authentication.istio.io/v1alpha1"
    kind: "MeshPolicy"
    metadata:
      name: "default"
      labels:
        app: istio-security
        chart: security-1.0.5
        release: istio
        heritage: Tiller
    spec:
      peers:
      - mtls: {}
    ---
    # Corresponding destination rule to configure client side to use mutual TLS when talking to
    # any service (host) in the mesh.
    apiVersion: networking.istio.io/v1alpha3
    kind: DestinationRule
    metadata:
      name: "default"
      labels:
        app: istio-security
        chart: security-1.0.5
        release: istio
        heritage: Tiller
    spec:
      host: "*.local"
      trafficPolicy:
        tls:
          mode: ISTIO_MUTUAL

//from istio-demo-auth.yaml

$ oc describe destinationrules default -n istio-system
Name:         default
Namespace:    istio-system
Labels:       app=istio-security
              chart=security-1.0.4
              heritage=Tiller
              release=istio
Annotations:  kubectl.kubernetes.io/last-applied-configuration={"apiVersion":"networking.istio.io/v1alpha3","kind":"DestinationRule","metadata":{"annotations":{},"labels":{"app":"istio-security","chart":"security-1...
API Version:  networking.istio.io/v1alpha3
Kind:         DestinationRule
Metadata:
  Creation Timestamp:  2019-01-30T19:33:30Z
  Generation:          1
  Resource Version:    3893
  Self Link:           /apis/networking.istio.io/v1alpha3/namespaces/istio-system/destinationrules/default
  UID:                 ec2a3c68-24c5-11e9-9928-525400fc4f79
Spec:
  Host:  *.local
  Traffic Policy:
    Tls:
      Mode:  ISTIO_MUTUAL
Events:      <none>

@hhovsepy
Copy link
Contributor

hhovsepy commented Jan 31, 2019

Testing results for different configurations:

  1. mesh wide mTLS: green lock icon
  2. EDIT namespace wide mTLS: no lock icon
  3. EDIT service wide mTLS: no lock icon
  4. VS or DR missing: red opened lock icon
  5. Istio configured in auth mode: green lock icon

business/istio_config.go Outdated Show resolved Hide resolved
kubernetes/istio_details_service.go Outdated Show resolved Hide resolved
@rhqci
Copy link

rhqci commented Feb 5, 2019

Jenkins CI: kiali-core-pr-e2e-test #507

  • ✔️ run-kiali-e2e-tests #[1150]

@rhqci
Copy link

rhqci commented Feb 5, 2019

Jenkins CI: kiali-core-pr-e2e-test #508

  • ✔️ run-kiali-e2e-tests #[1151]

@xeviknal
Copy link
Member Author

xeviknal commented Feb 5, 2019

@lucasponce comments approached :)

Copy link
Contributor

@lucasponce lucasponce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally with PR UI kiali/kiali-ui#963 and discussed with @xeviknal

@lucasponce lucasponce merged commit 3992111 into kiali:master Feb 6, 2019
@xeviknal xeviknal deleted the KIALI-2148 branch February 6, 2019 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires UI PR A PR sent to the backend kiali/kiali requires changes on frontend kiali/kiali-ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants