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

Add initial security best practices documentation #8952

Merged
merged 1 commit into from
Feb 17, 2021

Conversation

howardjohn
Copy link
Member

@howardjohn howardjohn commented Feb 10, 2021

No description provided.

@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Feb 10, 2021
@google-cla google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Feb 10, 2021
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 10, 2021
@istio-testing
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@howardjohn howardjohn marked this pull request as ready for review February 17, 2021 16:37
@howardjohn howardjohn requested a review from a team as a code owner February 17, 2021 16:37
@howardjohn howardjohn force-pushed the security/best-practices branch from 0567672 to 32b45f3 Compare February 17, 2021 16:37
@howardjohn howardjohn changed the title WIP: Add initial security best practices documentation Add initial security best practices documentation Feb 17, 2021
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Feb 17, 2021
@howardjohn howardjohn force-pushed the security/best-practices branch from 32b45f3 to 7032ee4 Compare February 17, 2021 16:58
@istio-testing
Copy link
Contributor

@howardjohn: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
doc.test.profile_none_istio.io 7032ee4 link /test doc.test.profile_none_istio.io

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@ericvn
Copy link
Contributor

ericvn commented Feb 17, 2021

@howardjohn I don't see any reason to not cherrypick to 1.9. Thoughts?

@istio-testing istio-testing merged commit a557c85 into istio:master Feb 17, 2021
@jacob-delgado
Copy link
Contributor

/cherry-pick release-1.9

@istio-testing
Copy link
Contributor

@jacob-delgado: new pull request created: #9003

In response to this:

/cherry-pick release-1.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Contributor

@duderino duderino left a comment

Choose a reason for hiding this comment

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

So happy to see this started! Thank you thank you @howardjohn

@@ -7,36 +7,170 @@ owner: istio/wg-security-maintainers
test: no
---

This section provides some deployment guidelines to help keep a service mesh secure.

## Use namespaces for isolation
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there should be an anti-patterns section (things not to do) and one of the items should be not relying too much on namespaces for isolation?

To reduce privileges granted to pods, Istio offers a [CNI plugin](/docs/setup/additional-setup/cni/) which removes this requirement.

{{< warning >}}
The Istio CNI plugin is currently an alpha feature.
Copy link
Contributor

Choose a reason for hiding this comment

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

@stewartbutler @mandarjog @justinpettit heads up, this will put pressure on carrying istio/cni into beta


All Istio features and APIs are assigned a [feature status](/about/feature-stages/), defining its stability, deprecation policy, and security policy.

Because alpha and experimental features do not have as strong security guarantees, it is recommended to avoid them whenever possible.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps make this stronger. We do not create security releases for vulnerabilities in experimental features. We decide on a case by case basis whether to fix vulnerabilities in alpha features (basically, if we think there is broad adoption of an alpha feature, we'll patch vulnerabilities following principles of responsible disclosure)

Istio also offers a smaller image based on [distroless images](/docs/ops/configuration/security/harden-docker-images/) that reduces the dependencies in the image.

{{< warning >}}
Distroless images are currently an alpha feature.
Copy link
Contributor

Choose a reason for hiding this comment

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

@howardjohn @sdake can we take this to beta?

Copy link
Member Author

Choose a reason for hiding this comment

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

opened a tracker in istio/enhancements#22


To fully lock down traffic, it is recommended to configure [authorization policies](/docs/tasks/security/authorization/).
These allow creating fine-grained policies to allow or deny traffic. For example, you can allow only requests from the `app` namespace to access the `hello-world` service.

Copy link
Contributor

Choose a reason for hiding this comment

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

After mutual TLS, I think it'd be good to have a section for using the beta security policies.

We should cover the limitations of ALLOW policies / describe when it's safer to use DENY.

We should also describe how authentication policies are decoupled from authorization policies and make it clear that authentication policies without corresponding authorization policies are just security theater.

@yangminzhu @myidpt @incfly

Copy link
Member Author

Choose a reason for hiding this comment

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

I would need help with "We should cover the limitations of ALLOW policies / describe when it's safer to use DENY.", if one of the people mentioned above could take this on?

I could take on the later part if needed


* Redirection only handles TCP based traffic. Any UDP or ICMP packets will not be captured or modified.
* Inbound capture is disabled on many [ports used by the sidecar](/docs/ops/deployment/requirements/#ports-used-by-istio) as well as port 22. This list can be expanded by options like `traffic.sidecar.istio.io/excludeInboundPorts`.
* Outbound capture may similarly be reduced through settings like `traffic.sidecar.istio.io/excludeOutboundPorts` or other means.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there should be a stronger link between this section and the immediately following NetworkPolicy section? You could explain which of the issues raised here can be addressed with k8s NetworkPolicy

{{< warning >}}
Distroless images are currently an alpha feature.
{{< /warning >}}

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add:

  1. lockdown Istiod debug API (and describe the functionality you will lose by doing so)
  2. locking down istiod insecure port
  3. warning about the limitations of Envoy Admin API - an attacker can extract information that can inform subsequent attacks (not ideal) and can quitquitquit the local sidecar (which isn't a big deal, you're just dos'ing yourself). also reference Admin endpoint security envoyproxy/envoy#2763

This enables applications that send plaintext HTTP traffic to be transparently "upgraded" to HTTPS.

Care must be taken when configuring the `DestinationRule`'s `tls` setting to specify the `caCertificates` field.
When this is not set, the servers certificate will not be verified.
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope, but it feels like the API should require either caCertificates or something like insecureSkipVerify: true

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the plan, but its challenging to change due to backwards compat.


### Restrict `Gateway` creation privileges

It is recommended to restrict creation of Gateway resources to trusted cluster administrators. This can be achieved by [Kubernetes RBAC policies](https://kubernetes.io/docs/reference/access-authn-authz/rbac/) or tools like [Open Policy Agent](https://www.openpolicyagent.org/).
Copy link
Contributor

Choose a reason for hiding this comment

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

May be worth mentioning that OPA/Gatekeeper can also be used to enforce some of these policies, like restricting hosts

Copy link
Member Author

Choose a reason for hiding this comment

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

I may be misunderstanding, but doesn't this line already recommend OPA?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean that it mentions OPA as a way to restrict Gateway creation to administrators, but it might be worth saying that OPA could be used for more of these recommendations too

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, got it. Agreed 👍


### Isolate sensitive services

It may be desired to enforce stricter physical isolation for sensitive services. For example, you may want to run a
Copy link
Contributor

Choose a reason for hiding this comment

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

May be helpful to offer example reasons for why someone might want to do this

Copy link
Contributor

@stewartbutler stewartbutler left a comment

Choose a reason for hiding this comment

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

I think there should also be a section here pointing to docs about setting up alerting as well, to catch when things are not working right. E.g. if the control plane stops pushing config, apps may continue to work but expected configuration may not propagate. It's not directly a security issue, but I'd consider any production cluster without istio alerts to be at risk.

istio-testing added a commit that referenced this pull request Feb 25, 2021
* Update customize prom scraping instruction. (#8976)

* Update customize prom scraping instruction.

* Update content/en/docs/ops/integrations/prometheus/index.md

Co-authored-by: Sven Mawson <sven@google.com>

Co-authored-by: Sven Mawson <sven@google.com>

* IstioCon blog post (#8984)

Co-authored-by: Frank Budinsky <frankb@ca.ibm.com>

* s/service-apis/gateway-api/ (#8988)

* Update index.md

* Update index.md

* Revert "remove rbac instruction (#8442)" (#8990)

This reverts commit a65a859.

* Update observability best practices (#8897)

* Update observability best practices

* Fix linting issue

* Try and clarify prometheus install

* Update content/en/docs/ops/best-practices/observability/index.md

Co-authored-by: Frank Budinsky <frankb@ca.ibm.com>

* Move to observability page

Co-authored-by: Frank Budinsky <frankb@ca.ibm.com>

* Add doc about how to work around missing metric expiry. (#8948)

* Add doc about how to work around missing metric expiry.

* address comment.

* lint

* add spelling change

* fix

* Update content/en/faq/metrics-and-logs/telemetry-v1-vs-v2.md

Co-authored-by: Douglas Reid <douglas-reid@users.noreply.github.com>

Co-authored-by: Douglas Reid <douglas-reid@users.noreply.github.com>

* Add initial security best practices documentation (#8952)

* Clarify Prometheus TLS settings. (#8962)

* Clarify Prometheus TLS settings.

* Update content/en/docs/ops/integrations/prometheus/index.md

Co-authored-by: Eric Van Norman <ericvn@us.ibm.com>

* Update content/en/docs/ops/integrations/prometheus/index.md

Co-authored-by: Eric Van Norman <ericvn@us.ibm.com>

* Update content/en/docs/ops/integrations/prometheus/index.md

Co-authored-by: Eric Van Norman <ericvn@us.ibm.com>

Co-authored-by: Eric Van Norman <ericvn@us.ibm.com>

* Automator: update common-files@master in istio/istio.io@master (#8997)

* Automator: update istio.io@ reference docs (#8998)

* Automator: update istio.io@ reference docs (#9004)

* Make attribute gen yaml file valid. (#9000)

* Fix the client IP addresses for the authz ingress task (#9002)

* Fix link to Configuration title (#9009)

Signed-off-by: Radim Hrazdil <rhrazdil@redhat.com>

* Automator: update istio.io@ reference docs (#9019)

* Automator: update istio@ test reference (#9021)

* Use istio-ecosystem wasm extensions repo in extensibility concept page. (#9018)

* add a troubleshooting guide for multicluster (#8957)

* add a troubleshooting guide for multicluster

* fix meta

* fix meta

* address review comments

* shift weight

* rel link

* lint

* fix link

* hard to tell what our mdlint customizations are...

* fix mc guide link

* add more context to high-level issues

* cleanup phrasing

* Remove fixed limitation warning (#9034)

This issue no longer exists, I verified via the code and tested it myself as well.

* Fixed "are is" to "are"  and addressed an incorrect link (#9035)

* fixing a typo

* changed the link to go directly to canary upgrades page

* Automator: update istio.io@ reference docs (#9036)

* fix circuit breaker task (#9022)

* fix circuit breaker task

* gen

* Add documentation for Analysis messsage IST0134 ServiceEntryAddresses… (#9020)

* Add documentation for Analysis messsage IST0134 ServiceEntryAddressesRequired

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

* Apply suggestions from code review

Co-authored-by: Ram Vennam <rvennam@us.ibm.com>

* Update content/en/docs/reference/config/analysis/ist0134/index.md

Co-authored-by: Ram Vennam <rvennam@us.ibm.com>

Co-authored-by: Ram Vennam <rvennam@us.ibm.com>

* Add blog for zero configuration Istio (#9025)

* Add blog for zero configuration Istio

The intent here is to show off what Istio provides out of the box, to
attempt to counteract some of the reputation Istio has gotten for being
over complicated/requiring too many CRDs.

* fix links

* Address comments

* Fix examples for newer kubectl (#9045)

* Fix istio.io tests when moving to later kubectl (#9046)

* Automator: update istio.io@ reference docs (#9047)

* Ignore error on first kiali apply (#9048)

* Ignore some errors (#9049)

* Fix syntax on local rate limiting (#9044)

* Add cross references to virtual machine docs (#8913)

* Add cross references to virtual machine docs

* Sven's suggestions

* Update content/en/docs/ops/diagnostic-tools/virtual-machines/index.md

Co-authored-by: Sven Mawson <sven@google.com>

Co-authored-by: Sven Mawson <sven@google.com>

* update authz troubleshoot common problems (#9043)

* update authz troubleshoot common problems

* update

* Add out-of-mesh server metadata info into telemetry v2 faq. (#9017)

* Add out-of-mesh server metadata info into telemetry v2 faq.

* Update content/en/faq/metrics-and-logs/telemetry-v1-vs-v2.md

Co-authored-by: Eric Van Norman <ericvn@us.ibm.com>

* reword

Co-authored-by: Eric Van Norman <ericvn@us.ibm.com>

* Release notes for 1.7.8 (#9054)

* Announce EOL for Istio 1.7 (#9005)

* Release notes for 1.7.7

* Update content/en/news/releases/1.7.x/announcing-1.7.7/index.md

Co-authored-by: Brian Avery <bavery@redhat.com>

* update

* update

* Announce EOL for Istio 1.7

* delay to 02-25

* Delete index.md

Co-authored-by: Brian Avery <bavery@redhat.com>

Co-authored-by: Pengyuan Bian <bianpengyuan@google.com>
Co-authored-by: Sven Mawson <sven@google.com>
Co-authored-by: Istio Automation <istio-testing-bot@google.com>
Co-authored-by: Frank Budinsky <frankb@ca.ibm.com>
Co-authored-by: craigbox <craigbox@google.com>
Co-authored-by: John Howard <howardjohn@google.com>
Co-authored-by: jacob-delgado <jacob.delgado@volunteers.acasi.info>
Co-authored-by: Douglas Reid <douglas-reid@users.noreply.github.com>
Co-authored-by: Eric Van Norman <ericvn@us.ibm.com>
Co-authored-by: lei-tang <32078630+lei-tang@users.noreply.github.com>
Co-authored-by: Radim Hrazdil <32546791+rhrazdil@users.noreply.github.com>
Co-authored-by: Steven Landow <steven@stlcomputerservices.com>
Co-authored-by: Kang-Bae <59033920+Kang-Bae@users.noreply.github.com>
Co-authored-by: masquee <okayanz@outlook.com>
Co-authored-by: Zufar Dhiyaulhaq <zufardhiyaulhaq@gmail.com>
Co-authored-by: Ram Vennam <rvennam@us.ibm.com>
Co-authored-by: Ryan Baker <ryan.baker@c2fo.com>
Co-authored-by: Yangmin Zhu <ymzhu@google.com>
Co-authored-by: Jimmy Chen <28548492+JimmyCYJ@users.noreply.github.com>
Co-authored-by: Brian Avery <bavery@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants