[SERVER-977] DELETE handler not implemented in the certificate_request API Created: 2015/10/20  Updated: 2016/05/19  Resolved: 2016/04/19

Status: Closed
Project: Puppet Server
Component/s: None
Affects Version/s: None
Fix Version/s: SERVER 2.4.0

Type: Bug Priority: Normal
Reporter: Matthew Gyurgyik Assignee: Unassigned
Resolution: Fixed Votes: 0
Labels: puppethack
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Relates
relates to SERVER-1272 Create test suite to validate the Pup... Closed
relates to SERVER-913 Resurrect and Refactor the HTTP CA AP... Closed
relates to SERVER-1257 Add section to docs to indicate puppe... Closed
Template:
Sub-team: jade
Story Points: 1
Sprint: Server Jade 2016-04-06, Server Jade 2016-04-20
QA Contact: Erik Dasher

 Description   

API Documentation indicates the certificate_request should support destroy/delete.
https://docs.puppetlabs.com/puppet/4.2/reference/http_api/http_certificate_request.html#destroy

$ curl -X GET -H 'Accept: s' -k --cert /home/msg31/.config/nasi/infra-mgmt.ccs.ornl.gov.cert.pem --key /home/msg31/.config/nasi/infra-mgmt.ccs.ornl.gov.key.pem https://puppetca1.ccs.ornl.gov:8140/puppet-ca/v1/certificate_request/msg31-test2.ccs.ornl.gov?environment=production

Returns the CSR (works as expected)

$ curl -X DELETE -H 'Accept: s' -k --cert /home/msg31/.config/nasi/infra-mgmt.ccs.ornl.gov.cert.pem --key /home/msg31/.config/nasi/infra-mgmt.ccs.ornl.gov.key.pem https://puppetca1.ccs.ornl.gov:8140/puppet-ca/v1/certificate_request/msg31-test2.ccs.ornl.gov?environment=production

Returns a 404 Not Found (not expected/broken)

After a conversation with camlow325, KevinCorcoran, and cprice404 it appears this not implemented. See code: https://github.com/puppetlabs/puppet-server/blob/master/src/clj/puppetlabs/services/ca/certificate_authority_core.clj#L265-L269

Relevant sections of the IRC log
<camlow325> nwolfe: Hmm, kind of looks like we aren't supporting DELETE for the certificate_request API in the Puppet Server CA. Agree?
<camlow325> I don't see a DELETE handler for it - https://github.com/puppetlabs/puppet-server/blob/master/src/clj/puppetlabs/services/ca/certificate_authority_core.clj#L265-L269.
<KevinCorcoran> camlow325: pyther: Yeah, that looks like a bug.
<KevinCorcoran> I don't think we left it out on purpose.
<camlow325> KevinCorcoran: Yeah, I think you're right
<KevinCorcoran> Looks like we missed that at the very beginning with Puppet Server, we've never supported DELETE on that endpoint. Not a recent regression. Ya know, I really hate how with compojure/comidi you just get a 404 when you get the verb wrong.



 Comments   
Comment by Kevin Corcoran [ 2016/02/24 ]

The old CA implementation seems to return the number of certificate requests which were deleted ...

DELETE /puppet-ca/v1/certificate_request/agency?environment=production
Accept: s
 
HTTP/1.1 200 OK
Content-Type: text/plain
 
1

Instead I suggest returning HTTP 204 No Content.

Comment by Justin Stoller [ 2016/03/31 ]

Looking further into this the delete handler for the certificate_status endpoint will delete any certificate and certificate request (docs, endpoint, impl). I'm wondering if this wasn't not implemented because it's duplicated functionality and the documentation just wasn't updated?

Regardless, I didn't find that information out until I did a first pass at implementing my own handler for deleting just certificate requests.

My first question is should we implement a delete handler for the certificate_request endpoint since that functionality is available in the certificate_status endpoint or should we instead update the documentation?

If the answer to that is to implement the delete method for the certificate_request endpoint, I have some questions regarding the best approach to the implementation in this PR for review.

Comment by Jeremy Barlow [ 2016/03/31 ]

Good points, Justin Stoller.

While it's true that the certificate_status endpoint could be used to delete a certificate request, I think there could be a use case related to authorization rules for making it possible to remove a certificate request via the certificate_request API even though the certificate_status API provides the same functionality. For example, some users might want to create an authorization rule that allows the certificate_request API to be used by clients to delete their own requests but not more generally give clients (especially ones without authenticated certificates) the ability to manage the state of both certificate requests and certificates themselves. We don't really have a way with the certificate_status API to craft custom rules that apply to just certificate requests vs. certificates since those aren't encoded into the URI request path or a query parameter. I can't envision where it would ever be desirable to grant access to the certificate_status endpoint for clients that don't have authenticated certificates whereas the certificate_request endpoint generally has to be available to unauthenticated clients since the typical clients of the endpoint don't actually have a certificate yet. Granted, this use case could be dangerous as well in that if the DELETE method for the certificate_status endpoint were opened up to all users - authenticated or not - then an unscrupulous client could use that endpoint to remove certificate requests made by users other than themselves.

In any event, it still seems we should implement support for the DELETE method on the certificate_request API for backward compatibility. In a future version of the CA API, though, I definitely think it would make sense to discuss whether or not we need to preserve the ability to delete certificate requests across multiple APIs.

Curious to hear Kevin Corcoran's and/or Chris Price's take.

Comment by Justin Stoller [ 2016/03/31 ]

That makes a lot of sense, Jeremy Barlow. I can definitely see why we would do both now.

Comment by Justin Stoller [ 2016/04/12 ]

This commit has gone through CI up to the smoke test stage. I believe all the tests necessary to validate it are ran within that step. The pipeline has not continued because of a different issue, failures in the pipeline were all unrelated to this patch.

Not sure if this ticket is blocked until we have a green build through the full pipeline, or if can move forward based on the preliminary results.

See https://jenkins.puppetlabs.com/view/puppet-server/view/master/job/platform_puppet-server_integration-system_no-conditional_smoke-master/223/LAYOUT=centos7-64ma-64a,LDAP_TYPE=default,PLATFORM=default,label=beaker/ for example.

Comment by Kevin Corcoran [ 2016/04/12 ]

We typically wait for CI to go completely green before advancing tickets, but that's not a hard and fast rule. However, if this is testable by QA now, it probably makes sense to go ahead and move it over.

Comment by Justin Stoller [ 2016/04/12 ]

Cool. Thanks, Kevin.

Erik Dasher this should be manually testable/reviewable at this point (assuming I understand that process - its been EZBaked). I'll leave this in this JIRA state until its passed all the stages in the CI pipeline, but if I understood the pipeline correctly it's passed against all tests but not against all platforms, but there's no platform specific code in the patch.

Generated at Wed Nov 13 13:06:43 PST 2019 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.