968451 - Document the exported functions exposed from mozilla::pkix (pkix/pkix.h)

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P4)

Description

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

This is a follow-up based on David's request in bug 921891.

Comment 1

[J.C. Jones [:jcj] (he/they)]

Attachment: bug_968451: Document the methods and results of pkix.h

I added documentation, per this bug, in the larger effort of preparing mozilla::pkix for NSS. I believe all this documentation to be correct and exhaustive.

Comment 2

[J.C. Jones [:jcj] (he/they)]

Attachment: bug_968451: Document the methods and results of pkix.h ... better!

This patch adjusts some formatting and text to be more clear and useful in the documentation.

Comment 3

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

Comment on attachment 8535687 [details] [diff] [review]
bug_968451: Document the methods and results of pkix.h ... better!

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

::: security/pkix/include/pkix/pkix.h
@@ +175,5 @@
> +//         too long.
> +// Result::FATAL_ERROR_INVALID_STATE
> +//         means that the TrustDomain provided an unexpected response.
> +// Result::FATAL_ERROR_NO_MEMORY
> +//         means a memory allocation failed, prohibiting validation.

1. This is not the best place to put this documentation on errors codes. Instead, if we're going to have such documentation at all, it should be in Result.h where the error codes are defined.

2. In most cases, the error codes are self-explanatory. We should (a) let the self-explanatory error codes speak for themselves, (b) rename the less clear error codes to be self-explanatory, and (c) scope the documentation to explaining the non-self-evident aspects of the error codes. For example, I think spending time documenting the difference between UNKNOWN_ISSUER vs. UNTRUSTED_ISSUER is very important (especially because it differs from NSS). Similarly, saying EXPIRED_CERTIFICATE means the certificate is expired doesn't help at all, but explaining the difference between EXPIRED_CERTICIATE and EXPIRED_ISSUER_CERTIFICATE is very important.

@@ +185,5 @@
> +// In the event the certificate is trustworthy, the method will return Success.
> +//
> +// This function attempts to find a trustworthy path from the supplied
> +// certificate to a trust anchor. In the event that no trusted path is found,
> +// the method returns an error result; the error ranking is described above.

This is redundant with the previous paragraph.

@@ +190,5 @@
> +// Parameters:
> +//  trustDomain:
> +//         Interface to the necessary PKI functions.
> +//  cert:
> +//         Certificate to test

Be consistent with periods.

@@ +194,5 @@
> +//         Certificate to test
> +//  time:
> +//         Timestamp for which the chain should be valid; this is useful to
> +//         analyze whether a record was trustworthy when it was made.
> +//  endEntityOrCa:

capitalization.

@@ +213,5 @@
> +//         This is the policy to apply; typically included in EV certificates.
> +//         If there is no policy, pass in CertPolicyId::anyPolicy.
> +//  stapledOCSPResponse:
> +//         The optional OCSP stapled response, if provided, will be provided to
> +//         the TrustDomain's CheckRevocation method.

Again, I question the value of this type of documentation. Instead of stating the obvious, let's rename the parameters to be clearer and state only the non-obvious.

@@ +224,5 @@
>  
> +// Verify that the given end-entity cert, which is assumed to have been already
> +// validated with BuildCertChain, is valid for the given hostname. The matching
> +// function prefers to match against the subjectAltName extension, falling back
> +// to the Common Name if SAN doesn't match.

Rather than saying "The matching function prefers to match..." I'd say that the function attempts to implement RFC6125, though there are some differences. Then explain the differences from RFC6125.

@@ +228,5 @@
> +// to the Common Name if SAN doesn't match.
> +// The hostname parameter is the trusted "reference ID" that we are matching the
> +// supplied certificate against. The format of hostname is assumed to be a
> +// string representation of an IPv4 address, an IPv6 addresss, or a normalized
> +// ASCII DNS name that uses punycode when needed.

It would help to explain what "normalized" means here. Also, it would help to note that the IPv4 and IPv6 syntaxes are restricted to some subset that might be considered "normalized".

@@ +233,3 @@
>  Result CheckCertHostname(Input cert, Input hostname);
>  
> +// Construct an RFC 6960-encoded OCSP request for the provided certId. The

capitalization: certId.
hyphenation: "RFC-6960-encoded".

I don't think it is necessary to say the request has no extensions.

Note the intent of this function is to construct an OCSP request for the client to send to an OCSP responder.

Comment 4

[J.C. Jones [:jcj] (he/they)]

Attachment: bug_968451

Brian, Keeler: thanks both of you for your feedback. I tried to walk a better line in this patch.

I culled the most obvious result codes from the list, and moved them into Result.h as you recommended. 

The style nits are resolved (though I guess by default I use the same capitalization as the variable names in comments, sorry about that).

On the assumption that the variable names are generally clear, I didn't modify any of them. 

Regarding the CheckCertHostname method and its RFC compliance: I noticed two deltas from the RFC and included them in the comment. If there are others, I am happy to include but I could use a point in the right direction.

Thanks!

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

JC - I've had a lot of other reviews to get through. I should be able to address this tomorrow.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8536859 [details] [diff] [review]
bug_968451

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

LGTM with comments addressed.

::: security/pkix/include/pkix/Result.h
@@ +60,5 @@
> +//         by the issuer.
> +// Result::ERROR_BAD_DER
> +//         means the input was improperly encoded.
> +// Result::ERROR_UNKNOWN_ERROR
> +//         means that the library provided an error we didn't anticipate. See

Maybe "an external library" (e.g. NSS)

@@ +66,5 @@
> +// Result::FATAL_ERROR_LIBRARY_FAILURE
> +//         is an unexpected fatal error indicating a library had an unexpected
> +//         failure, and we can't proceed.
> +// Result::FATAL_ERROR_INVALID_ARGS
> +//         means that the certificate supplied resulted in a chain that was

This is more of "we violated our own expectations and there's a bug somewhere".

@@ +69,5 @@
> +// Result::FATAL_ERROR_INVALID_ARGS
> +//         means that the certificate supplied resulted in a chain that was
> +//         too long.
> +// Result::FATAL_ERROR_INVALID_STATE
> +//         means that the TrustDomain provided an unexpected response.

This can happen internally to mozilla::pkix as well. This is more of "we violated our own expectations and there's a bug somewhere" again.

::: security/pkix/include/pkix/pkix.h
@@ +80,5 @@
>  // path), more specific errors will be returned.
>  //
>  // ----------------------------------------------------------------------------
> +// Meanings of specific error codes can be found in Result.h
> +

nit: let's just have one blank line here, not two

@@ +89,3 @@
>  //
> +// Parameters:
> +//  Time:

The parameter names should match the function declaration, so these should all start with a lowercase letter.

@@ +91,5 @@
> +//  Time:
> +//         Timestamp for which the chain should be valid; this is useful to
> +//         analyze whether a record was trustworthy when it was made.
> +//  RequiredKeyUsageIfPresent:
> +//         What key usage bits must be set, if they are present at all, to be

if the extension is present, that is

@@ +96,5 @@
> +//         considered a valid chain. Multiple values should be OR'd together.
> +//         If you don't want to specify anything, use
> +//         KeyUsage::noParticularKeyUsageRequired.
> +//  RequiredEKUIfPresent:
> +//         What extended key usage bits must be set, if the EKU field exists, to

s/field/extension/

@@ +115,5 @@
> +// validated with BuildCertChain, is valid for the given hostname. The matching
> +// function attempts to implement RFC 6125 with a couple of differences:
> +// - IP addresses are out of scope of RFC 6125, but this method accepts them for
> +//   backward compatibility (see SearchNames in pkixnames.cpp)
> +// - A wildcard in a DNS-ID may only appear at the end of the label

Actually I think we recently changed this to be 'a wildcard may only appear as the entirety of the first label'.

Comment 7

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 968451 - Document the exported functions exposed from mozilla::pkix (pkix/pkix.h). r=keeler

Keeping the r+ from keeler.

This patch cleans up the remaining review comments. Keeler, for the two FATAL_ERROR results I chose these wordings; they are accurate in usage, but I'm just guessing on intent:

// Result::FATAL_ERROR_INVALID_ARGS
//         means that we violated our own expectations on inputs and there's a
//         bug somewhere.
// Result::FATAL_ERROR_INVALID_STATE
//         means that we violated our own expectations on state and there's a
//         bug somewhere.

At any rate, this patch only affects comments, and so I won't push a try run unless I hear that I should.

Thanks bsmith & keeler!

Comment 8

[J.C. Jones [:jcj] (he/they)]

Marking checkin-needed.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ead65d7b9e8

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ead65d7b9e8