Cache-Control HTTP header for server endpoints

[km274]

Add Cache-Control: private, no-store HTTP header to server endpoints that respond with sensitive info.

Fixes #793

I decided which endpoints to add the header to by investigating each endpoint and the data it returns.

Please let me know if it would be a good idea to update or add tests to check for the new header where needed. My own judgment as of the time of opening this PR was to not update the tests because 1) existing tests aren't checking HTTP response headers from what I saw and 2) I felt like the addition of a header might be too trivial to be worth adding new test code.

[km274]

I checked out the CI test failure (see excerpt from logs below). Since this particular failure is present in some other recent CI runs and isn't related to my changes, I'll just continue to wait for feedback.

CGO_ENALBED=1 gotestsum -- -coverprofile=tpmsimulatorcoverage.out -short -covermode=atomic -tags tpmsimulator ./acme 
# github.com/smallstep/certificates/acme [github.com/smallstep/certificates/acme.test]
Error: acme/challenge_tpmsimulator_test.go:52:8: assignment mismatch: 1 variable but simulator.New returns 2 values
WARN invalid TestEvent: FAIL	github.com/smallstep/certificates/acme [build failed]
bad output from test2json: FAIL	github.com/smallstep/certificates/acme [build failed]

=== Errors
Error: acme/challenge_tpmsimulator_test.go:52:8: assignment mismatch: 1 variable but simulator.New returns 2 values

DONE 0 tests, 1 error in 25.586s
make: *** [Makefile:99: testtpmsimulator] Error 2
Error: Process completed with exit code 2.

[hslatman]

Hey @kippmorris7, you're right about the CI issue. I fixed it yesterday with #1464. If you merge the latest master, yours will run OK too.

[areed]

Webhook create and update responses contain secrets and should not be cached.

Provisioner responses never contain sensitive data.

ACME responses contain some data that may be considered sensitive. I'm not certain how to treat these.

[km274]

Thank you for the review, @areed ! After looking back over my work, I agree that we can remove the header in most of the places you pointed out, but I think we still need it on the provisioner responses.

[areed]

@kippmorris7 This is the type used for webhooks when they appear in provisioners. The secret and credentials are always omitted.

[km274]

@kippmorris7 This is the type used for webhooks when they appear in provisioners. The secret and credentials are always omitted.

I see. In that case I understand why we can remove the header from Provisioners in api/api.go. I double checked the linkedca.Provisioner though, and it looks like the the secrets will not be omitted there:

• Link to linkedca.Provisioner: https://github.com/smallstep/linkedca/blob/1389fd5b2d901b09abf84d678c89f63c3d29ae54/provisioners.pb.go#L324
• Link to nested Webhook type: https://github.com/smallstep/linkedca/blob/1389fd5b2d901b09abf84d678c89f63c3d29ae54/provisioners.pb.go#L1950

With this in mind, it seems to me that in authority/admin/api/provisioners.go, we can safely remove the header from GetProvisioners, but we should leave it in the other functions, since they respond with linkedca.Provisioners. What do you think?

[areed]

With this in mind, it seems to me that in authority/admin/api/provisioners.go, we can safely remove the header from GetProvisioners, but we should leave it in the other functions, since they respond with linkedca.Provisioners. What do you think?

Yes, you're right.

[km274]

Cool, I removed the header from two of the provisioner responses and left it on the others like we discussed.

We still need to address the ACME endpoints. Having done my reading, I'm ready to remove them based on my own understanding, but I wasn't clear on whether you were waiting for a third opinion first.

[areed]

We still need to address the ACME endpoints. Having done my reading, I'm ready to remove them based on my own understanding, but I wasn't clear on whether you were waiting for a third opinion first.

After reviewing the security considerations of the token I'm comfortable removing the headers for ACME endpoints also.

[areed]

@hslatman Would the SCEP response data ever contain sensitive information? From what I can tell it's either a certificate or a list of capabilities and would never contain the challenge secret.

[hslatman]

Any secrets are encrypted before being sent to the CA, and not returned to clients, so no obvious sensitive information is leaked. There's some bits of SCEP transaction related information that can be extracted, such as transaction ID and nonces, but I don't think these leak information; at least not directly.

While some responses to GET requests could be cached for a specific amount of time (e.g. GetCACert, GetCACaps), I feel responses to POST requests must not be cached. The last (informational) SCEP RFC doesn't say much about caching, only that it has caused issues in the past with large requests and when requests weren't actually idempotent.

We currently support and signal the more modern POSTPKIOperation (via POST), so clients generally don't send the large GET requests. The responses for POST requests contain client specific results and are unlikely to be fully idempotent, so IMO shouldn't be cached.

In terms of interoperability: I would have to take a look at other SCEP servers if they ever set cache headers. I think clients should simply ignore them when they don't understand them or can't do anything with them, but SCEP being legacy, there's a lot of old systems that may be doing weird things.

[areed]

@kippmorris7 Let's remove this one and then the PR will be good to merge.

[km274]

I've removed it. To double check my understanding, why did you decide against keeping the headers on the POST responses?

[tashian]

I'm glad this is finally being addressed; thank you @kippmorris7 !

[km274]

Hey everyone, I just rebased since it's been a while. Is there anything else you'd like me to take another look at before this is ready to merge?