Hi,
On Sat, Mar 5, 2016 at 3:34 PM, Arne Schwabe <[email protected]> wrote:
> While crl files can change regulary and it is usually not a good idea to
> statically include them into config files, handling multiple files and
> updating files on mobile files is tiresome/problematic. Inlining a static
> version of the crl file is better in these use cases than to use no crl at
> all.
>
> OpenVPN 3 already supports inlining crl-verify, so <crl-verify> is already
> used in config files.
Feature-ACK, but some remarks:
> @@ -6498,7 +6498,7 @@ X509_1_C=KG
> .\"*********************************************************
> .SH INLINE FILE SUPPORT
> OpenVPN allows including files in the main configuration for the
> -.B \-\-ca, \-\-cert, \-\-dh, \-\-extra\-certs, \-\-key, \-\-pkcs12,
> \-\-secret
> +.B \-\-ca, \-\-cert, \-\-dh, \-\-extra\-certs, \-\-key, \-\-pkcs12,
> \-\-secret, \-\-crl-verify
> and
> .B \-\-tls\-auth
> options.
Real nitpicking, but now that I'm sending mail anyway: this exceeds
the 80-char 'limit', while the surrounding text seems to obey. Would
you mind putting .B \-\-crl-verify on a new line instead?
> @@ -2729,10 +2729,11 @@ options_postprocess_filechecks (struct options
> *options)
> "--pkcs12");
>
> if (options->ssl_flags & SSLF_CRL_VERIFY_DIR)
> - errs |= check_file_access_chroot (options->chroot_dir, CHKACC_FILE,
> options->crl_file, R_OK|X_OK,
> + errs |= check_file_access_chroot (options->chroot_dir, CHKACC_FILE,
> + options->crl_file, R_OK|X_OK,
> "--crl-verify directory");
This looks like an accidental formatting change.
> else
> - errs |= check_file_access_chroot (options->chroot_dir, CHKACC_FILE,
> options->crl_file, R_OK,
> + errs |= check_file_access_chroot (options->chroot_dir,
> CHKACC_FILE|CHKACC_INLINE, options->crl_file, R_OK,
> "--crl-verify");
This line too becomes quite long (it at least exceeds my default
editor window width ;) ).
> --- a/src/openvpn/ssl_verify_backend.h
> +++ b/src/openvpn/ssl_verify_backend.h
> @@ -254,7 +254,7 @@ result_t x509_write_pem(FILE *peercert_file,
> openvpn_x509_cert_t *peercert);
> * certificate or does not contain an entry for it.
> * \c FAILURE otherwise.
> */
> -result_t x509_verify_crl(const char *crl_file, openvpn_x509_cert_t *cert,
> - const char *subject);
> +result_t x509_verify_crl(const char *crl_file, const char *crl_inline,
> + openvpn_x509_cert_t *cert, const char *subject);
Could you add the new parameter to the doxygen too?
> --- a/src/openvpn/ssl_verify_openssl.c
> +++ b/src/openvpn/ssl_verify_openssl.c
> @@ -578,7 +578,7 @@ x509_write_pem(FILE *peercert_file, X509 *peercert)
> * check peer cert against CRL
> */
> result_t
> -x509_verify_crl(const char *crl_file, X509 *peer_cert, const char *subject)
> +x509_verify_crl(const char *crl_file, const char* crl_inline, X509
> *peer_cert, const char *subject)
This becomes a quite long line.
> --- a/src/openvpn/ssl_verify_polarssl.c
> +++ b/src/openvpn/ssl_verify_polarssl.c
> @@ -359,18 +359,29 @@ x509_write_pem(FILE *peercert_file, x509_crt *peercert)
> * check peer cert against CRL
> */
> result_t
> -x509_verify_crl(const char *crl_file, x509_crt *cert, const char *subject)
> +x509_verify_crl(const char *crl_file, const char* crl_inline, x509_crt
> *cert, const char *subject)
This becomes a quite long line.
> {
> result_t retval = FAILURE;
> x509_crl crl = {0};
> struct gc_arena gc = gc_new();
> char *serial;
>
> - if (!polar_ok(x509_crl_parse_file(&crl, crl_file)))
> + if (!strcmp (cert_file, INLINE_FILE_TAG) && crl_inline)
This doesn't compile. I think you meant to write crl_file there, not cert_file.
> {
> - msg (M_WARN, "CRL: cannot read CRL from file %s", crl_file);
> - goto end;
> + if (!polar_ok(x509_crl_parse_(&crl, crl_inline, strlen(crl_inline))))
This doesn't compile. The final _ of x509_crl_parse_() should be removed.
> + {
> + msg (M_WARN, "CRL: cannot read CRL from file %s", crl_file);
> + goto end;
> + }
This warning message does not make sense for an inline crl. Change to
"cannot parse inline CRL"?
After the above changes, I tested the code and it works as expected.
-Steffan
