On 10/04/17 11:15, Steffan Karger wrote: > This allows the user to specify what certificate crypto algorithms to > support. The supported profiles are 'preferred' (default), 'legacy' and > 'suiteb', as discussed in <84590a17-1c48-9df2-c48e-4160750b2...@fox-it.com> > (https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14214.html). > > This only implements the feature for mbed TLS builds, because for mbed it > is both more easy to implement and the most relevant because mbed TLS 2+ > is by default somewhat restrictive by requiring 2048-bit+ for RSA keys. > > Signed-off-by: Steffan Karger <steffan.kar...@fox-it.com> > --- > src/openvpn/options.c | 8 ++++++++ > src/openvpn/options.h | 1 + > src/openvpn/ssl.c | 3 +++ > src/openvpn/ssl_backend.h | 10 ++++++++++ > src/openvpn/ssl_mbedtls.c | 51 > +++++++++++++++++++++++++++++++++++++++++++++++ > src/openvpn/ssl_mbedtls.h | 1 + > src/openvpn/ssl_openssl.c | 6 ++++++ > 7 files changed, 80 insertions(+) > [...snip...] > diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c > index ba8dadf..1fcee72 100644 > --- a/src/openvpn/ssl_mbedtls.c > +++ b/src/openvpn/ssl_mbedtls.c [...snip...] > void > +tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile) > +{ > + if (0 == strcmp(profile, "preferred"))
While you do say in the docs that profile MAY NOT be NULL, this will
cause a SEGV if it happens. Wouldn't it be better better to either
report an error or ASSERT() if profile == NULL?
> + {
> + ctx->cert_profile = openvpn_x509_crt_profile_preferred;
> + }
> + else if (0 == strcmp(profile, "legacy"))
> + {
> + ctx->cert_profile = openvpn_x509_crt_profile_legacy;
> + }
> + else if (0 == strcmp(profile, "suiteb"))
> + {
> + ctx->cert_profile = openvpn_x509_crt_profile_suiteb;
> + }
> + else
> + {
> + msg (M_FATAL, "ERROR: Invalid cert profile: %s", profile);
> + }
> +}
[...snip...]
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -384,6 +384,12 @@ tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const
> char *ciphers)
> }
>
> void
> +tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
> +{
> + msg (M_WARN, "WARNING: --tls-cert-profile not supported for OpenSSL");
> +}
> +
While I do understand the use of M_WARN here. Wouldn't it be better to
use M__NONFATAL and make it more a like an "INFO" log line instead.
--tls-cert-profile is a brand new feature, where certificate profiles
are not (yet?) implemented for OpenSSL. So that's strictly not a
warning of more critical aspects. But it is however important to
highlight it.
Otherwise, I think this makes sense. It would be good to have something
similar for OpenSSL too, to be feature complete against mbed TLS.
It seems usage() is lacking details about --tls-cert-profile for the
--help screen.
I have reviewed this by also comparing the implementation to what's done
in OpenVPN 3 [1]. The profile declaration and enabling it seems to be
very much aligned. (on a side note, I have already added a default
profile for "legacy" in Fedora, as many users complained about OpenVPN
breaking on Fedora 26 and newer, which also looks fairly recognisable to
what is done here).
I will run some real tests tomorrow. But quick smoke testing looks good.
--
kind regards,
David Sommerseth
OpenVPN Technologies, Inc
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
