New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

response.c: set SSL_CLIENT_VERIFY & SSL_CLIENT_S_DN #63

Closed

mackyle wants to merge 1 commit into lighttpd:master from mackyle:patch/extra-vars

Contributor

mackyle commented May 11, 2016

SSL_CLIENT_VERIFY is set to "NONE", "SUCCESS" or "FAILED:reason".
This is compatible with Apache's mod_ssl variable of the same name.

SSL_CLIENT_S_DN is set to the oneline version of the client certificate
subject's distinguished name and may be used as a setting for the
ssl.verifyclient.username config option. When Apache's mod_ssl is
configured to use 'FakeBasicAuth' it uses the SSL_CLIENT_S_DN value for
the username (that ultimately may end up in REMOTE_USER). The value
that will be set for SSL_CLIENT_S_DN may be determined using the
openssl x509 -noout -subject -in <cert.pem> command.

Signed-off-by: Kyle J. McKay

mackyle force-pushed the patch/extra-vars branch 2 times, most recently from 7027605 to 48a9262 Compare

May 11, 2016 07:31

Member

gstrauss commented May 16, 2016

x-ref: https://redmine.lighttpd.net/issues/2693

gstrauss reviewed

View reviewed changes

src/response.c

+              			}
+              			buffer_copy_buffer(ds->value, envds->value);
+              		}
+              		array_insert_unique(con->environment, (data_unset *)envds);

Member

gstrauss May 16, 2016

Is there any reason why this is not further up with the code which fills in envds?

Member

gstrauss commented May 16, 2016 •

edited

I added a couple comments inline to the diff. Please review.

I presume that you are using Apache names of SSL_* vars as the convention to follow.
https://httpd.apache.org/docs/2.4/mod/mod_ssl.html
Is that the case? I have not yet checked other popular web servers (e.g. nginx) or proxies (e.g. haproxy) or caching proxies (e.g. varnish) (and I am sure there are many other popular servers in each of those categories).

While on the subject, of variables, how do you think the values should be encoded? Please see the paragraphs below the env var table in the Environment Variables section, as well as the SSLOptions LegacyDNSStringFormat. What do you think? https://httpd.apache.org/docs/2.4/mod/mod_ssl.html

mackyle force-pushed the patch/extra-vars branch 2 times, most recently from 19b1832 to 7294e5a Compare

May 19, 2016 03:03

Member

gstrauss commented May 23, 2016

@mackyle have you had a chance to review my feedback? Also, as noted in #62, I plan to review all the TLS-related tickets together so that solutions will not conflict.

Member

gstrauss commented Jun 4, 2016

@mackyle have you had a chance to review my feedback? Also, as noted in #62, I plan to review all the TLS-related tickets together so that solutions will not conflict.


          response.c: set SSL_CLIENT_VERIFY & SSL_CLIENT_S_DN

bb9f62a

SSL_CLIENT_VERIFY is set to "NONE", "SUCCESS" or "FAILED:reason".
This is compatible with Apache's mod_ssl variable of the same name.

SSL_CLIENT_S_DN is set to the oneline version of the client certificate
subject's distinguished name and may be used as a setting for the
ssl.verifyclient.username config option.  When Apache's mod_ssl is
configured to use 'FakeBasicAuth' it uses the SSL_CLIENT_S_DN value for
the username (that ultimately may end up in REMOTE_USER).  The value
that will be set for SSL_CLIENT_S_DN may be determined using the
`openssl x509 -noout -subject -in <cert.pem>` command.

Signed-off-by: Kyle J. McKay

mackyle force-pushed the patch/extra-vars branch from 7294e5a to bb9f62a Compare

June 5, 2016 19:36

Member

gstrauss commented Jun 10, 2016

@mackyle have you had a chance to review my feedback? Also, as noted in #62, I plan to review all the TLS-related tickets together so that solutions will not conflict.

1 similar comment

Member

gstrauss commented Jun 20, 2016

@mackyle have you had a chance to review my feedback? Also, as noted in #62, I plan to review all the TLS-related tickets together so that solutions will not conflict.

Member

gstrauss commented Jul 4, 2016

@mackyle have you had a chance to review my feedback? Also, as noted in #62, I plan to review all the TLS-related tickets together so that solutions will not conflict.

There are additional tickets which request other env vars.

gstrauss reviewed

View reviewed changes

src/response.c

               		return;
               	}
+              	buffer_copy_string(ds_cv->value, "SUCCESS");
+              	array_insert_unique(con->environment, (data_unset *)ds_cv);

Member

gstrauss Oct 11, 2016 •

edited

above two lines should be part of the SSL_get_verify_result() condition block above to show that the outcome of the test sets SSL_CLIENT_VERIFY depending on the outcome of the condition test.

array_set_key_value() should be preferred over array_insert_unique() when it is desirable to replace an existing entry, if it exists, rather than having it handled by data_string insert_dup(), and perform a list append (combining the values with ", ") (And yes, other code should be modified, too, but that should be handled in a separate pull request)

Member

gstrauss commented Oct 11, 2016

The manpage for X509_NAME_oneline() says:
The functions X509_NAME_oneline() and X509_NAME_print() are legacy functions which produce a non standard output form, they don't handle multi character fields and have various quirks and inconsistencies. Their use is strongly discouraged in new applications.

Please consider using X509_NAME_print_ex() and propose a reasonable set of flags for a consistent and still-useful result.

gstrauss mentioned this pull request

[mod_openssl] ignore client verification error if not enforced #83

Closed

lighttpd-git closed this in

fb87ae8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment