Re: [Patch] Automatically trust valid certificates on windows via CryptoApi

On 3/12/07, Bert Huijben <bert@vmoo.com> wrote:
> > -----Original Message-----
> > From: Ivan Zhakov [mailto:chemodax@gmail.com]
> > Sent: zondag 11 maart 2007 10:18
> > To: Bert Huijben
> > Cc: dev@subversion.tigris.org
> > Subject: Re: [Patch] Automatically trust valid certificates on windows via
> CryptoApi
> >
> > Hi Bert,
> >
> > I like idea of your patch and patch itself. I've not tested and
> > carefully reviewed it yet. I'll do it ASAP.
> >
> > Would you also please provide the change log message for this commit, as
> > described in HACKING?
>
> Hi Ivan,
>
> Thanks for looking into it.
>
> I hope the following log message matches the guidelines:
Hi Bert, thank you for log message. Your patch looks very good, it
just needs several improvements.

First I think that svn_auth_get_windows_ssl_server_trust_provider
should go to subversion/libsvn_subr/ssl_server_trust_providers.c
instead of simple_providers.c

Also please find several comments/questions inside the patch:

Index: subversion/include/svn_auth.h
===================================================================
--- subversion/include/svn_auth.h (revision 23794)
+++ subversion/include/svn_auth.h (working copy)
@@ -730,6 +730,25 @@
    apr_pool_t *pool);

+#if (defined(WIN32) && !defined(__MINGW32__)) || defined(DOXYGEN)
+/**
+ * Create and return @a *provider, an authentication provider of type @c
+ * svn_auth_cred_ssl_server_trust_t, allocated in @a pool.
+ *
+ * This provider automatically validates ssl server certificates with
+ * the CryptoApi, like Internet Explorer and the Windows network API do.
+ * This allows the rollout of root certificates via Windows Domain
+ * policies, instead of subversion specific configuration.
+ *
+ * @since New in 1.5.
+ * @note This function is only available on Windows.
+ */
+void
+svn_auth_get_windows_ssl_server_trust_provider
+ (svn_auth_provider_object_t **provider,
+ apr_pool_t *pool);
+#endif /* WIN32 || DOXYGEN */
+
 /** Create and return @a *provider, an authentication provider of type @c
  * svn_auth_cred_ssl_client_cert_t, allocated in @a pool.
  *
Index: subversion/libsvn_subr/cmdline.c
===================================================================
--- subversion/libsvn_subr/cmdline.c (revision 23794)
+++ subversion/libsvn_subr/cmdline.c (working copy)
@@ -376,6 +376,10 @@
   APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;

   /* The server-cert, client-cert, and client-cert-password providers. */
+#if defined(WIN32) && !defined(__MINGW32__)
+ svn_auth_get_windows_ssl_server_trust_provider(&provider, pool);
+ APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
+#endif
   svn_auth_get_ssl_server_trust_file_provider(&provider, pool);
   APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
   svn_auth_get_ssl_client_cert_file_provider(&provider, pool);
Index: subversion/libsvn_subr/simple_providers.c
===================================================================
--- subversion/libsvn_subr/simple_providers.c (revision 23794)
+++ subversion/libsvn_subr/simple_providers.c (working copy)
@@ -720,6 +720,181 @@
   *provider = po;
 }

+
+/* retieve ssl server CA failure overrides (if any) from CryptoApi */
Small typo: "Retrieve" and dot at the and of comment.

+static svn_error_t *
+windows_ssl_server_trust_first_credentials(void **credentials,
+ void **iter_baton,
+ void *provider_baton,
+ apr_hash_t *parameters,
+ const char *realmstring,
+ apr_pool_t *pool)
+{
+ typedef PCCERT_CONTEXT (WINAPI *createcertcontext_fn_t)(
+ DWORD dwCertEncodingType,
+ const BYTE *pbCertEncoded,
+ DWORD cbCertEncoded);
+
+ typedef BOOL (WINAPI *getcertchain_fn_t)(
+ HCERTCHAINENGINE hChainEngine,
+ PCCERT_CONTEXT pCertContext,
+ LPFILETIME pTime,
+ HCERTSTORE hAdditionalStore,
+ PCERT_CHAIN_PARA pChainPara,
+ DWORD dwFlags,
+ LPVOID pvReserved,
+ PCCERT_CHAIN_CONTEXT* ppChainContext);
+
+ typedef VOID (WINAPI *freecertchain_fn_t)(
+ PCCERT_CHAIN_CONTEXT pChainContext);
+
+ typedef BOOL (WINAPI *freecertcontext_fn_t)(
+ PCCERT_CONTEXT pCertContext);
+
+ HINSTANCE cryptodll = NULL;
+ createcertcontext_fn_t createcertcontext;
+ getcertchain_fn_t getcertchain;
+ freecertchain_fn_t freecertchain;
+ freecertcontext_fn_t freecertcontext;
+ int certlen = 0;
+ char *binarycertificate;
+ PCCERT_CONTEXT context = NULL;
+ CERT_CHAIN_PARA chainpara;
+ PCCERT_CHAIN_CONTEXT chaincontext = NULL;
In Subversion we separate words in variable names using underscore
symbol. So better name cert_len, binary_certificate, chain_context,
and chain_para for consistency.

+ svn_boolean_t ok = TRUE;
+
+ apr_uint32_t *failures = apr_hash_get(parameters,
+ SVN_AUTH_PARAM_SSL_SERVER_FAILURES,
+ APR_HASH_KEY_STRING);
+ const svn_auth_ssl_server_cert_info_t *cert_info =
+ apr_hash_get(parameters,
+ SVN_AUTH_PARAM_SSL_SERVER_CERT_INFO,
+ APR_HASH_KEY_STRING);
+
+ if (0 != (*failures & ~SVN_AUTH_SSL_UNKNOWNCA))
+ {
+ /* give up, go on to next provider; the only thing we can accept
+ is an unknown certificate authority */
+
+ *credentials = NULL;
+ return SVN_NO_ERROR;
+ }
+
+ /* In case anyone wonders why we use LoadLibraryA here: This will
+ always work on Win9x/Me, whilst LoadLibraryW may not. */
+ cryptodll = LoadLibraryA("Crypt32.dll");
+
+ if (!cryptodll || !cert_info->ascii_cert)
Why you checking that ascii_cert is non-null? Is it possible situation?

+ {
+ /* give up, go on to next provider. */
+ *credentials = NULL;
+ return SVN_NO_ERROR;
+ }
+
+ createcertcontext =
+ (createcertcontext_fn_t)GetProcAddress(cryptodll,
+ "CertCreateCertificateContext");
+ getcertchain =
+ (getcertchain_fn_t)GetProcAddress(cryptodll, "CertGetCertificateChain");
+ freecertchain =
+ (freecertchain_fn_t)GetProcAddress(cryptodll, "CertFreeCertificateChain");
+ freecertcontext =
+ (freecertcontext_fn_t)GetProcAddress(cryptodll,
+ "CertFreeCertificateContext");
+
+ if (!createcertcontext || !getcertchain || !freecertchain ||
+ !freecertcontext)
+ ok = FALSE;
+
+ /* Use apr-util as CryptStringToBinaryA is available only on XP+ */
+ certlen = apr_base64_decode_len(cert_info->ascii_cert);
+
+ if (certlen <= 0)
+ ok = FALSE;
+ else
+ {
+ binarycertificate = apr_palloc(pool, certlen);
+ certlen = apr_base64_decode(binarycertificate, cert_info->ascii_cert);
+ }
It doesn't make sense to check result of apr_base64_decode_len() so,
this could simplified to just:
binary_certificate = apr_palloc(pool,
apr_base64_decode_len(cert_info->ascii_cert));
cert_len = apr_base64_decode(binarycertificate, cert_info->ascii_cert);

+
+ if (ok)
+ {
+ /* Parse the certificate into a context */
+ context = (*createcertcontext)(X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,
+ binarycertificate, certlen);
To call function by pointer you shouldn't use "*". Just use
"create_cert_context(...);"

+
+ if (!context)
+ ok = FALSE; /* Windows does not think the certificate is valid */
+ }
+
+ if (ok)
+ {
+ /* Retrieve the certificate chain of the certificate
+ (a certificate without a valid root does not have a chain */
+ memset(&chainpara, 0, sizeof(chainpara));
+ chainpara.cbSize = sizeof(chainpara);
+
+ if ((*getcertchain)(NULL, context, NULL, context->hCertStore,
+ &chainpara, 0, NULL, &chaincontext))
Ditto.

+ {
+ if (0 != (chaincontext->rgpChain[0]->TrustStatus.dwErrorStatus &
+ ~(CERT_TRUST_IS_NOT_TIME_NESTED |
+ CERT_TRUST_REVOCATION_STATUS_UNKNOWN)))
+ {
+ /* The certificate is not 100% valid, just fall back to the
+ subversion certificate handling */
+
+ ok = FALSE;
+ }
+ }
+ else
+ ok = FALSE;
+ }
+
+ if (chaincontext)
+ (*freecertchain)(chaincontext);
Ditto.

+ if (context)
+ (*freecertcontext)(context);
Ditto.

+ if (cryptodll)
+ FreeLibrary(cryptodll);
+
+ if (!ok)
+ {
+ /* go on to next provider. */
+ *credentials = NULL;
+ return SVN_NO_ERROR;
+ }
+ else
+ {
+ svn_auth_cred_ssl_server_trust_t *creds =
+ apr_pcalloc(pool, sizeof(*creds));
+ creds->may_save = FALSE; /* No need to save it */
+ *credentials = creds;
+ }
+
+ return SVN_NO_ERROR;
+}
+
+
+static const svn_auth_provider_t windows_server_trust_provider = {
+ SVN_AUTH_CRED_SSL_SERVER_TRUST,
+ windows_ssl_server_trust_first_credentials,
+ NULL,
+ NULL,
+};
+
+/* Public API */
+void
+svn_auth_get_windows_ssl_server_trust_provider(svn_auth_provider_object_t
**provider,
+ apr_pool_t *pool)
+{
+ svn_auth_provider_object_t *po = apr_pcalloc(pool, sizeof(*po));
+
+ po->vtable = &windows_server_trust_provider;
+ *provider = po;
+}
+
+
 #endif /* WIN32 */

-- 
Ivan Zhakov
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org