fix: send back 400 status code on bad requests

[jahzielv]

Related issue: fleetdm/fleet#15635

• Allow caller to set handler path on scepserver.MakeHTTPHandler
• fix: return bad request errors when requests fail validation
• feat: some tests

[roperzh]

sorry, I requested a review by accident.

[mna]

Looks great! Just some concerns about the new validations, to make sure it doesn't break what were previously (I think?) ignored/no-ops requests.

[mna]

Might be worth double-checking by @roperzh who's more familiar than I am with the macOS MDM flow - my concern is that this could generate errors from requests that currently succeeded (but presumably ignored/no-ops) if there was no operation?

[roperzh]

to be honest I had to go and look at the spec because I didn't know the answer to this.

from what I can see, some messages can have empty operations so +1 to remove this.

[jahzielv]

Gotcha; fwiw, currently if you leave out the operation, fleet responds with a 500. Removing this code would revert back to that behavior. Should I instead update the code to allow empty operations? Ditto for an empty message (like here) cc @roperzh

[mna]

currently if you leave out the operation, fleet responds with a 500.

Ah gotcha, yeah if it did generate an error previously, then the added validation makes sense to me. I thought this case might've been a successful no-op before.

[roperzh]

I think you're right about operation, taking a second look here:

https://github.com/fleetdm/scep/blob/3aa7a5a9d937a028c081e46f605c4a85fbcdadde/server/endpoint.go#L137-L149

just as a curiosity, I also see this:

Note that when used with HTTP POST, the only OPERATION possible is "PKIOperation", so many CAs don't check this value or even notice its absence

[mna]

Here too, since we're adding new validation, just to make sure that we're not erroring on what is possibly a valid request that was previously ignored?

[roperzh]

agreed on this one as well.

[roperzh]

I'm thinking: what about clients that use POST? (in SCEP you use a single path, and can use GET or POST, all communications are handled by parameters)

wonder if we should add the check upstream so it works independent of the request method used?

https://github.com/fleetdm/scep/blob/3aa7a5a9d937a028c081e46f605c4a85fbcdadde/server/service.go#L75

[jahzielv]

@roperzh oh, I didn't realize that was the case! yeah, if you can use POST or GET for PKIOperations, then we should check before here. I can update that.

Something like this?

func message(r *http.Request) ([]byte, error) {
	var msg string
	q := r.URL.Query()
	if _, ok := q["message"]; ok {
		msg = q.Get("message")
	}
	if len(msg) == 0 {
		return nil, &BadRequestError{Message: "missing PKIOperation message"}
	}
	switch r.Method {
	case "GET":
		op := q.Get("operation")
		if op == "PKIOperation" {
			msg2, err := url.PathUnescape(msg)
			if err != nil {
				return nil, &BadRequestError{Message: fmt.Sprintf("invalid PKIOperation message: %s", msg)}
			}

			decoded, err := base64.StdEncoding.DecodeString(msg2)
			if err != nil {
				return nil, &BadRequestError{Message: fmt.Sprintf("failed to base64 decode message: %s: %s", err.Error(), msg2)}
			}

			return decoded, nil
		}
		return []byte(msg), nil
	case "POST":
		return ioutil.ReadAll(io.LimitReader(r.Body, maxPayloadSize))
	default:
		return nil, errors.New("method not supported")
	}
}

[roperzh]

@jahzielv I think that won't do it for POST because the message comes in the request body 🤔

If it's just operation=PKIOperation we're worried about (which seems reasonable for now), what do you think about doing it in the service (link I pointed above)

if len(data) == 0 {
    // bad request!
}

[jahzielv]

🤦 gotcha. yeah, needed a small update to another part of the code, but that works! updated @roperzh

[mna]

👍 Once we move this to the monorepo, we'll probably be able to reuse the error type from the service package, but for now this LGTM!

[mna]

Nit, but if it doesn't fan out to too many changes, we can use t.Cleanup(func() {...}) instead of returning the teardown function, so the caller doesn't have to remember to defer that function call.

[roperzh]

Martin review covers everything, I just want to add a note to sanity check between you and @mna (as this is going to move to the monorepo soon)

• fleetdm/fleet currently uses a branch in this repo named remove-path-setting-on-scep-handler
• this PR contains the changes in remove-path-setting-on-scep-handler and targets main

anything works IMO, just wanted to make sure we're all on the same page

[roperzh]

lgtm!