Add http node attestor #4909

kfox1111 · 2024-02-23T15:20:44Z

Adds an http node attestor

Commit conforms to CONTRIBUTING.md?
Proper tests/regressions included?
Documentation updated?

evan2645

Thank you for this @kfox1111 and for your patience. I left a handful of high level comments/questions. I had many smaller comments that I held back, I think we're clear to move this out of draft and add tests etc whenever you have a chance

doc/plugin_agent_nodeattestor_httppop.md

doc/plugin_server_nodeattestor_httppop.md

pkg/agent/plugin/nodeattestor/httppop/httppop.go

evan2645 · 2024-04-09T18:25:53Z

pkg/common/plugin/httppop/httppop.go

+	return &Challenge{Nonce: nonce}, nil
+}
+
+func CalculateResponse(challenge *Challenge) (*Response, error) {


What do you envision happening here? Since the challenge is fulfilled by out of band call back from the server, I don't think we need to send anything back on the stream?

Mostly a copy/paste thing from other plugins that use it...

But, there does need to be a message from the agent to the server after the agent starts up the http server and hosts the token to tell the server it can now try and call it. So, there is a Response, even though its blank.

Should I remove the extra function or leave it for consistency with the other plugins?

kfox1111 · 2024-04-10T00:34:43Z

@evan2645 Thanks for the review and the discussion! All good things to consider. :)

Paul-Luciano-2003

helllo, just saying hi.
i have to build a better domain, pitch in for team players.

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

azdagron · 2024-07-18T16:20:15Z

pkg/common/plugin/httpchallenge/httpchallenge.go

+		Path:   fmt.Sprintf("/.well-known/spiffe/nodeattestor/http_challenge/%s/challenge", attestationData.AgentName),
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)


This timeout is probably better applied by the caller. And instead of using context.Background(), it should use the context from the RPC so that it respects stream cancellation.

Do you mean, the value of the timeout should be passed to the function, or the whole context.WithTimeout should be lifted out, and its resulting context passed to this function?

I mean the latter. The plugin should pass its stream context to this function. The benefit there is that if the RPC is cancelled ahead of the timeout then the plugin can fail verification immediately and return. The plugin can set the timeout on the context before passing it into this function.

k. I think I implemented this. but not sure if its doing the right thing with context.Background() still...

pkg/common/plugin/httpchallenge/httpchallenge.go

pkg/common/plugin/httpchallenge/httpchallenge_test.go

pkg/server/plugin/nodeattestor/httpchallenge/httpchallenge_test.go

azdagron · 2024-07-18T16:34:15Z

pkg/server/plugin/nodeattestor/httpchallenge/httpchallenge_test.go

+	u, _ := url.Parse(server.URL)
+	_, port, _ := net.SplitHostPort(u.Host)
+	oldDialContext := http.DefaultTransport.(*http.Transport).DialContext
+	dialContext := func(ctx context.Context, network, addr string) (net.Conn, error) {


Why do we need this special dialing? Can the hostname be "localhost" instead of "foo"?

chatted via slack about this... Maybe resolved now. Please rereview.

azdagron · 2024-07-18T16:41:34Z

pkg/server/plugin/nodeattestor/httpchallenge/httpchallenge_test.go

+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			common_httpchallenge.DefaultRandReader = strings.NewReader("123456789abcdefghijklmnopqrstuvwxyz")


If possible, it's better to avoid mutating package level variables from tests. Doing so inhibits parallelizing test execution in the future.

Instead of trying to control the generated nonce, can the closure you pass in for the challenge response function update a variable that controls what is returned from the http server?

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

pkg/agent/plugin/nodeattestor/httpchallenge/httpchallenge_test.go

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Co-authored-by: Andrew Harding <azdagron@gmail.com> Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

azdagron

Looking great! Just the last few nits and I think we're ready to merge. Thanks for the hard work on this, @kfox1111!

pkg/server/plugin/nodeattestor/httpchallenge/httpchallenge.go

doc/plugin_server_nodeattestor_http_challenge.md

azdagron · 2024-08-03T12:29:53Z

pkg/common/plugin/httpchallenge/httpchallenge.go

+		Path:   fmt.Sprintf("/.well-known/spiffe/nodeattestor/http_challenge/%s/challenge", attestationData.AgentName),
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)


I mean the latter. The plugin should pass its stream context to this function. The benefit there is that if the RPC is cancelled ahead of the timeout then the plugin can fail verification immediately and return. The plugin can set the timeout on the context before passing it into this function.

pkg/common/plugin/httpchallenge/httpchallenge_test.go

azdagron · 2024-08-03T12:35:34Z

pkg/server/plugin/nodeattestor/httpchallenge/httpchallenge_test.go

+	u, _ := neturl.Parse(url)
+	_, port, _ := net.SplitHostPort(u.Host)
+	return &http.Client{
+		Transport: &http.Transport{


Another option here is to clone the default transport like in my other comment and then replace the dialer.

Co-authored-by: Andrew Harding <azdagron@gmail.com> Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

azdagron

\o/

kfox1111 · 2024-08-06T14:00:13Z

Thanks for all the help everyone! :)

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

kfox1111 requested review from evan2645, amartinezfayo, azdagron, MarcosDY and rturner3 as code owners February 23, 2024 15:20

kfox1111 marked this pull request as draft February 23, 2024 15:20

kfox1111 mentioned this pull request Feb 23, 2024

DNS/HTTP Node Attestor #4788

Closed

amartinezfayo assigned evan2645 Feb 27, 2024

evan2645 reviewed Apr 9, 2024

View reviewed changes

Paul-Luciano-2003 approved these changes Apr 16, 2024

View reviewed changes

Paul-Luciano-2003 suggested changes Apr 20, 2024

View reviewed changes

Add http challenge node attestor

b322773

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

kfox1111 force-pushed the http branch from cfbfa0e to b322773 Compare May 10, 2024 16:49

kfox1111 added 12 commits May 11, 2024 09:48

Fix various issues so it works again after refactor

f08d9d2

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Fix some issues

009166f

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Fix some issues

7caac6f

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Fix some issues

3e68d07

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Fix some issues

baaa4f6

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Fix some issues

029f117

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Fix some issues

82af10b

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Fix some issues

86fbf7c

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Implement tofu. Incorperate feedback

727c5f9

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Fix some lint bits

9f42248

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

More lint

76d761f

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

More lint

a693947

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

kfox1111 marked this pull request as ready for review May 16, 2024 22:42

kfox1111 added 2 commits May 16, 2024 16:57

Merge branch 'main' into http

28dc797

Merge branch 'main' into http

6416f01

azdagron self-assigned this Jun 4, 2024

kfox1111 added 5 commits June 24, 2024 09:03

Add some unit tests

69edbe1

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Add a bunch of httpchallenge server plugin tests

41f1e74

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Add some more tests

92e72d5

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Test server works

151d3c3

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Add tofu checks

b6c4a41

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

azdagron reviewed Jul 18, 2024

View reviewed changes

Add httpchallenge agent tests

f00d02f

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

azdagron reviewed Jul 19, 2024

View reviewed changes

pkg/agent/plugin/nodeattestor/httpchallenge/httpchallenge_test.go Outdated Show resolved Hide resolved

pkg/agent/plugin/nodeattestor/httpchallenge/httpchallenge_test.go Outdated Show resolved Hide resolved

kfox1111 and others added 13 commits July 19, 2024 09:51

Incorperate feedback

81b205a

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

gofmt files

501a957

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Apply suggestions from code review

a404aa3

Co-authored-by: Andrew Harding <azdagron@gmail.com> Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>

Fix lint issues

5ab497d

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Fix lint issues

b5caee3

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Fix test

1af2187

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Incorperate feedback. Add localhost block.

a2c0385

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Fix test

be2b414

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Add test for localhost test. Incorperate feedback

2bec5df

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Incorperate feedback

4c95d25

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Fix lint issue

2ac078f

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Merge branch 'main' into http

32f8f04

Merge branch 'main' into http

f47af5c

azdagron reviewed Aug 3, 2024

View reviewed changes

kfox1111 and others added 3 commits August 5, 2024 10:56

Update doc/plugin_server_nodeattestor_http_challenge.md

f596796

Co-authored-by: Andrew Harding <azdagron@gmail.com> Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>

Merge branch 'main' into http

2880654

Incorperate feedback

3dae7a2

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

azdagron approved these changes Aug 6, 2024

View reviewed changes

azdagron added this to the 1.10.2 milestone Aug 6, 2024

azdagron merged commit 5b1966b into spiffe:main Aug 6, 2024
33 checks passed

valverdethiago pushed a commit to valverdethiago/spire that referenced this pull request Aug 18, 2024

Add http node attestor (spiffe#4909)

9cc02d1

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add http node attestor #4909

Add http node attestor #4909

kfox1111 commented Feb 23, 2024 •

edited

Loading

evan2645 left a comment

evan2645 Apr 9, 2024

kfox1111 Apr 10, 2024

kfox1111 Apr 24, 2024

kfox1111 commented Apr 10, 2024

Paul-Luciano-2003 left a comment

azdagron Jul 18, 2024

kfox1111 Jul 20, 2024

azdagron Aug 3, 2024

kfox1111 Aug 5, 2024

azdagron Jul 18, 2024

kfox1111 Jul 20, 2024

azdagron Jul 18, 2024

azdagron left a comment

azdagron Aug 3, 2024

azdagron Aug 3, 2024

azdagron left a comment

kfox1111 commented Aug 6, 2024

Add http node attestor #4909

Add http node attestor #4909

Conversation

kfox1111 commented Feb 23, 2024 • edited Loading

evan2645 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfox1111 commented Apr 10, 2024

Paul-Luciano-2003 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azdagron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azdagron left a comment

Choose a reason for hiding this comment

kfox1111 commented Aug 6, 2024

kfox1111 commented Feb 23, 2024 •

edited

Loading