On Mon, Oct 01, 2018 at 01:12:55PM -0500, Jared Bents wrote:
> This patch added 'check_cert_subject' support to match the value of
> every field against the DN of the subject in the client certificate. If
> the values do not match, the certificate verification will fail and will reject
> the user.
>
> This option allows hostapd to match every individual field in the right order,
> also allow '*' character as a wildcard (e.g OU=Development*).
>
> Note: Hostapd will match string up to 'wildcard' against the DN of the subject
> in the client certificate for every individual field.
This needs to be rebased on top of patch 2/3 (and earlier modified
version of 1/3). Additional comments on the implementation below:
> diff --git a/src/crypto/tls.h b/src/crypto/tls.h
> -int __must_check tls_global_set_verify(void *tls_ctx, int check_crl, int strict);
> +int __must_check tls_global_set_verify(void *tls_ctx, int check_crl, int strict, char *check_cert_subject);
This would break the build with all other TLS libraries, i.e., all
src/crypto/tls_*.c with this function need to be updated and the other
ones should likely reject check_cert_subject != NULL to avoid unexpected
behavior.
Should that check_cert_subject be marked const?
> diff --git a/src/crypto/tls_openssl.c b/src/crypto/tls_openssl.c
> @@ -182,6 +182,7 @@ static int tls_add_ca_from_keystore_encoded(X509_STORE *ctx,
>
> static int tls_openssl_ref_count = 0;
> static int tls_ex_idx_session = -1;
> +struct tls_dn_field_order_cnt dn_cnt = {0};
Please don't use a global variable. Shouldn't that dn_cnt be within
struct tls_data or struct tls_connection?
> @@ -192,6 +193,7 @@ struct tls_context {
> int check_crl_strict;
> int check_crl;
> const char *ca_cert;
> + char *check_cert_subject;
That should likely be within struct tls_data.
> @@ -1359,6 +1366,11 @@ struct tls_connection * tls_connection_init(void *ssl_ctx)
> + if (data->client_cert_subject)
> + tls_global->check_cert_subject = data->client_cert_subject;
> + else
> + tls_global->check_cert_subject = NULL;
That if statement does not seem to make much sense.. Why not just
assigning the variable as is with the second line there since it is
going to handle the NULL case as well?
> +static int client_cert_fingerprint(X509* cert, enum digest_alg alg)
> + switch (alg)
> + {
> + case DIGEST_HASH_ALG_MD5:
> + /* Get the digest */
> + ret = X509_digest(cert, EVP_md5(), fingerprint, &len);
> + break;
..
Could this use the existing enum crypto_hash_alg and crypto_hash_*()
functions instead of implementing same again with OpenSSL specific
functions here?
--
Jouni Malinen PGP id EFC895FA
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
