On Mon, Oct 01, 2018 at 01:12:54PM -0500, Jared Bents wrote:
> This patch has been added new flag 'crl_reload_interval' to reload crl. This can
> be used to reload ca_cert file on every new tls session if
> difference between last reload and current reload time(seconds) greater-than
> crl_reload_interval.
>
> Note: If interval time > 0 and < 300 then crl_reload_interval will reset to
> 300 seconds. For this support 'check_crl' should be 1 or 2.
> Here, We have selected 300 seconds as a reset value to reduce overhead of crl
> reload funtionality on new session. We can choose less than 300 seconds but it
> will increase overhead of crl reload on new session.
This sounds like a reasonable thing to do, but couple of comments on the
implementation details:
> diff --git a/src/crypto/tls.h b/src/crypto/tls.h
> #ifndef TLS_H
> #define TLS_H
> +#include <openssl/x509v3.h>
This tls.h file needs to be independent of which crypto/TLS library is
used, so it cannot pull in OpenSSL header files.
> +X509_STORE *tls_crl_cert_reload(const char *ca_cert, int check_crl);
Or define something to return an OpenSSL specific data structure.
That said, it does not look like either of these changes are really
needed in tls.h since that tls_crl_cert_reload() is not called from
anywhere outside tls_openssl.c. In other words, that function should be
marked static in tls_openssl.c and not declared in tls.h.
> diff --git a/src/crypto/tls_openssl.c b/src/crypto/tls_openssl.c
> +#include <pthread.h>
Could you please clarify why this would need pthread mutex support? What
is that trying to protect against? hostapd is a single-threaded
process, so it is not immediately obvious what this is trying to do.
Either remove this unnecessary mutex or document why it is needed.
> @@ -189,6 +190,8 @@ struct tls_context {
> int cert_in_cb;
> char *ocsp_stapling_response;
> int check_crl_strict;
> + int check_crl;
> + const char *ca_cert;
> };
These should more likely be added into struct tls_data which is also
where I moved the check_crl_strict in an earlier patch.
> @@ -968,8 +975,19 @@ void * tls_init(const struct tls_config *conf)
> + data->crl_reload_interval = conf->crl_reload_interval;
> + /* Set crl_reload_interval is equal to 5 minutes to reduce overhead of
> + * crl reload funtionality on new session. We can choose less than 5
> + * minutes but it will increase overhead of crl reload on new session.
> + * */
> + if(data->crl_reload_interval > 0 && data->crl_reload_interval < 300) {
> + wpa_printf(MSG_INFO,
> + "crl reload interval is set too low, reset it to 300");
> + data->crl_reload_interval = 300;
> + }
> + }
Is there a strong need to enforce this 300 second? That makes it
inconvenient to test this functionality since any automated test case
would need to last at least five minutes to hit this limit.
> @@ -1333,8 +1355,39 @@ struct tls_connection * tls_connection_init(void *ssl_ctx)
> + time_t now;
> + /* Get current time */
> + now = time(NULL);
Please use os_get_reltime() instead.
> + /* Replace X509 store if it is time to update crl */
> + /* Replace X509 store if difference between current time and previous store
> + * reload time greater-than crl_reload_interval */
> + if (data->crl_reload_interval > 0 && data->crl_last_reload +
> + data->crl_reload_interval <= now) {
So os_reltime_*() helpers here, e.g., with os_reltime_expired().
> + pthread_mutex_lock(&data->mutex);
> + /* recheck data->crl_last_reload because it may be inaccurate
> + * without mutex */
Why? What could have modified it without mutex in a single-threaded
process?
> +X509_STORE *tls_crl_cert_reload(const char *ca_cert, int check_crl)
This is the function that should be static within tls_openssl.c.
--
Jouni Malinen PGP id EFC895FA
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
