GnuTLS: load and send X.509 certificate chains

[yadij]

Feature parity with OpenSSL in terms of loading and validating a chain
of X.509 intermediary certificates from a PEM file during configuration,
and delivery of the resulting chain on TLS ServerHello handshakes.

[yadij]

Replacement PR for #458

[rousskov]

@yadij, the PR needs more work, but I think it is time to give you a chance to review recent changes and adjust what you think should be adjusted (inside and outside those changes). Please ping me when you are happy with the branch code, and I will re-review.

[yadij]

Putting aside my dislike of the extra CPU cycles this API design creates from all the de-ref/re-ref taking I can accept your changes.

@rousskov This last push is simply a rebase to fix the code collision and github editors empty line removal. Over to you for final review.

[rousskov]

Putting aside my dislike of the extra CPU cycles this API design creates from all the de-ref/re-ref taking

What makes you think that these reference and dereference operations spend extra CPU cycles? At that low level, both pointers and references are just memory addresses to the compiler. The low-level CPU instructions are exactly the same, regardless of whether the memory is accessed via a raw pointer or a reference.

When used correctly, these never-nil and always-immutable pointers (called references) can make code safer, clearer, and faster. They do not cost anything extra in terms of CPU cycles.

[rousskov]

Alex: the PR needs more work, but I think it is time to give you a chance to review recent changes and adjust what you think should be adjusted (inside and outside those changes). Please ping me when you are happy with the branch code, and I will re-review.

Amos: Over to you for final review.

This code is buggy and still needs more polishing work AFAICT. I left a few specific change requests, but this is not a comprehensive review -- I ignored most polishing issues (among others). Writing code by review is inefficient and unpleasant. Please let me know if you prefer that I finish this development work instead. I know others are waiting for this PR to land.

[rousskov]

/// Squid-defined error code (<0), an error code returned by X.509 API, or zero
typedef int ErrorCode;

Using Security::ErrorCode type here conflicts with that type description (e.g., GNUTLS_E_FILE_ERROR returned by gnutls_load_file() is negative). It also mismatches the otherwise identical code in KeyData::loadX509CertFromFile(). I hope the type description and loadX509CertFromFile() are OK, and this (poorly duplicated) code is wrong.

Suggested change

	Security::ErrorCode x = gnutls_load_file(certFilename, &rawFileContent);
	Security::LibErrorCode x = gnutls_load_file(certFilename, &rawFileContent);

[rousskov]

Please refactor to use std::vector instead of raw new/delete. Use std::vector::data() to pass the raw array to gnutls_certificate_set_x509_key(). I expect certCount to be gone after this refactoring.

[yadij]

This does not seem right to me. It assumes that std::vector is always a single consecutive memory allocation - AFAIK that is not a guarantee from std::vector, just a common compiler choice.

std::array does document that guarantee officially, so I will look into switching to that.

[yadij]

This file changes result from incremental review polishing. The original change around these was split off into an already merged PR without this pointer-safety action.
TODO: split this off also.