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 https://github.com/smallstep/certificates/pull/1464. If you merge the latest master, yours will run OK too.

[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.

[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?