certs review

David, here's a short review of all the cumulative changes you've made
to your issue-650-certs branch:

* svn_ra_dav_h

  In @brief, you say 'functions used by the server'. Huh? ra_dav is
  100% client-side stuff.

  Please document the public declarations of the provider constructors
  more fully. How does it work? What runtime hash params does it
  use? How will it behave when invoked via the public svn_auth.h API?
  Look at svn_wc.h:svn_wc_get_simple_provider for an example.

  (For example, the server-cert provider doesn't actually provide
  server certs, just a flag; the doc string needs to make it really
  clear exactly what it does and how it works.)

  Also, pleeeeeeze wrap to 80 columns. That's in the HACKING file. :-)

* svn_auth.h

  (svn_auth_cred_client_ssl_t): ah, nice, you actually *did* pass
  back paths to the cert and key files. very nice. even though you
  don't have direct access to the binary key blob, this is almost as
  good. much clearer! In the doc string, make it clear that the file
  fields are *paths* to files.

* session.c

  Ack! 80 column overruns throughout your changes! Ack! Fix all
  those!!

  (server_ssl_callback): remove the // C++ comment please. :-)

  (client_ssl_keypw_callback, client_ssl_callback): whoa, tex... you
  never check the error return value from
  svn_auth_first_credentials(). Why not just wrap the call in the
  SVN_ERR() macro? And in client_ssl_callback, you're dereffing
  credentials when it may be NULL! Also, these funcs could use short
  docstrings.

* config_file.c

  Your "ssl-ignore-*" examples are inverted from the doc strings.
  Look closely. :-)

+ "### ssl-ignore-unknown-ca Allow untrusted server certificates\n"
+ "### ssl-ignore-invalid-date Allow expired/postdated certificates\n"+ "### ssl-ignore-host-mismatch Allow certificates for other servers\n
+ "# ignore-ssl-unknown-ca = true\n"
+ "# ignore-ssl-host-mismatch = true\n"
+ "# ignore-ssl-invalid-date = true\n"

* This is great stuff, all in all. As you said, there are still a
  couple of pieces needed to make this cert-stuff feature complete:

    * a prompting provider for server-certs. presumably the specific
      error would arrive via auth_baton runtime hash, and the provider
      would ask the user if the particular error is ok to ignore. it
      also means that this provider could be loaded unconditionally,
      and server-certs could be validated even when no CAcerts.pem
      file is mentioned in the config file.

    * a prompting provider for client-certs. it can simply ask for
      the location of a key file.

  Both prompting providers would be registered *after* file providers,
  and make use of the prompt function within svn_client_ctx_t.

Keep up the great work! I feel a merge to trunk coming very soon.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Mar 10 23:50:31 2003