For unknown certificates OCSP should have unknown CertStatus (part 2) #4534
Conversation
The CA's internal OCSP fails to handle certs issued by an unknown CA. There is code in the CA's validation to handle that scenario but that validation is never triggered as the request handling code that wraps it considers not knowing the origin CA to be an error condition. The code is changed to allow the validating CA to proceed even if the origin CA is unknown, reporting Unknown for the CertStatus, while delegating to the origin CA if it is found.
|
@edewata @fmarco76 @ladycfu - there is an issue with the on-demand compose tool I was using to produce a test module. Therefore, this has not been tested in any way. I shall report back with findings when I have them but the fix is small so I pushed it anyway so you guys can see. Maybe one of you is more successful than me at testing it :-D |
| } catch (EBaseException e) { | ||
| // If we don't know the issuer allow this CA to validate | ||
| // and report the CertStatus as Unknown | ||
| } |
There was a problem hiding this comment.
I think here we should catch EDBRecordNotFoundException specifically (see LDAPSession.read()) which indicates that the cert doesn't exist in CA's cert repository. Other exceptions might indicate there's a real issue in CA so we should let it bubble up like before so it can be handled properly by the caller.
|
For testing I'd recommend creating an upstream CI test. These changes are not OS-specific, so we should be able to test it on Fedora as well. Later QE can verify the fix on the actual build for RHEL. |
In case of multiple CAs the correct one was selected using the first certificate. This could provide inconsistent. Now the selection is based on the request issuer name. Additionally, the output has been made consistent with the external OCSP for all the possibilities of subject and issuers.
| * found, dispatch to its 'validate' method. Otherwise | ||
| * continue. | ||
| * by its DN in CAEngine. If found, dispatch to its 'validate' | ||
| * method. Otherwise continue. | ||
| * | ||
| * 3. If this CA is NOT the issuing CA, we locate the | ||
| * issuing CA and dispatch to its 'validate' method. | ||
| * Otherwise, we move forward to generate and sign the | ||
| * aggregate OCSP response. | ||
| */ |
There was a problem hiding this comment.
@fmarco76 I suggest that you request @frasertweedale for a review here since it seems have touched the lightweight CA area.
There was a problem hiding this comment.
On second thought, looking closely, although it added additional name/key hash checks, the code path should be the same for the lightweight CA case. I think it should be safe for v10.13 for now. We can hold off porting to other branches till we hear back from @frasertweedale
There was a problem hiding this comment.
Yes, the path is the same. The reason to look at the right CertificationAuthority is to get the correct DN and Public Key to verify. The improvement could be to perform the test for each certificate in the request.
| // not found | ||
| certStatus = new UnknownInfo(); // not issued not all | ||
| certStatus = new GoodInfo(); // not issued not all |
There was a problem hiding this comment.
despite what's being said in RFC 6960 wrt "good" response, since this is inside the CA, who knows for sure that it is not a cert issued by itself, I think "unknown" is a better fit for this response (v.s. standalone ocsp who has no means of finding out). In the "note" area of the rfc in that section, it says ' the "unknown" status indicates that the status could not be determined by this responder' so making it "unknown" would be fine I think.
There was a problem hiding this comment.
Last version of the spec suggest to provide reject status in this case (or good for backward compatibility) so the client does not have to look at other responders. I leave this decision to you.
There was a problem hiding this comment.
@fmarco76 as a matter of fact, I had an about face after looking at the rfc more closely. It should return "revoked" in this case. However, not having time (minutes away from close of sprint) to find a handy way to construct a "revoked" response at the time, I opted to assign unknown to it. I think that's what the lab is expecting from the emails I received. Either way it's still better than returning "good". This is a rare case anyway, so we can leave it to the lab and patch it if needed. Thanks!
| @@ -1836,9 +1843,12 @@ private SingleResponse processRequest(Request req) { | |||
| } else { | |||
| certStatus = new UnknownInfo(); | |||
| } | |||
| } catch (Exception e) { | |||
There was a problem hiding this comment.
Are we sure there are no other type of exceptions? This catch all exception in general is a safety net in a pinch, if not the best practice.
|
@fmarco76 I did a few tests which all turned out to be working as expected. One thing though is that when submitted an ocsp request to the CA, in the debug log all I see was the following: 2023-08-10 18:50:04 [http-nio-21080-exec-5] INFO: AAclAuthz: Granting submit permission for certServer.ee.request.ocsp I see that the sign() method in ca/src/main/java/com/netscape/ca/CASigningUnit.java generates that comment. I think the sign() method was probably initially only used for signing certificates. It should probably simply say "signing ..." |
| * found, dispatch to its 'validate' method. Otherwise | ||
| * continue. | ||
| * by its DN in CAEngine. If found, dispatch to its 'validate' | ||
| * method. Otherwise continue. | ||
| * | ||
| * 3. If this CA is NOT the issuing CA, we locate the | ||
| * issuing CA and dispatch to its 'validate' method. | ||
| * Otherwise, we move forward to generate and sign the | ||
| * aggregate OCSP response. | ||
| */ |
There was a problem hiding this comment.
On second thought, looking closely, although it added additional name/key hash checks, the code path should be the same for the lightweight CA case. I think it should be safe for v10.13 for now. We can hold off porting to other branches till we hear back from @frasertweedale
The CA's internal OCSP fails to handle certs issued by an unknown CA.
There is code in the CA's validation to handle that scenario but that
validation is never triggered as the request handling code that wraps it
considers not knowing the origin CA to be an error condition.
The code is changed to allow the validating CA to proceed even if the
origin CA is unknown, reporting Unknown for the CertStatus, while
delegating to the origin CA if it is found.
This finishes what was started with #4532