Re: [Bug-wget] [PATCH] Trust on first use

[Giuseppe Scrivano]

Thanks for your contribution!

I've some comments on the code:

Molnár Géza <address@hidden> writes:

> diff --git a/src/gnutls.c b/src/gnutls.c
> index 5a89e06..38bf2af 100644
> --- a/src/gnutls.c
> +++ b/src/gnutls.c
> @@ -76,6 +76,85 @@ key_type_to_gnutls_type (enum keyfile_type type)
>     preprocessor macro.  */
>  
>  static gnutls_certificate_credentials_t credentials;
> +
> +int
> +save_trusted_certificate(gnutls_x509_crt_t* cert, const char* host){

please leave a space between the function name and '(', same applies to
the code below.


> +  FILE* of;
> +  int result = -1;
> +  char* outputfile = aprintf("%s.crt",host); // for now

please not use C++ comments, use the /* C style.  */ (and drop the
comment above completely).  Also, put the * near the variable name, so
it will be "char *outputfile", should be changed in the code below too,
where it happens.

> +  
> +  if(!file_exists_p(outputfile))
> +    {
> +      size_t bufferSize = 4096;
> +      char *cert_data = (char*) malloc(bufferSize*sizeof(char));
> +      if(!cert_data) return -1; // Could not allocate memory, let's bail 
> out. Maybe an error message here? 

just use xmalloc, it will abort if the memory cannot be allocated.


> +      result = gnutls_x509_crt_export (*cert, GNUTLS_X509_FMT_PEM, 
> cert_data, &bufferSize);

I think you should retry with a bigger buffer (use xrealloc) on a
GNUTLS_E_SHORT_MEMORY_BUFFER error, the needed size will be in
bufferSize.



> diff --git a/src/openssl.c b/src/openssl.c
> index b8a9614..07d7cc4 100644
> --- a/src/openssl.c
> +++ b/src/openssl.c
> @@ -270,6 +270,7 @@ ssl_init (void)
>  
>    SSL_CTX_set_default_verify_paths (ssl_ctx);
>    SSL_CTX_load_verify_locations (ssl_ctx, opt.ca_cert, opt.ca_directory);
> +  if(opt.trust_model != built_in_only) SSL_CTX_load_verify_locations 
> (ssl_ctx, opt.ca_cert, ".");

move the if body to a new line.

>    if (opt.crl_file)
>      {
> @@ -631,6 +632,37 @@ static char *_get_rfc2253_formatted (X509_NAME *name)
>    return out ? out : xstrdup("");
>  }
>  
> +int
> +save_trusted_certificate(X509 *cert, const char* host){
> +  FILE* of;
> +  int result = 0;
> +  char *signer_hash = aprintf("%x",X509_subject_name_hash(cert));
> +  char *outputfile = NULL;
> +  int n = -1;
> +  
> +  do
> +  {
> +    free(outputfile);
> +    ++n;
> +    outputfile = aprintf("%s.%d",signer_hash,n);
> +  }

space after commas.



> +  while(file_exists_p(outputfile));
> +
> +  of = fopen(outputfile,"wb");
> +  if(of)
> +    {
> +      result = PEM_write_X509(of,cert);
> +      fclose(of);
> +    }
> +    
> +  if(!result)
> +    logprintf (LOG_NOTQUIET, _("ERROR: Could not save certificate to file: 
> %s\n"), outputfile);
> +  
> +  free(outputfile);
> +  free(signer_hash);
> +  return result;  
> +}
> +
>  /* Verify the validity of the certificate presented by the server.
>     Also check that the "common name" of the server, as presented by
>     its certificate, corresponds to HOST.  (HOST typically comes from
> @@ -654,6 +686,7 @@ ssl_check_certificate (int fd, const char *host)
>    long vresult;
>    bool success = true;
>    bool alt_name_checked = false;
> +  bool could_save_crt = false;
>  
>    /* If the user has specified --no-check-cert, we still want to warn
>       him about problems with the server's certificate.  */
> @@ -705,6 +738,15 @@ ssl_check_certificate (int fd, const char *host)
>          case X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT:
>            logprintf (LOG_NOTQUIET,
>                       _("  Self-signed certificate encountered.\n"));
> +          if(opt.trust_model == trust_on_first_use)
> +            {
> +            logprintf (LOG_NOTQUIET, _("Saving certificate as requested.\n") 
> );
> +            could_save_crt = save_trusted_certificate(cert,host);
> +            }
> +          else
> +            if(opt.trust_model != built_in_only)
> +              logprintf (LOG_NOTQUIET, _("Use 
> --trust-mode=trust-on-first-use to save this certificate as trusted!\n"));
> +            
>            break;
>          case X509_V_ERR_CERT_NOT_YET_VALID:
>            logprintf (LOG_NOTQUIET, _("  Issued certificate not yet 
> valid.\n"));
> @@ -718,7 +760,7 @@ ssl_check_certificate (int fd, const char *host)
>            logprintf (LOG_NOTQUIET, "  %s\n",
>                       X509_verify_cert_error_string (vresult));
>          }
> -      success = false;
> +      success = could_save_crt;
>        /* Fall through, so that the user is warned about *all* issues
>           with the cert (important with --no-check-certificate.)  */
>      }
> diff --git a/src/options.h b/src/options.h
> index 5a1532c..9363c43 100644
> --- a/src/options.h
> +++ b/src/options.h
> @@ -277,6 +277,14 @@ struct options
>    char *encoding_remote;
>    char *locale;
>  
> +#ifdef HAVE_SSL
> +  enum trust_types{

space here before '{'


If nobody has complains about this change, we should add some
documentation, (possibly a test?), and rewrite the commit message to
list the changes using a ChangeLog style.

Giuseppe