Skip to content

Use Bad Request for invalid inputs in /chares #57

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cowbon wants to merge 1 commit into
base: main
Choose a base branch
from
+72 −29
Open

Use Bad Request for invalid inputs in /chares #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion api/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,18 @@ type Server struct {
options string
}

func responseCodeToHTTP(responseCode uint32) int {
// Plugin should return 200 on success, 400 for caller input errors, and 500 for everything else.
switch responseCode {
case 200:
return http.StatusOK
case 400:
return http.StatusBadRequest
default:
return http.StatusInternalServerError
}
}

func NewServer(logger *zap.SugaredLogger, manager plugin.IManager, options string) *Server {
return &Server{
logger: logger,
Expand Down Expand Up @@ -215,7 +227,7 @@ func (s *Server) RatsdChares(w http.ResponseWriter, r *http.Request, param Ratsd
if !out.Status.Result {
errMsg := fmt.Sprintf(
"failed to get attestation report from %s: %s ", pn, out.Status.Error)
p := problems.NewDetailedProblem(http.StatusInternalServerError, errMsg)
p := problems.NewDetailedProblem(responseCodeToHTTP(out.StatusCode), errMsg)
s.reportProblem(w, p)
return false
}
Expand Down
17 changes: 10 additions & 7 deletions attesters/mocktsm/mocktsm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package mocktsm
import (
"encoding/json"
"fmt"
"net/http"
"strconv"

"github.com/google/go-configfs-tsm/configfs/configfsi"
Expand Down Expand Up @@ -39,11 +40,12 @@ type MockPlugin struct {
client *faketsm.Client
}

func getEvidenceError(e error) *compositor.EvidenceOut {
func getEvidenceError(e error, statusCode uint32) *compositor.EvidenceOut {
return &compositor.EvidenceOut{
Status: &compositor.Status{
Result: false, Error: e.Error(),
},
StatusCode: statusCode,
}
}

Expand Down Expand Up @@ -78,13 +80,13 @@ func (m *MockPlugin) GetEvidence(in *compositor.EvidenceIn) *compositor.Evidence
errMsg := fmt.Errorf(
"nonce size of the mockTSM attester should be %d, got %d",
nonceSize, uint32(len(in.Nonce)))
return getEvidenceError(errMsg)
return getEvidenceError(errMsg, http.StatusBadRequest)
}

if in.ContentType != mediaType {
errMsg := fmt.Errorf(
"no supported format in mock TSM plugin matches the requested format")
return getEvidenceError(errMsg)
return getEvidenceError(errMsg, http.StatusBadRequest)
}
req := &report.Request{
InBlob: in.Nonce,
Expand All @@ -96,7 +98,7 @@ func (m *MockPlugin) GetEvidence(in *compositor.EvidenceIn) *compositor.Evidence
if err := json.Unmarshal(in.Options, &options); err != nil {
errMsg := fmt.Errorf(
"failed to parse %s: %v", in.Options, err)
return getEvidenceError(errMsg)
return getEvidenceError(errMsg, http.StatusBadRequest)
}
}

Expand All @@ -105,15 +107,15 @@ func (m *MockPlugin) GetEvidence(in *compositor.EvidenceIn) *compositor.Evidence
if err != nil || level < 0 {
errMsg := fmt.Errorf("privilege_level %s is invalid",
privlevel)
return getEvidenceError(errMsg)
return getEvidenceError(errMsg, http.StatusBadRequest)
}
req.Privilege = &report.Privilege{Level: uint(level)}
}

resp, err := report.Get(m.client, req)
if err != nil {
errMsg := fmt.Errorf("failed to get mock TSM report: %v", err)
return getEvidenceError(errMsg)
return getEvidenceError(errMsg, http.StatusInternalServerError)
}

out := &tokens.TSMReport{
Expand All @@ -125,12 +127,13 @@ func (m *MockPlugin) GetEvidence(in *compositor.EvidenceIn) *compositor.Evidence
outEncoded, err := out.ToJSON()
if err != nil {
errMsg := fmt.Errorf("failed to JSON encode mock TSM report: %v", err)
return getEvidenceError(errMsg)
return getEvidenceError(errMsg, http.StatusInternalServerError)
}

return &compositor.EvidenceOut{
Status: statusSucceeded,
Evidence: outEncoded,
StatusCode: http.StatusOK,
}
}

Expand Down
6 changes: 6 additions & 0 deletions attesters/mocktsm/mocktsm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package mocktsm
import (
"encoding/hex"
"fmt"
"net/http"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -65,6 +66,7 @@ func Test_GetEvidence_wrong_nonce_size(t *testing.T) {
Result: false,
Error: errMsg,
},
StatusCode: http.StatusBadRequest,
}

assert.Equal(t, expected, p.GetEvidence(in))
Expand All @@ -82,6 +84,7 @@ func Test_GetEvidence_invalid_format(t *testing.T) {
Result: false,
Error: "no supported format in mock TSM plugin matches the requested format",
},
StatusCode: http.StatusBadRequest,
}

assert.Equal(t, expected, p.GetEvidence(in))
Expand All @@ -106,6 +109,7 @@ func Test_GetEvidence_No_Options(t *testing.T) {
expected := &compositor.EvidenceOut{
Status: statusSucceeded,
Evidence: outEncoded,
StatusCode: http.StatusOK,
}

assert.Equal(t, expected, p.GetEvidence(in))
Expand Down Expand Up @@ -134,6 +138,7 @@ func TestGetEvidence_With_Invalid_Options(t *testing.T) {
Result: false,
Error: tt.msg,
},
StatusCode: http.StatusBadRequest,
}

assert.Equal(t, expected, p.GetEvidence(in))
Expand Down Expand Up @@ -161,6 +166,7 @@ func Test_GetEvidence_With_Valid_Privilege_level(t *testing.T) {
expected := &compositor.EvidenceOut{
Status: statusSucceeded,
Evidence: outEncoded,
StatusCode: http.StatusOK,
}

assert.Equal(t, expected, p.GetEvidence(in))
Expand Down
19 changes: 11 additions & 8 deletions attesters/tsm/tsm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package tsm
import (
"encoding/json"
"fmt"
"net/http"
"strconv"

"github.com/google/go-configfs-tsm/configfs/linuxtsm"
Expand Down Expand Up @@ -41,11 +42,12 @@ var (

type TSMPlugin struct{}

func getEvidenceError(e error) *compositor.EvidenceOut {
func getEvidenceError(e error, statusCode uint32) *compositor.EvidenceOut {
return &compositor.EvidenceOut{
Status: &compositor.Status{
Result: false, Error: e.Error(),
},
StatusCode: statusCode,
}
}

Expand Down Expand Up @@ -88,7 +90,7 @@ func (t *TSMPlugin) GetEvidence(in *compositor.EvidenceIn) *compositor.EvidenceO
errMsg := fmt.Errorf(
"nonce size of the TSM attester should be %d, got %d",
tsmNonceSize, uint32(len(in.Nonce)))
return getEvidenceError(errMsg)
return getEvidenceError(errMsg, http.StatusBadRequest)
}

for _, format := range supportedFormats {
Expand All @@ -103,7 +105,7 @@ func (t *TSMPlugin) GetEvidence(in *compositor.EvidenceIn) *compositor.EvidenceO
if err := json.Unmarshal(in.Options, &options); err != nil {
errMsg := fmt.Errorf(
"failed to parse %s: %v", in.Options, err)
return getEvidenceError(errMsg)
return getEvidenceError(errMsg, http.StatusBadRequest)
}
}

Expand All @@ -112,21 +114,21 @@ func (t *TSMPlugin) GetEvidence(in *compositor.EvidenceIn) *compositor.EvidenceO
if err != nil || level < 0 {
errMsg := fmt.Errorf("privilege_level %s is invalid",
privlevel)
return getEvidenceError(errMsg)
return getEvidenceError(errMsg, http.StatusBadRequest)
}
req.Privilege = &report.Privilege{Level: uint(level)}
}

client, err := linuxtsm.MakeClient()
if err != nil {
errMsg := fmt.Errorf("failed to create config TSM client: %v", err)
return getEvidenceError(errMsg)
return getEvidenceError(errMsg, http.StatusInternalServerError)
}

resp, err := report.Get(client, req)
if err != nil {
errMsg := fmt.Errorf("failed to get TSM report: %v", err)
return getEvidenceError(errMsg)
return getEvidenceError(errMsg, http.StatusInternalServerError)
}

out := &tokens.TSMReport{
Expand All @@ -148,16 +150,17 @@ func (t *TSMPlugin) GetEvidence(in *compositor.EvidenceIn) *compositor.EvidenceO
outEncoded, err := encodeOp()
if err != nil {
errMsg := fmt.Errorf("failed to encode TSM report as %s: %v", encodeAs, err)
return getEvidenceError(errMsg)
return getEvidenceError(errMsg, http.StatusInternalServerError)
}

return &compositor.EvidenceOut{
Status: statusSucceeded,
Evidence: outEncoded,
StatusCode: http.StatusOK,
}
}
}

errMsg := fmt.Errorf("no supported format in tsm plugin matches the requested format")
return getEvidenceError(errMsg)
return getEvidenceError(errMsg, http.StatusBadRequest)
}
22 changes: 15 additions & 7 deletions attesters/tsm/tsm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package tsm

import (
"fmt"
"net/http"
"testing"

"github.com/google/go-configfs-tsm/configfs/linuxtsm"
Expand All @@ -20,15 +21,19 @@ var (
)

func Test_getEvidenceError(t *testing.T) {
e := fmt.Errorf("sample error")
tests := []uint32 {http.StatusBadRequest, http.StatusBadRequest}
for _, tt := range tests {
e := fmt.Errorf("sample error")

expected := &compositor.EvidenceOut{
Status: &compositor.Status{
Result: false, Error: "sample error",
},
}
expected := &compositor.EvidenceOut{
Status: &compositor.Status{
Result: false, Error: "sample error",
},
StatusCode: tt,
}

assert.Equal(t, expected, getEvidenceError(e))
assert.Equal(t, expected, getEvidenceError(e, tt))
}
}

func Test_GetOptions(t *testing.T) {
Expand Down Expand Up @@ -87,6 +92,7 @@ func Test_GetEvidence_wrong_nonce_size(t *testing.T) {
Result: false,
Error: errMsg,
},
StatusCode: http.StatusBadRequest,
}

assert.Equal(t, expected, p.GetEvidence(in))
Expand All @@ -104,6 +110,7 @@ func Test_GetEvidence_invalid_format(t *testing.T) {
Result: false,
Error: "no supported format in tsm plugin matches the requested format",
},
StatusCode: http.StatusBadRequest,
}

assert.Equal(t, expected, p.GetEvidence(in))
Expand Down Expand Up @@ -133,6 +140,7 @@ func TestGetEvidence_With_Invalid_Options(t *testing.T) {
Result: false,
Error: tt.msg,
},
StatusCode: http.StatusBadRequest,
}

assert.Equal(t, expected, p.GetEvidence(in))
Expand Down
3 changes: 2 additions & 1 deletion proto/compositor.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,5 +60,6 @@ message EvidenceIn {

message EvidenceOut {
Status status = 1;
bytes evidence = 2;
uint32 statusCode = 2;
Copy link
Contributor

@thomas-fossati thomas-fossati Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this always supposed to carry an HTTP status code?
If so, I suggest we make it self-evident in the name.

Copy link
Collaborator Author

@cowbon cowbon Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call it httpStatusCode? I believe the usage is mainly to tell what type of HTTP error is encountered by the leaf attesters, though this patch sets the statusCode to be 200 when the API call is successful.

Copy link
Contributor

@thomas-fossati thomas-fossati Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coupling the REST API with the backend protocol may or may not be a good idea 😄

This kind of coupling can be problematic if we want to reuse the backend protocol with another API (e.g., CoAP), for example.

A better, albeit clunkier, approach would be to define the backend protocol and its own codes, and have a mapping function defined by each frontend protocol (HTTP, CoAP, other).

Copy link
Collaborator Author

@cowbon cowbon Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked over RFC 7252 and it seems CoAP also uses 400 for Bad Requests, 500 for internal server errors. Now, all errors reported by the leaf attesters are treated as 500 Internal Server Error even the error is caused by the client (e.g. invalid nonce), and we want to categorize the errors returned by the leaf attesters. To make the naming of statusCode more self-evident, we could call it subattesterResponseCode, or just respondCode.
Given ratsd core is still HTTP-based, i.e. writing to the httpResponseWriter, should we adopt 400 Bad Request, 500 Internal Server error from HTTP/CoAP as the statusCode between the lead attester and the leafs, and defer the decoupling until we refactor the ratsd core to support other protocols using the same backend?

Copy link
Contributor

@thomas-fossati thomas-fossati Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that the two categories you have highlighted are good:

  • caller input errors (that map to HTTP 4xx and CoAP 4.xx)
  • business logic error (that map to HTTP 5xx, CoAP 5.xx)

If we also add a "frontend protocol” error mapper, e.g.:

func responseCodeToHTTP(uint32 responseCode) (code int) {
    // switch ...
}

we are good to go.

Copy link
Collaborator Author

@cowbon cowbon Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the mapper in the server code.

bytes evidence = 3;
}
20 changes: 15 additions & 5 deletions proto/compositor/compositor.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.