Adds ability to use x.509 certificates for authenticating users.

Review Request #938 — Created Aug. 2, 2009 and discarded


Review Board SVN (deprecated)


These changes enable Review Board to use x.509 certificates to be used alongside the builtin username/password authentication backend. This functionality will be useful for deployments in environments with a Public Key Infrastructure set up.

I deliberately avoided modifying post-review to support client certificates due to a lack of decent support for them in Python's high-level libraries. Instead, I modified the user preferences view to permit authenticated users to set their Django password. Django's built-in support for multiple authentication backends will then permit them to use the builtin auth backend with post-review.
Tested on my test Review Board server, running Apache/mod_python,mod_ssl/Django 1.1.
  1. Thanks for working on this! I have a laundry list of changes I'd like you to make, but most of them are style guideline stuff.
    1. No problem. It's something I've wanted to do the right way for a while. Thanks for the speedy feedback.
  2. /trunk/reviewboard/accounts/ (Diff revision 1)
    Should be in the list alphabetically.
  3. /trunk/reviewboard/accounts/ (Diff revision 1)
    I know the others don't do it, but this should inherit from object. The others all should too, eventually.
    Also, please add a doc block here describing the purpose and use of this backend.
  4. /trunk/reviewboard/accounts/ (Diff revision 1)
    This should make use of the siteconfig stuff. Only grab this value as needed.
  5. /trunk/reviewboard/accounts/ (Diff revision 1)
    Need to wrap this in a try/catch. If the user specifies an invalid regular expression, this will fail.
    1. Good call. Fixed it. What's the preferred way of getting feedback to the user on errors?
  6. /trunk/reviewboard/accounts/ (Diff revision 1)
    This should be prefixed with the backend name. Also, no "!" is needed.
  7. /trunk/reviewboard/accounts/ (Diff revision 1)
    No blank line here.
  8. /trunk/reviewboard/accounts/ (Diff revision 1)
    Should just do that settings lookup here.
  9. /trunk/reviewboard/accounts/ (Diff revision 1)
    Trailing whitespace.
  10. /trunk/reviewboard/accounts/ (Diff revision 1)
    Can you change this to "can_change_password"?
  11. /trunk/reviewboard/accounts/ (Diff revision 1)
    Can collapse this to:
    if can_change_password and form.cleaned_data['password1']:
  12. /trunk/reviewboard/admin/ (Diff revision 1)
    "Username Field"
  13. /trunk/reviewboard/admin/ (Diff revision 1)
    correspond *to*
  14. /trunk/reviewboard/admin/ (Diff revision 1)
    "extracted." should align with the beginning of the string on the previous line.
    Also, "will be extracted," rather than "is to be extracted."
  15. /trunk/reviewboard/admin/ (Diff revision 1)
    "Username Regex"
    Do we really need to expose this to the user? How often will they need to do something custom? If it's generally in e-mail form or something, I'd rather keep it simple.
    1. It is a bit ugly, but I couldn't think of any other way to get a 100% solution. Since the format of the certificate fields is completely up to the issuer, I don't think that taking care of the general case will be sufficient. In most cases, the email will be present in the certificate, although it isn't required. I wanted to make sure this backend would fit everybody's needs.
      Would it be acceptable to keep this setting, but not expose it in the admin UI? We could default to getting the username from the email address, or perhaps just matching on the email address, but leave the regular expression syntax in there for the outlier cases. This could still be set in the
  16. /trunk/reviewboard/admin/ (Diff revision 1)
  17. /trunk/reviewboard/admin/ (Diff revision 1)
  18. /trunk/reviewboard/admin/ (Diff revision 1)
    Remove "this" on "this Review Board.
  19. /trunk/reviewboard/admin/ (Diff revision 1)
    Blank line between these.
  20. /trunk/reviewboard/admin/ (Diff revision 1)
    Should subclass object.
  21. /trunk/reviewboard/admin/ (Diff revision 1)
    Also, I'd like to bail from this as early as possible. We should check the auth backend immediately and determine whether we should execute any of this code.
    1. Sounds good. Is SiteConfig.objects.get_current() the preferred way of doing this? It seems like hitting the database would be more expensive than a couple of dictionary lookups... I haven't fixed this yet, as I'm not sure what the best way to do it is.
  22. /trunk/reviewboard/admin/ (Diff revision 1)
    I'd prefer getattr(settings, "X509_USERNAME_FIELD", None), so that we don't break if this setting isn't at all in there.
    Also, this can be combined with the line below.
  23. Should be aligned at column 0.
  24. Same here.
  2. /trunk/reviewboard/admin/ (Diff revision 2)
    Just a minor remark, but I think the correct spelling is "X.509", not "x.509"
  2. /trunk/reviewboard/accounts/ (Diff revision 2)
    Parameters should be aligned.
  3. /trunk/reviewboard/accounts/ (Diff revision 2)
    Same here. No need for str(e), though, as the "%s" covers that.
  4. /trunk/reviewboard/accounts/ (Diff revision 2)
    Remove this blank line.
  5. /trunk/reviewboard/accounts/ (Diff revision 2)
    We should store form.cleaned_data['password1'] in a variable and then check that variable here and use it in the sha1 code below.
  6. /trunk/reviewboard/admin/ (Diff revision 2)
  7. /trunk/reviewboard/admin/ (Diff revision 2)
  8. /trunk/reviewboard/admin/ (Diff revision 2)
  9. /trunk/reviewboard/admin/ (Diff revision 2)
  10. /trunk/reviewboard/admin/ (Diff revision 2)
  11. /trunk/reviewboard/admin/ (Diff revision 2)
    str(None) isn't going to work so well...
    We should do:
    x509_settings_field = getattr(settings, "X509_USERNAME_FIELD")
    if x509_settings_field:
        x509_field = env.get(x509_settings_field)
Review request changed