[squid-dev] [PATCH] Fetch missing certificates

Fri Jul 15 14:08:01 UTC 2016

A new patch.

It also includes the following fixes:
   - Sets the log_uri for ClientHttpRequest build by Downloader
   - Removes two XXX comments from PeerConnector class, which are not 
valid any more
   - Make the Downloader::CbDialer a CallDialer kid.

Please also see my comments bellow.

On 07/14/2016 02:16 PM, Amos Jeffries wrote:
> 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/

ok

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

ok

>
> in src/Downloader.cc:
>
> * missing copyright

ok

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

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

ok

>
> * 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.

ok

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

ok

>
> * 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.

ok

>
> * 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.

ok

>
>
> * 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.

I added 1-2 debugs in Downloader::buildRequest() method.

>
>
> * Downloader::buildRequest method
>
>   - please use xstrdup() instead of strdup()

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

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

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

ok

>
>
> * Downloader::handleReply method
>
>   - "TODO: remove the following check:"  ? its followed by a casting.

The casting is used only for the check follows the casting.
I moved the TODO after 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.

In the future we may want other cases of failed. Maybe "!reply" or zero 
buffers size etc.
If no problem I prefer to leave it.

>
>
> * 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() ?

The done() is a more general test, include the case the Downloader 
aborted for a reason.

>
>
> in src/Downloader.h
>
> * missing copyright

ok

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

ok

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

>
>   - "/* 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.

ok

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

ok

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

ok, I fixed them, but we should change this style.
It is easier to have open the .h file and read the documentation for a 
method you are going to use, than searching in the cc file to find the 
documentation.

>
>   - 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

ok.

>   + since the private members mostly use '_' the 'SBuf object' one should
> as well for consistency.

ok

>
>
> in src/HttpRequest.h:
>
> * s/object intiated/which initiated/ and s/if exist/if any/
>

ok

>
> in src/Makefile.am:
>
> * list the .cc before the .h on new entries please.

ok

>
>
> in src/base/AsyncJob.cc:
>
> * s/debugs(93 , /debugs(93, /

ok

>
>
> in src/client_side_reply.cc:
>
> * please remove the method name string
> "clientReplyContext::sendMoreData:" from the touched debugs() lines.

ok

>
> * 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.

ok

>
>
> 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;

ok

>
>
> in src/security/forward.h:
>
> * please put CertList before CertRevokeList.
>   I'm trying to keep these alphabetical for easier patch porting in future.
>

ok

>
> in src/ssl/PeerConnector.cc:
>
> * Ssl::PeerConnector::certDownloadingDone
>
>   - what does the downloadStatus status parameter mean? what values can
> it have?

It is the HTTP status code.
This patch does not check for this.
If there are returned data and if they are a valid certificate, it 
consider the response valid.

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

ok

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

The DER format is used by RFCs for this and similar cases.

>
>   - "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;

OK for "get()" instead of valid() and static cast.

But I am not a fan of the "auto" type.
It is good to avoid searching/typing strange names like those we are 
using in iterators but my opinion is that we must not overuse it.
Else we would have to search for a class member declaration to find out 
the correct type of a parameter.

>
>
> 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' ?

  The MaxCertsDownloads normally will have a value of 10 or 15 or 20.
In practice we do not care about its size, it can be 8bit, 16bit, 32bit 
or 64bit integer. Also we do not care about the memory space it 
requires. In the cases where we do not care about the size of a 
parameter it is not bad idea to use the general data type.

>
>
> in src/ssl/bio.h:
>
> * Please use doxygen \retval or \return instead of "Returns" on the new
> gotHello*() methods
>
> * serverCertificatesIfAny() is not documented
>
ok

>
> in src/ssl/support.cc:
>
> * C++ casting please, not C-style.

ok,

>
> * 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.

I remove this comment.

>
> ... 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) {
>    }

ok

>
>
>
> in src/ssl/support.h:
>
> * can we use Security::CertPointer for the second parameter of
> CertsIndexedList ?

I am not seeing any reason.
In any case we can do it with an other patch if required.

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

ok

>
> * useSquidUntrusted() appears to be defined twice in a row.
>
>
>
> Cheers
> Amos
>
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: SQUID-112-fetch-certificates-t4.patch
Type: text/x-patch
Size: 73392 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20160715/c8aa5553/attachment-0001.bin>