Expose pod tempting on acme http issuer by JoshVanL · Pull Request #1749 · cert-manager/cert-manager

JoshVanL · 2019-06-05T14:27:09Z

Signed-off-by: JoshVanL vleeuwenjoshua@gmail.com

What this PR does / why we need it:
Exposes a core.PodTemplate resource for the ACME HTTP issuer config.
This enables one to change values around the solver pod, i.e. labels and annotations.
It uses the current cert-manager values as a default and "merges" custom values ontop.

fixes #1097

Special notes for your reviewer:

Expose pod template for the ACME issuer solver pod

/assign

validation Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

munnerz

Looks good. I've added a few comments, notably I'm starting to think we should be more and more careful with the fields we do allow users to set.

This could be used as a privilege escalation point if we are not careful, as lower-privileged users are able to configure cert-manager to create resources that they otherwise may not be able to create.

I'm beginning to think we may be best off creating our own PodTemplateSpec like structure that for the time being only contains ObjectMeta, and then setting the "fields allowed to be overridden" to be a whitelist instead of a blacklist (i.e. we only allow labels & annotations initially). This would at least prevent any new fields that we aren't currently aware of being utilised by an attacker/evil user 😄

munnerz · 2019-06-07T09:52:31Z

pkg/apis/certmanager/v1alpha1/types_issuer.go

+	// Optional template to configure the solver pods. Not all pod template
+	// options are valid (e.g. name).
+	// +optional
+	PodTemplate corev1.PodTemplateSpec `json:"podTemplate,omitempty"`


This should be a field under ingress, as it is specifically a configuration option that the Ingress based HTTP01 solver uses (which is coincidentally the only HTTP01 solver type we support).

We should also make this a *corev1.PodTemplateSpec, as a user may omit this field altogether, and we can then identify 'not set' values.

munnerz · 2019-06-07T09:53:22Z

docs/tasks/issuers/setup-acme/http01/index.rst

+             namespace: default
+             labels:
+               environment: production
+               foo: bar


(based on previous experience in these docs) - we should include the full YAML structure here else it confuses users. If there are extra verbose parts that aren't relevant when doing so, you can replace the irrelevant fields with ... so long as you keep the indentation correct

munnerz · 2019-06-07T09:54:32Z

docs/tasks/issuers/setup-acme/http01/index.rst

+               foo: bar
+
+Unless changed, the pod's metadata will remain the default for each unedited
+field when a podTemplate is specified.


will remain the default for each unedited field when a podTemplate is specified. can you reword this? I'm a bit lost as to what you're trying to convey. Maybe: If not specified, a default set of metadata will be applied to the generated solver pod.?

munnerz · 2019-06-07T09:56:02Z

docs/tasks/issuers/setup-acme/http01/index.rst

+             namespace: default
+             labels:
+               environment: production
+               foo: bar


It also looks like you are missing the metadata field here. Perhaps this is why you were having issues in your tests? I'd expect it to look something like:

podTemplate: metadata: namespace: default labels: environment: production foo: bar

munnerz · 2019-06-07T09:59:12Z

pkg/apis/certmanager/v1alpha1/types_issuer.go

 	Ingress *ACMEChallengeSolverHTTP01Ingress `json:"ingress"`
+
+	// Optional template to configure the solver pods. Not all pod template
+	// options are valid (e.g. name).


Could you expand this comment out some more?

Optional pod template used to configure the ACME challenge solver pods used for HTTP01 challenges. Setting this option could lead to unintended side-effects, however it may be necessary when running in certain network restricted environments (e.g. Istio). Only `metadata` fields can be set here. All other fields will be ignored. The `metadata.name` and `metadata.namespace` fields cannot be overridden.

munnerz · 2019-06-07T10:01:24Z

pkg/apis/certmanager/validation/issuer.go

+		el = append(el, field.Invalid(fldPath.Child("podTemplate"), sol.PodTemplate.GenerateName, "generateName cannot be set for solver pod template"))
+	}
+
+	if sol.PodTemplate.Spec.String() != new(corev1.PodSpec).String() {


😬 this seems awkward. Can we instead compare if sol.PodTemplate.Spec != corev1.PodSpec{}? I think that'll work as expected, because if both are not set then they will both have empty values set for all of their fields.

munnerz · 2019-06-07T10:02:37Z

pkg/issuer/acme/http/pod.go

+	// Override defaults if they have changed in the pod template.
+	if ch.Spec.Solver != nil {
+		pod = s.mergePodObjectMetaWithPodTemplate(pod,
+			ch.Spec.Solver.HTTP01.PodTemplate.DeepCopy())


Deep copy shouldn't be needed here as we don't actually modify podTempl in the mergePodObjectMetaWithPodTemplate method

pkg/issuer/acme/http/pod_test.go

JoshVanL · 2019-06-07T10:44:10Z

Thanks for the comments @munnerz, really useful. Will get to making changes now

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

JoshVanL · 2019-06-10T08:03:13Z

/unassign
/assign @munnerz

munnerz

Just a few final comments.. all looks good to me 😄

pkg/issuer/acme/http/pod.go

pkg/issuer/acme/http/pod_test.go

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

JoshVanL · 2019-06-11T15:39:13Z

Thanks @munnerz, back to you :)

munnerz · 2019-06-11T17:42:44Z

/lgtm
/approve

jetstack-bot · 2019-06-11T17:42:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoshVanL, munnerz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [munnerz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jetstack-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 5, 2019

jetstack-bot assigned JoshVanL Jun 5, 2019

jetstack-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Jun 5, 2019

jetstack-bot requested a review from munnerz June 5, 2019 14:27

JoshVanL force-pushed the pod-templating branch from ad31b43 to f511fa5 Compare June 5, 2019 18:39

JoshVanL added 3 commits June 7, 2019 10:41

Assigns solver pod to exposed template with defaults, includes

f2ba4d9

validation Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Exposes pod template to change solver ObjectMetdata

b290288

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Move podTemplate to solvers

0925392

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

JoshVanL force-pushed the pod-templating branch from 8d391fb to cd6d6ce Compare June 7, 2019 09:41

Adds podTemplate docs

622eaa1

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

JoshVanL force-pushed the pod-templating branch from cd6d6ce to 622eaa1 Compare June 7, 2019 09:44

munnerz reviewed Jun 7, 2019

View reviewed changes

JoshVanL added 2 commits June 9, 2019 18:37

Restrict podTemplate to labels and annotations only

5146dc2

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Update docs on podTemplating only using labels and annotations

e9c9ea2

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

JoshVanL force-pushed the pod-templating branch from da99160 to e9c9ea2 Compare June 10, 2019 08:02

JoshVanL changed the title ~~WIP: expose pod tempting on acme http issuer~~ Expose pod tempting on acme http issuer Jun 10, 2019

jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 10, 2019

jetstack-bot assigned munnerz and unassigned JoshVanL Jun 10, 2019

munnerz reviewed Jun 11, 2019

View reviewed changes

pkg/issuer/acme/http/pod.go Show resolved Hide resolved

pkg/issuer/acme/http/pod.go Show resolved Hide resolved

pkg/issuer/acme/http/pod_test.go Outdated Show resolved Hide resolved

More nil checking/precautions and update test name

f526fbe

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2019

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 11, 2019

jetstack-bot merged commit 93faf9c into cert-manager:master Jun 11, 2019

tony84727 mentioned this pull request Dec 18, 2019

Support VirtualService resources for HTTP01 solving for better Istio support #1636

Closed

haf mentioned this pull request Feb 19, 2020

Kubernetes Ingress with Cert-Manager not working on a brand-new cluster. istio/istio#20680

Closed

gAmUssA mentioned this pull request Feb 23, 2022

[cert-manager.md] Improve ClusterIssuer snippet Kong/docs.konghq.com#3707

Merged

Conversation

JoshVanL commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

munnerz left a comment

Choose a reason for hiding this comment

Uh oh!

munnerz Jun 7, 2019

Choose a reason for hiding this comment

Uh oh!

munnerz Jun 7, 2019

Choose a reason for hiding this comment

Uh oh!

munnerz Jun 7, 2019

Choose a reason for hiding this comment

Uh oh!

munnerz Jun 7, 2019

Choose a reason for hiding this comment

Uh oh!

munnerz Jun 7, 2019

Choose a reason for hiding this comment

Uh oh!

munnerz Jun 7, 2019

Choose a reason for hiding this comment

Uh oh!

munnerz Jun 7, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoshVanL commented Jun 7, 2019

Uh oh!

JoshVanL commented Jun 10, 2019

Uh oh!

munnerz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoshVanL commented Jun 11, 2019

Uh oh!

munnerz commented Jun 11, 2019

Uh oh!

jetstack-bot commented Jun 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JoshVanL commented Jun 5, 2019 •

edited

Loading