[HOPSWORKS-579] Refactor Hopsworks-ca for multi intermediate CAs #934
Conversation
|
Work in progress, early feedback welcome |
| throw new AppException(Response.Status.INTERNAL_SERVER_ERROR, "Error retrieving CAs certificates"); | ||
| } | ||
|
|
||
| CSRView signedCsr = new CSRView(csrView.getCsr(), signedCert, chainOfTrust.getValue0(), chainOfTrust.getValue1()); |
There was a problem hiding this comment.
Can we skip sending back the CSR?
| throw new AppException(Response.Status.INTERNAL_SERVER_ERROR, "Error retrieving CAs certificates"); | ||
| } | ||
|
|
||
| CSRView signedCsr = new CSRView(csrView.getCsr(), signedCert, chainOfTrust.getValue0(), chainOfTrust.getValue1()); |
There was a problem hiding this comment.
Same here, can we skip sending back the CSR?
| } | ||
|
|
||
| @DELETE | ||
| public Response revokeCertificate(@QueryParam("certId") String certId) throws AppException { |
There was a problem hiding this comment.
NOTE: This assumes that kagent will handle the current certificate version
| @@ -258,7 +264,10 @@ public void validateCertificate(X509Certificate certificate) throws IOException | |||
| commands.add(OPENSSL); | |||
| commands.add("verify"); | |||
| commands.add("-CAfile"); | |||
| commands.add(Paths.get(settings.getIntermediateCaDir(), "certs", "ca-chain.cert.pem").toString()); | |||
| commands.add(Paths.get(pki.getCACertsDir(caType), "ca-chain.cert.pem").toString()); | |||
There was a problem hiding this comment.
This should be a method in PKI. Depending on the intermediate CA the chain file name "ca-chain.cert.pem" might differ
| } | ||
| } | ||
|
|
||
| public String getCAConfPath(CAType caType) { |
There was a problem hiding this comment.
It would be better for type safety the return type to be Path
| } | ||
| } | ||
|
|
||
| public String getCACertsDir(CAType caType) { |
There was a problem hiding this comment.
Same here, return type Path
| return Paths.get(getCAParentPath(caType), "certs").toString(); | ||
| } | ||
|
|
||
| public String getCACRLPath(CAType caType){ |
SirOibaf
changed the title
|
Tests passed. @tkakantousis whenever you want give it the final ok and merge |
There was a problem hiding this comment.
I see that you tried removing the Dela related signing part from the CertSigningService into DelaTrackerCertController and DelaTrackerCertService, however I see that the CertSigningService still uses DelaTrackerCertController. If this service depends on that controller, should we name the controller Dela... ? it seems that the controller is not Dela specific.
Looking at the code I see that you didn't change the paths when spliting the service into Dela... Can I assume no api (path, params) change to the signing service as part of dela?
| } | ||
|
|
||
| private CAExceptionErrors error; | ||
| private CertificateType certType; |
There was a problem hiding this comment.
@SirOibaf Hops custom exceptions in v2 will extend RESTException, which brings two extra fields for passing runtime information to the users and developers using the API. For example you might want to return for which file was the CERTNOTFOUND thrown and to the developer you might want to return the entire stacktrace. Just for making it easier when the code is migrated to v2, can you add these two fields in the exception? userMsg and devMsg.
| } | ||
|
|
||
| @DELETE | ||
| public Response revokeCertificate(@QueryParam("certId") String certId) throws CAException, IOException { |
There was a problem hiding this comment.
@SirOibaf Add swagger API annotations for all endpoints. https://github.com/swagger-api/swagger-core/wiki/annotations#apiparam
Particularly for mandatory queryparams and the response view class. For example
@ApiOperation(value = "Find Execution by Id", response = ExecutionDTO.class)
Make sure there is no duplicate PR for this issue
Post a link to the associated JIRA issue
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information: