[WIP] Feat integrate spiffe #1663

TessaIO · 2024-04-14T18:43:12Z

Resolves #1186

netlify · 2024-04-14T18:43:30Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`9a67b7b`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/67e92c285fb2100008293afb
😎 Deploy Preview	https://deploy-preview-1663--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

TessaIO · 2024-04-15T06:59:12Z

/cc marquiz

ArangoGutierrez · 2024-04-15T13:38:31Z

Let's close #1434 ;)

ArangoGutierrez

Some first comments

pkg/utils/spiffe/spiffe.go

pkg/utils/spiffe/spiffe_test.go

pkg/utils/spiffe/spiffe.go

cmd/nfd-master/main.go

pkg/utils/spiffe/spiffe.go

pkg/utils/spiffe/spiffe_test.go

cmd/nfd-master/main.go

deployment/helm/node-feature-discovery/templates/spire-agent-daemonset.yaml

pkg/nfd-master/nfd-master.go

marquiz · 2024-05-24T08:22:09Z

/milestone v0.17

k8s-ci-robot · 2025-02-23T12:10:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: TessaIO
Once this PR has been reviewed and has the lgtm label, please ask for approval from arangogutierrez. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

deployment/helm/node-feature-discovery/values.yaml

Copilot

PR Overview

This PR integrates SPIFFE support to sign and verify NodeFeature objects, addressing issue #1186. Key changes include:

Adding SPIFFE configuration options in the Helm chart and deployment templates.
Implementing SPIFFE client logic and cryptographic signing/verification functions (with accompanying tests) in pkg/utils/spiffe.
Updating nfd-worker and nfd-master to sign and verify NodeFeature objects based on SPIFFE.

Reviewed Changes

File	Description
deployment/helm/node-feature-discovery/values.yaml	Adds SPIFFE configuration options
pkg/utils/spiffe/spiffe.go	Implements SPIFFE signing and verification functions
pkg/utils/spiffe/spiffe_test.go	Provides tests for the SPIFFE implementation
docs/reference/*	Updates command line references to include SPIFFE flags
pkg/nfd-worker/nfd-worker.go	Integrates SPIFFE signing into the worker’s CRD creation/update
pkg/nfd-master/nfd-master.go	Integrates SPIFFE verification in node feature reconciliation
cmd/nfd-worker/main.go & cmd/nfd-master/main.go	Adds CLI flags for enabling SPIFFE

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

pkg/utils/spiffe/spiffe.go

Signed-off-by: TessaIO <[email protected]>

TessaIO · 2025-03-09T18:43:35Z

@ArangoGutierrez @marquiz any other comments about implementation? (I will start working on docs once we agree on the implementation)

marquiz · 2025-03-12T10:07:52Z

any other comments about implementation? (I will start working on docs once we agree on the implementation)

Thank you @TessaIO for the update. The implementation generally looks feasible to me.

The only problem I see, found in my testing, is that now the NF object changes every 60s (or whatever the worker sleepInterval is set to). The data hash does not change but the signature changes -> NF is updated -> trickles down to nfd-master getting update from every node every 60s.

I think this needs to be solved. We should cache the hash (and the signature?) in nfd-worker and not re-sign if it hasn't changed, with forced re-signing every 24 hours or smth. We could also address this in a separate PR.

Any thoughts or comments @TessaIO @ArangoGutierrez?

pkg/utils/spiffe/spiffe.go

marquiz · 2025-03-24T10:51:58Z

pkg/utils/spiffe/spiffe.go

+	}
+
+	dataHash := sha256.Sum256([]byte(stringifyData))
+	if signature, ok := hash_signature_cache[string(dataHash[:])]; ok {


With the caching I started to think that the signing certificates have a TTL, and thus the signature also becomes expired at some point, right(?) What is the default ttl?

I started to wonder that it might be necessary for the nfd-worker to also (every time) validate that the signature is still valid. If not -> re-sign the data.

Comments?

Yes, that's a good point. I was able to reproduce it by reducing the TTL to 1m, and I found that the master wasn't able to verify the signature. I think it's necessary to pass the TTL to the worker so that each time it invalidates the cache, it re-signs again.

marquiz · 2025-03-24T10:57:43Z

deployment/helm/node-feature-discovery/templates/master.yaml

+            path: /run/spire/agent-sockets/api.sock
+            type: Socket


Sorry, I need come take back my earlier comment about the mounts 🤦‍♂️ If the spire-agent pod is deleted and re-created (for whatever reason) the nfd pods will permanently lose the spire sock (as the underlying socket changes), if I'm not mistaken. Thus a possible fix would be (and ditto nfd-worker).

path: /run/spire/agent-sockets type: Directory

Thoughts?

Good point. api.sock is a symlink to spire-agent.sock, so if spire-agent gets restarted or re-created we still have a symlink to the same file name that I think kubelet updates that periodically, right?

Signed-off-by: TessaIO <[email protected]> Signed-off-by: AhmedGrati <[email protected]>

k8s-ci-robot · 2025-03-30T13:35:51Z

@TessaIO: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-node-feature-discovery-build-image-cross-generic	`9a67b7b`	link	true	`/test pull-node-feature-discovery-build-image-cross-generic`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

marquiz · 2025-03-31T11:29:41Z

pkg/utils/spiffe/spiffe.go

+	}
+
+	dataHash := sha256.Sum256([]byte(stringifyData))
+	if len(latestSignature) > 0 {


We'd still need to check the hash too, right? Only return the cached signature if the cached hash matches.

k8s-ci-robot requested review from adrianchiris and ArangoGutierrez April 14, 2024 18:43

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 14, 2024

TessaIO force-pushed the feat-integrate-spiffe branch 4 times, most recently from b60483c to 2917fe1 Compare April 14, 2024 20:45

k8s-ci-robot requested a review from marquiz April 15, 2024 06:59

ArangoGutierrez requested changes Apr 17, 2024

View reviewed changes

pkg/utils/spiffe/spiffe.go Outdated Show resolved Hide resolved

pkg/utils/spiffe/spiffe_test.go Outdated Show resolved Hide resolved

pkg/utils/spiffe/spiffe.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2024

TessaIO force-pushed the feat-integrate-spiffe branch 2 times, most recently from c433995 to 5e5183d Compare April 22, 2024 16:11

TessaIO requested a review from ArangoGutierrez April 22, 2024 16:12

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2024

ArangoGutierrez requested changes Apr 23, 2024

View reviewed changes

cmd/nfd-master/main.go Show resolved Hide resolved

pkg/utils/spiffe/spiffe.go Outdated Show resolved Hide resolved

pkg/utils/spiffe/spiffe_test.go Outdated Show resolved Hide resolved

pkg/utils/spiffe/spiffe_test.go Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2024

TessaIO force-pushed the feat-integrate-spiffe branch from 5e5183d to 4e5af99 Compare April 28, 2024 09:48

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2024

TessaIO force-pushed the feat-integrate-spiffe branch 2 times, most recently from 65e5b03 to aee2686 Compare April 28, 2024 09:52

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2024

marquiz reviewed May 2, 2024

View reviewed changes

cmd/nfd-master/main.go Show resolved Hide resolved

deployment/helm/node-feature-discovery/templates/spire-agent-daemonset.yaml Outdated Show resolved Hide resolved

pkg/nfd-master/nfd-master.go Show resolved Hide resolved

TessaIO force-pushed the feat-integrate-spiffe branch from aee2686 to 520dd72 Compare May 12, 2024 19:27

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 12, 2024

TessaIO force-pushed the feat-integrate-spiffe branch from a73437c to ff883ec Compare February 23, 2025 12:10

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2025

TessaIO force-pushed the feat-integrate-spiffe branch 5 times, most recently from e16d13c to fad8248 Compare February 23, 2025 12:56

marquiz reviewed Feb 24, 2025

View reviewed changes

deployment/helm/node-feature-discovery/values.yaml Outdated Show resolved Hide resolved

marquiz mentioned this pull request Mar 3, 2025

Feat integrate spiffe #1434

Closed

ArangoGutierrez requested a review from Copilot March 3, 2025 09:31

Copilot AI reviewed Mar 3, 2025

View reviewed changes

pkg/utils/spiffe/spiffe.go Outdated Show resolved Hide resolved

TessaIO force-pushed the feat-integrate-spiffe branch from fad8248 to ca92e5a Compare March 9, 2025 18:40

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2025

feat: add spiffe integration in master and worker

c77c370

Signed-off-by: TessaIO <[email protected]>

TessaIO force-pushed the feat-integrate-spiffe branch from ca92e5a to b1d33c2 Compare March 9, 2025 18:41

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2025

TessaIO requested review from marquiz and ArangoGutierrez March 9, 2025 18:42

TessaIO changed the title ~~Feat integrate spiffe~~ [WIP] Feat integrate spiffe Mar 23, 2025

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 23, 2025

TessaIO force-pushed the feat-integrate-spiffe branch 2 times, most recently from 29d0a91 to 73e31a8 Compare March 23, 2025 13:58

marquiz reviewed Mar 24, 2025

View reviewed changes

deploy: add spire manifests in helm and kustomize

9a67b7b

Signed-off-by: TessaIO <[email protected]> Signed-off-by: AhmedGrati <[email protected]>

TessaIO force-pushed the feat-integrate-spiffe branch from 73e31a8 to 9a67b7b Compare March 30, 2025 11:33

marquiz reviewed Mar 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Feat integrate spiffe #1663

[WIP] Feat integrate spiffe #1663

TessaIO commented Apr 14, 2024 •

edited

Loading

netlify bot commented Apr 14, 2024 •

edited

Loading

TessaIO commented Apr 15, 2024

ArangoGutierrez commented Apr 15, 2024

ArangoGutierrez left a comment

marquiz commented May 24, 2024

k8s-ci-robot commented Feb 23, 2025

Copilot AI left a comment

TessaIO commented Mar 9, 2025

marquiz commented Mar 12, 2025

marquiz Mar 24, 2025

TessaIO Mar 30, 2025

marquiz Mar 24, 2025

TessaIO Mar 30, 2025

k8s-ci-robot commented Mar 30, 2025

marquiz Mar 31, 2025

[WIP] Feat integrate spiffe #1663

Are you sure you want to change the base?

[WIP] Feat integrate spiffe #1663

Conversation

TessaIO commented Apr 14, 2024 • edited Loading

netlify bot commented Apr 14, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

TessaIO commented Apr 15, 2024

ArangoGutierrez commented Apr 15, 2024

ArangoGutierrez left a comment

Choose a reason for hiding this comment

marquiz commented May 24, 2024

k8s-ci-robot commented Feb 23, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

TessaIO commented Mar 9, 2025

marquiz commented Mar 12, 2025

marquiz Mar 24, 2025

Choose a reason for hiding this comment

TessaIO Mar 30, 2025

Choose a reason for hiding this comment

marquiz Mar 24, 2025

Choose a reason for hiding this comment

TessaIO Mar 30, 2025

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 30, 2025

marquiz Mar 31, 2025

Choose a reason for hiding this comment

TessaIO commented Apr 14, 2024 •

edited

Loading

netlify bot commented Apr 14, 2024 •

edited

Loading