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 optional network policy object for deploy of step-certificates #172

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sshipway
Copy link

Description

This update to the helm template for step-certificates allows the optional creation of NetworkPolicy rules.

For users who have a Default-Deny policy rule on their kubernetes cluster (as we do) the normal deploy of a LoadBalancer Service will not be accessible from outside, and a Policy rule is required to permit the traffic.

Documentation is added to the Readme file and the values.yaml for the new available values.

Example

networkpolicy:
  enabled: true
  allow:
  -  "10.0.0.0/8"

In addition a couple of minor yaml formatting changes to make our automated yamllint a little happier.

@sshipway sshipway requested a review from a team as a code owner January 18, 2024 02:31
@CLAassistant
Copy link

CLAassistant commented Jan 18, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the needs triage Waiting for discussion / prioritization by team label Jan 18, 2024
@hslatman hslatman requested a review from maraino January 18, 2024 18:19
Copy link
Collaborator

@maraino maraino left a comment

Choose a reason for hiding this comment

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

Hi @sshipway, I'm not very familiar with network policies, so I'm not sure about some things. In any case, I have a few comments I'd like you to fix.

Comment on lines +33 to +38
egress:
- ports:
- protocol: TCP
port: 443
- protocol: TCP
port: 80
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the egress for port 80 should be enabled by default, having 443 makes sense, but I would add the list in the values.yaml, including 443 there, and do a range to create the list of ports.

It also makes sense to add to.ipBlock.cidr... as optional to the egress.

Copy link
Collaborator

Choose a reason for hiding this comment

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

On second thought, 80 also makes sense for ACME HTTP-01 challenges, but they both should be configurable from the values.yaml, as you might want to also add UDP 53 for ACME DNS-01 or some other custom thing for OIDC webhooks, ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the DNS resolution affected by this?

Copy link
Author

Choose a reason for hiding this comment

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

DNS should be fine, as the Kubes already allows DNS resolution to make its way out of the cluster.
Making the ports a list would be possible, but is it really necessary for challenges, which I think are mandated by the ACME standard to be on 80 and 443?
For the egress policy, it makes sense to restrict that to the same subnet(s) as the ingress (as the ones requesting would be the same ones queried). I've added that stanza.

Comment on lines +24 to +25
- protocol: TCP
port: {{ .Values.service.port }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not familiar with networkPolicy, but isn't the targetPolicy enough?

.Values.service.port is used by the ingress (ingress.yaml). I'm unsure if it should be here, as the selector matches the pod. If it is necessary, it should be only there if the ingress is enabled.

Copy link
Author

Choose a reason for hiding this comment

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

You won't need this if you haven't enabled the ingress and only expect to service acme requests from other containers in the same cluster, but then if that's the case, you're not going to need to enable the Policy at all as that applies to communication outside of the Kubernetes cluster.
So we need to allow the service port inbound if we want hosts outside kubernetes to be able to access acme.

Copy link
Author

Choose a reason for hiding this comment

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

Ah I see, you think we won't need .Values.service.Port (which is on the ingres and service) because we go in via the .Values.service.targetPort (on the container), or maybe vice-versa? It might be that we can get rid of targetPort if either Service or Ingres are enabled, and get rid of Port if neither are. TBH I'm not 100% certain how kubes works on this, so I opened both to be safe (as its no extra risk if the request bypasses the ingres). If anyone has a definite on this Im happy for guidance?

step-certificates/templates/policy.yaml Outdated Show resolved Hide resolved
@maraino maraino self-requested a review January 31, 2024 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage Waiting for discussion / prioritization by team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants