Add http node attestor #4909
Add http node attestor #4909
Conversation
kfox1111
requested review from
evan2645,
amartinezfayo,
azdagron,
MarcosDY and
rturner3
as code owners
There was a problem hiding this comment.
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
pkg/common/plugin/httppop/httppop.go
Outdated
| return &Challenge{Nonce: nonce}, nil | ||
| } | ||
|
|
||
| func CalculateResponse(challenge *Challenge) (*Response, error) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Should I remove the extra function or leave it for consistency with the other plugins?
|
@evan2645 Thanks for the review and the discussion! All good things to consider. :) |
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
| } | ||
|
|
||
| // make sure the configuration produces valid data | ||
| if _, err := loadConfigData(config); err != nil { |
There was a problem hiding this comment.
I'd rather change it. I think it makes it all easier to reason about.
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>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
| Path: fmt.Sprintf("/.well-known/spiffe/nodeattestor/http_challenge/%s/challenge", attestationData.AgentName), | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
pkg/server/plugin/nodeattestor/httpchallenge/httpchallenge_test.go
Outdated
Show resolved
Hide resolved
| 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) { |
There was a problem hiding this comment.
Why do we need this special dialing? Can the hostname be "localhost" instead of "foo"?
There was a problem hiding this comment.
chatted via slack about this... Maybe resolved now. Please rereview.
| for _, tt := range tests { | ||
| tt := tt | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| common_httpchallenge.DefaultRandReader = strings.NewReader("123456789abcdefghijklmnopqrstuvwxyz") |
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
pkg/agent/plugin/nodeattestor/httpchallenge/httpchallenge_test.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
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>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Adds an http node attestor
Fixes: #4788