[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	`1be5343`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/67fb792baa4281000980d6df
😎 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

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 <ahmedgrati1999@gmail.com>

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?

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.

Copilot

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

Files not reviewed (1)

go.mod: Language not supported

Copilot · 2025-04-13T08:20:51Z

pkg/utils/spiffe/spiffe.go

+	WorkloadApiClient workloadapi.Client
+}
+
+var latestSignature []byte


Global caching variables (latestSignature and latestHash) are used in SignData without any synchronization mechanism, which can cause race conditions in concurrent scenarios. Consider protecting these variables with a mutex to guarantee thread safety.

Signed-off-by: TessaIO <ahmedgrati1999@gmail.com> Signed-off-by: AhmedGrati <ahmedgrati1999@gmail.com>

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 4 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 <ahmedgrati1999@gmail.com>

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

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

TessaIO force-pushed the feat-integrate-spiffe branch from 9a67b7b to 852fb9e Compare April 13, 2025 08:20

TessaIO requested review from marquiz and Copilot April 13, 2025 08:20

Copilot AI reviewed Apr 13, 2025

View reviewed changes

deploy: add spire manifests in helm and kustomize

1be5343

Signed-off-by: TessaIO <ahmedgrati1999@gmail.com> Signed-off-by: AhmedGrati <ahmedgrati1999@gmail.com>

TessaIO force-pushed the feat-integrate-spiffe branch from 852fb9e to 1be5343 Compare April 13, 2025 08:43

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

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

marquiz Mar 31, 2025

Copilot AI left a comment

Copilot AI Apr 13, 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

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

marquiz Mar 31, 2025

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI Apr 13, 2025

Choose a reason for hiding this comment

TessaIO commented Apr 14, 2024 •

edited

Loading

netlify bot commented Apr 14, 2024 •

edited

Loading