853812 - Expose OCSP POST function to applications

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Expose more certificate validation building blocks to applications

This patch does three things:

1. It adds "const" to many parameters of NSS functions related to certificates, to make it clear that those functions are pure, at least with respect to the certificate passed in. The distinction between pure and non-pure functions in NSS is not obvious and these const declarations help a lot in understanding things. Note that I use the "const SECItem*" construct often to mean "pointer to a buffer that won't be modified" even though the compiler will not enforce that since SECItem.data is non-const.

2. It exposes the function CERT_PostOCSPRequest, so that utilities and tests can post OCSP requests and receive responses just like NSS does internally, but with the added flexibility of being able to encode the request and/or decode the response themselves.

3. It exposes a few NSS functions that are useful for validating certificates. The new CERT_FindOIDInOIDSeqByTagNum function can be used, for example, to search a certificate's EKU extension for a particular EKU with minimal (by NSS's standards) overhead.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 728156 [details] [diff] [review]
Expose more certificate validation building blocks to applications

Review of attachment 728156 [details] [diff] [review]:
-----------------------------------------------------------------

::: coreconf/WIN32.mk
@@ +169,5 @@
>  ifneq ($(_MSC_VER),$(_MSC_VER_6))
>      # Convert certain deadly warnings to errors (see list at end of file)
>      OS_CFLAGS += -we4002 -we4003 -we4004 -we4006 -we4009 -we4013 \
>       -we4015 -we4028 -we4033 -we4035 -we4045 -we4047 -we4053 -we4054 -we4063 \
> +     -we4064 -we4078 -we4087 -we4090 -we4098 -we4390 -we4551 -we4553 -we4715

Making warning 4090 into an error has the useful effect that Visual C++ will stop the build wherever you forgot to convert a "X*" to a "const X*" during the course of adding "const" to parameter declarations. This makes it much easier to review the const-related parts of the patches, because you don't have to worry about something being overlooked, except in the rare cases where const_casting had to be done. (The cases where I const_cast are clearly labeled.)

Comment 2

[Ryan Sleevi]

Comment on attachment 728156 [details] [diff] [review]
Expose more certificate validation building blocks to applications

Review of attachment 728156 [details] [diff] [review]:
-----------------------------------------------------------------

I would ask that you split out #1 (the constification) from this patch - along with the warning change.

Now that we've switched from CVS, hopefully that will be easy. It's important to separate out "functional" and "non-functional" changes, and the const-ification seems entirely orthogonal to what this change is doing. The same with the warning change. I think they're both *great* changes, I just don't think we should mix them with a functionality change (or with the same bug, when it comes to generating release notes)

I've marked this r- because I think more documentation is needed if these are going to be part of the public API. I have also not reviewed this patch as closely as I'd like, in part because of the const-ification changes. It's good to be able to review the necessary and sufficient parts independently.

::: lib/certdb/cert.h
@@ +1653,5 @@
>   */
>  extern void
>  CERT_DestroyCERTRevocationFlags(CERTRevocationFlags *flags);
>  
> +SECStatus

Please add comments describing this function.

/*
 * Searches for "tagnum" within an encoded OID sequence.
 * If the encoded sequence is valid, returns SECSuccess and
 * sets "found" to whether or not the OID was present in the
 * sequence. On error, returns SECFailure.
 */

For example, the goal of the above comment is to try to make it clear that it returns *SECSuccess* if the OID is *not* found - since it uses the PRBool to indicate whether or not its actually found.

@@ +1657,5 @@
> +SECStatus
> +CERT_FindOIDInOIDSeqByTagNum(const SECItem * encodedOIDSeq, SECOidTag tagnum,
> +			     /*out*/ PRBool * found);
> +
> +/* Returns detailed CRL cache revocation status of the cert. */

Please provide a more expanded comment. Some of your variable names, such as "dp" or "t", are not sufficiently clear. I'm guessing you mean "distribution points" and "verification time".

You should also indicate success/failure conditions.

::: lib/certdb/certdb.c
@@ +476,5 @@
> +    if (!found) {
> +	PORT_SetError(SEC_ERROR_INVALID_ARGS);
> +	return SECFailure;
> +    }
> +    *found = PR_FALSE;

Is it NSS convention to update output parameters even on error?

At least within libpkix, I didn't think it wrote to the out params until the very end.

@@ +478,5 @@
> +	return SECFailure;
> +    }
> +    *found = PR_FALSE;
> +
> +    seq = cert_DecodeOidSequence(encodedOIDSeq, PR_FALSE);

This means that repeated calls to this function will have to re-decode the OID sequence. Should we instead be looking at making cert_DecodeOidSequence public (or perhaps it already has a CERT_ equivalent API)?

::: lib/certhigh/ocsp.c
@@ +3554,5 @@
> +    registeredHttpClient = SEC_GetRegisteredHttpClient();
> +
> +    if (registeredHttpClient
> +            &&
> +            registeredHttpClient->version == 1) {

This is a particularly odd indentation style for NSS.

::: lib/certhigh/ocsp.h
@@ +698,5 @@
>   */
>  extern SECItem*
>  CERT_CreateEncodedOCSPErrorResponse(PLArenaPool *arena, int error);
>  
> +SECItem* CERT_PostOCSPRequest(PRArenaPool *arena, const char *location,

Please extensively document this function.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 1: add const modifier to many certificate-related functions

Thanks for the quick review, Ryan. I am splitting up the patches now.

Comment 4

[Ryan Sleevi]

Comment on attachment 728460 [details] [diff] [review]
Part 1: add const modifier to many certificate-related functions

Review of attachment 728460 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for splitting this up.

Overall, I've mostly got some laments here and some "low hanging fruit while you're at it because surely it's no hassle, right? oh?" comments, it would be great to fix, but it's not wildly inconsistent with the existing code.

Two requests:
1) Can you file (and land this patch) under a new bug, just so it's clear the patch<-> bug relation. As it is, this patch on its own (which is *great*) has nothing obvious to do with the bug, and it'd be good to be able to call this out separately in the release notes.
2) Please check with Bob or Wan-Teh regarding the /*const_cast*/ remark. As I mention below, I understand why, but it seems wrong & unnecessary to me, and so I'd prefer not to include it - unless they WOULD prefer to include it.

That said, because my comments are whitespace-or-delete-comments, I'm happy to r+ this.

::: cmd/lib/derprint.c
@@ +179,5 @@
>      rv = prettyPrintStringStart(out, str, len, level);
>      if (rv < 0)
>  	return rv;
>  
> +    time_item.data = /*const_cast*/(unsigned char *)str;

I'm not sure the purpose of these comments. Because NSS compiles as C code (and presumably will continue to do so for some time), these references to the C++ functions don't mean much.

I realize that the goal may be to indicate "No, really, it's OK", but it seems superfluous/unnecessary. I'd defer to Bob or Wan-Teh to see if they have an opinion on this; my own is that I understand why, but it looks/seems wrong.

::: lib/certdb/cert.h
@@ +1162,3 @@
>  
>  SECStatus CERT_DecodeInhibitAnyExtension
>      (CERTCertificateInhibitAny *decodedValue, SECItem *extnValue);

lament: The inconsistent wrapping in this file (eg: compare with line 1159, which keeps the "(" on the same line as the function - and with line 895, which does NOT) makes me particularly sad.

It'd be good to clean these up - but not necessary in this patch unless it also bothers you as much as it does me :)

::: lib/certdb/crl.c
@@ +1726,1 @@
>                           CERTCrlEntry** returned)

low hanging fruit: Is it worth fixing the indentation here since you're touching line 1725?

::: lib/certdb/genname.c
@@ +1340,5 @@
>  ** It returns SECFailure if the name fails to satisfy the constraints,
>  ** or if some code fails (e.g. out of memory, or invalid constraint)
>  */
>  SECStatus
> +cert_CompareNameWithConstraints(const CERTGeneralName     *name, 

lament: trailing whitespace here.

@@ +1465,5 @@
>  	}
>  	if (matched == SECSuccess || rv != SECSuccess)
>  	    break;
> +	current = CERT_GetNextNameConstraint(
> +				/*const_cast*/(CERTNameConstraint*)current);

odd indentation here. It doesn't match what you've updated elsewhere.

::: lib/certhigh/certvfy.c
@@ +37,5 @@
>  /*
>   * verify the signature of a signed data object with the given DER publickey
>   */
>  SECStatus
> +CERT_VerifySignedDataWithPublicKey(const CERTSignedData *sd, 

lament: trailing whitespace.

::: lib/util/hasht.h
@@ +5,5 @@
>  
>  #ifndef _HASHT_H_
>  #define _HASHT_H_
>  
> +#include "prtypes.h"

Why was this necessary?

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 1: Expose some existing APIs to callers outside of NSS

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 2: Expose NSS's OCSP POST logic to (test) programs

Ryan, I know there are all kinds of ways to improve this API to make it more flexible or whatnot. However, I think that in general applications that really care will just end up taking over the formatting and processing of the HTTP requests themselves. The intention of this patch is not to create something super useful for many applications, but rather to expose the fetching logic in a more flexible way to test programs like tstclnt and vfychain.

Comment 7

[Ryan Sleevi]

Comment on attachment 729338 [details] [diff] [review]
Part 2: Expose NSS's OCSP POST logic to (test) programs

Review of attachment 729338 [details] [diff] [review]:
-----------------------------------------------------------------

Overall seems reasonable, when combined with the functions exposed in NSS 3.6. So r+, but one note below.

::: lib/certhigh/ocsp.c
@@ +3553,5 @@
> +
> +    registeredHttpClient = SEC_GetRegisteredHttpClient();
> +
> +    if (registeredHttpClient && registeredHttpClient->version == 1) {
> +        encodedResponse = fetchOcspHttpClientV1(arena,

Your wrapping style here seems inconsistent with the rest of the file.

It seems there are two wrapping styles prevalent:

Either wrap and align all args to the opening paren (eg: see line 3759) or indent one level and put each arg on a new line (eg: see 3406)

I'm not sure if this is just an artifact of tabs + splinter, but it looks intentional

Comment 8

[Ryan Sleevi]

Comment on attachment 729323 [details] [diff] [review]
Part 1: Expose some existing APIs to callers outside of NSS

Review of attachment 729323 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/certdb/cert.h
@@ +1529,5 @@
> +CERT_CheckCRLAssumingValidIssuer(const CERTCertificate * cert,
> +                                 CERTCertificate * issuer, const SECItem * dp,
> +                                 PRTime t, void * wincx,
> +                                 CERTRevocationStatus *revStatus,
> +                                 CERTCRLEntryReasonCode *revReason);

1) I'm generally uncomfortable with the "Reserved for future use" - my experience with such fields is that they inevitably turn out to be not good enough when the future use arrives, and so some new API is introduced, and the existing API is left with this anachronistic "NULL"
2) The known bug highlighted by the XXX TODO seems odd when exposing as part of a public API.

Naming wise, while the "AssumingValid" is descriptive, I feel it's a weird way for a public API to try to express the contract - it feels a bit... waffley?

How do you feel about simply
CERT_CheckCRLForIssuer(...)
or even
CERT_CheckIssuerCRLCache(...)

It makes it clear
1) You're checking the cache
2) You're only checking the CRLs - not the actual validity of the cert<->issuer relationship.

WDYT?

::: lib/certdb/crl.c
@@ +2618,5 @@
> +
> +    if (!issuer) {
> +        PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
> +        return SECFailure;
> +    }

Can you please order these conditionals like they are in your new function?

Namely

if (!cert || !issuer) { ... }
if (revStatus) { ... }
if (revReason) { ... }
if (t && ...) { ... }

It's just a bit jarring to see the asymmetry here of the issuer check happening afterwards.

I can speculate that you did this to ensure that both revStatus and revReason are always set to negative values, even when SECFailure is returned, and if that's the case, then you should update CERT_CheckCRLAssumingValidIssuer to provide that same guarantee.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Ryan, thanks for the reviews. Based on your feedback, I am going to rethink my approach to the CRL functionality, and I will do that work in a separate bug.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/projects/nss/rev/15c1a03bca91