[squid-dev] [PATCH] Fetch missing certificates

[Amos Jeffries]

Audit of patch #2;

This is mostly polish, but there are a few logic issues still present.


in the patch description / commit message:

* s/independed /independent /

* s/from net/from the network/

* s/an Downloader class/class Downloader/


* in newely added or altered documentation, comments and debugs please
refer to "TLS" not "SSL".


in src/Downloader.cc:

* missing copyright

* in class DownloaderContext please use a empty line to separate the
methods and member variables.

* please use nullptr in new code instead of NULL
 - same applies in all the other changed parts too

* please do not use " X == NULL" or "X != NULL" in boolean conditions
anymore (that includes assert or Must conditions).
 - C++11 provides proper boolean operators. If this causes problems with
any object then that object requires the bool operator's to be defined.

* s/debugs(33 , /debugs(33, /

* the debugs lines indicating constructor and destructor have been run
need to be symmetrical and name the object type.

 - They are there for the find-alive.pl script, so need to match the
pattern it is searching for:
 eg. debugs(33, 6, "Downloader constructed, this=" << (void*)this);
 eg. debugs(33, 6, "Downloader destructed, this=" << (void*)this);

 - same for other new objects contrustors and destructors. IF you have
debugs() namign them.

* please do not add whitespace between function/name and first '(' in
any new code, it makes things much harder to find when not using an IDE.


* in Downloader::~Downloader

 - please reserve debugs level 2 for protocol activity.
  Since we are considering Downloader a type of high level protocol
actor its conceivable that we will want to use 11,2 for displaying its
HTTP-ish messages before the rest of Squid logic gets to mangle them.


* Downloader::buildRequest method

 - please use xstrdup() instead of strdup()

 - please use the SBuf url_ member variable in the debugs instead of the
raw-char* pointer.

 - no need for safe_free() of a local variable before a return. use
xfree() instead.

 - debugs "Invalid FTP URL" ? perhapse "Invalid URI" would be more
accurate here.


* Downloader::handleReply method

 - "TODO: remove the following check:"  ? its followed by a casting.

 - 'const bool failed' is used only the once. There's no point having it
when the flag assigned to it can be tested directly by the if-condition.


* Downloader::downloadFinished method

 - please remove the commented deleteThis and comment above it. They are
not necessary after the Must(done())
 - that comment brings up the question of whether that Must() should be
confirming doneAll() instead of done() ?


in src/Downloader.h

* missing copyright

* please include http/forward.h instead of defining "class HttpReply"

 - includes for base/AsyncCall.h and cbdata.h are also redundant and can
be removed

 - "/* AsyncJob API */" only needs to be put once.
   move methods up into the first ones list if necessary, and remove
whitespace lines between the API re-definitions.

 - the parameter names on handleReply are no necessary, the line can be
shortened by removing them.

 - our coding style places documentation of class 'private' methods in
the .cc file not the .h.

 - the symbol name 'callback' has a clash between the private method,
and the private member Pointer. This is a case where the '_' suffix is
required to distinguish them.
  Ironically you have _ on most other members already. :-P
 + since the private members mostly use '_' the 'SBuf object' one should
as well for consistency.


in src/HttpRequest.h:

* s/object intiated/which initiated/ and s/if exist/if any/


in src/Makefile.am:

* list the .cc before the .h on new entries please.


in src/base/AsyncJob.cc:

* s/debugs(93 , /debugs(93, /


in src/client_side_reply.cc:

* please remove the method name string
"clientReplyContext::sendMoreData:" from the touched debugs() lines.

* the Comm::IsConnOpen(conn->clientConnection) check can now be removed
from the TOS/NFMARK tests, since the new conn->isOpen() check now
handles that case.


in src/security/Handshake.cc:

* Security::HandshakeParser::ParseCertificate #else condition is empty
and should be removed.

* You should also be able to write its body simpler in C++11:

  +  typedef const unsigned char *x509Data;
  +  const x509Data x509Start =
reinterpret_cast<x509Data>(raw.rawContent());
  +  x509Data x509Pos = x509Start;

 becomes:

  +  auto x509Pos = reinterpret_cast<unsigned char *>(raw.rawContent());
  +  const auto x509Start = x509Pos;


in src/security/forward.h:

* please put CertList before CertRevokeList.
 I'm trying to keep these alphabetical for easier patch porting in future.


in src/ssl/PeerConnector.cc:

* Ssl::PeerConnector::certDownloadingDone

 - what does the downloadStatus status parameter mean? what values can
it have?

 - please pre-increment certsDownloads instead of post-increment.

 - why is DER format being used here? I think I know, but it needs to be
stated for clarity, Squid normally uses PEM format.

 - "check if __ has uri to download from" missing words, and s/uri/URI/

 - s/checkForMissingCertificates ()/checkForMissingCertificates()/


* in Ssl::PeerConnector::checkForMissingCertificates and
Ssl::PeerConnector::startCertDownloading

 - request->downloader is a CbcPointer<Dowloader> already. so get()
produces a Downloader raw-pointer.

which means instead of this mess:
  const Downloader *csd = dynamic_cast<const
Downloader*>(request->downloader.valid());

You should be able to do:
 const auto *csd = request->downloader.get();

Or better, use the CbcPointer instead of a raw-pointer. Most of the
things using csd dont need a raw-pointer at all. You can probably just use:
  const auto &csd = request->downloader;


in src/ssl/PeerConnector.h:

* since certsDownloads is apparently constrained to values up to
MaxCertsDownloads. Can we please use a small integer type like uint8_t
instead of a full 'int' ?


in src/ssl/bio.h:

* Please use doxygen \retval or \return instead of "Returns" on the new
gotHello*() methods

* serverCertificatesIfAny() is not documented


in src/ssl/support.cc:

* C++ casting please, not C-style.

* the new function Ssl::uriOfIssuerIfMissing contains a comment:
  // There is a URI where we can download a certificate.
  // Check to see if this is required.

... but it does not check anything, it just returns the value found.
  - I think that second line of comment needs to be dropped.


* in Ssl::missingChainCertificatesUrls since the for-loop is iterating
over all values each time. Use a C++11 for-each loop here
  for (const auto &i : serverCertificates) {
  }



in src/ssl/support.h:

* can we use Security::CertPointer for the second parameter of
CertsIndexedList ?

* dont use \ingroup for newly added items.
 - they should be in an appropriate namespace instead (Ssl:: for these).


* useSquidUntrusted() appears to be defined twice in a row.



Cheers
Amos