Re: Using UPN from subjectAltName with SSLUserName
On Sun, Jun 28, 2015 at 05:11:57PM +0200, Kaspar Brand wrote:
On 22.06.2015 10:37, Jan Pazdziora wrote:
Please find a new patch attached which I hope covers all the
parts you've outlined, for SSL_CLIENT_SAN_OTHER_msUPN_*.
Thanks. Your implementation assumes that only a single otherName form
(msUPN) needs to be supported, but I would prefer to code it in a
somewhat more extensible way.
Does the attached patch work for you? As a practical way of
Yes it does.
My only question (and comments bellow) is about passing the oid rather
than nid through the functions. I can see the string id-on-dnsSRV
used twice in the patch in call OBJ_txt2nid and twice in call OBJ_txt2obj
as well when ideally all which should be needed one
OBJ_txt2nid(id-on-dnsSRV) and one OBJ_create if not found -- the rest
could be done by comparing integers (nid). Unless I'm missing something
about the oid/nid interaction.
demonstrating generic support of otherName forms, I have
added the case of the SRVName otherName form (RFC 4985, for things like
_carddavs._tcp.example.com, exposed via SSL_SERVER_SAN_OTHER_dnsSRV_n).
Kaspar
Index: modules/ssl/ssl_engine_init.c
===
--- modules/ssl/ssl_engine_init.c (revision 1687983)
+++ modules/ssl/ssl_engine_init.c (working copy)
@@ -352,6 +352,11 @@ apr_status_t ssl_init_Module(apr_pool_t *p, apr_po
init_dh_params();
+if (OBJ_txt2nid(id-on-dnsSRV) == NID_undef) {
+(void)OBJ_create(1.3.6.1.5.5.7.8.7, id-on-dnsSRV,
+ SRVName otherName form);
+}
Why do the OBJ_create call here and not in
ssl_var_lookup_ssl_cert_san, once it it clear it will be needed at
all?
Index: modules/ssl/ssl_util_ssl.c
===
--- modules/ssl/ssl_util_ssl.c(revision 1687983)
+++ modules/ssl/ssl_util_ssl.c(working copy)
@@ -252,19 +252,46 @@ char *modssl_X509_NAME_to_string(apr_pool_t *p, X5
return result;
}
+static void parse_otherName_value(apr_pool_t *p, ASN1_TYPE *value,
+ ASN1_OBJECT *oid,
+ apr_array_header_t **entries)
+{
+const char *str;
+
+if (!value || !oid || !*entries)
+ return;
+
+/*
+ * Currently supported otherName forms:
+ * - msUPN (1.3.6.1.4.1.311.20.2.3): Microsoft User Principal Name
+ * - dnsSRV (1.3.6.1.5.5.7.8.7): SRVName, as specified in RFC 4985
+ */
+if ((OBJ_obj2nid(oid) == NID_ms_upn)
Since the OBJ_obj2nid is made always before the comparison, could it be
simpler to just convert to nid in ssl_var_lookup_ssl_cert_san and pass
int, instead passingof ASN1_OBJECT * to modssl_X509_getSAN and
parse_otherName_value?
+(value-type == V_ASN1_UTF8STRING)
+(str = asn1_string_to_utf8(p, value-value.utf8string))) {
+APR_ARRAY_PUSH(*entries, const char *) = str;
+} else if ((OBJ_obj2nid(oid) == OBJ_txt2nid(id-on-dnsSRV))
+ (value-type == V_ASN1_IA5STRING)
+ (str = asn1_string_to_utf8(p, value-value.ia5string))) {
+APR_ARRAY_PUSH(*entries, const char *) = str;
+}
+}
+
/*
* Return an array of subjectAltName entries of type type. If idx is -1,
* return all entries of the given type, otherwise return an array consisting
* of the n-th occurrence of that type only. Currently supported types:
* GEN_EMAIL (rfc822Name)
* GEN_DNS (dNSName)
+ * GEN_OTHERNAME (see parse_otherName_value for currently supported forms)
*/
-BOOL modssl_X509_getSAN(apr_pool_t *p, X509 *x509, int type, int idx,
-apr_array_header_t **entries)
+BOOL modssl_X509_getSAN(apr_pool_t *p, X509 *x509, int type, ASN1_OBJECT
*oid,
+int idx, apr_array_header_t **entries)
{
STACK_OF(GENERAL_NAME) *names;
-if (!x509 || (type GEN_OTHERNAME) || (type GEN_RID) || (idx -1) ||
+if (!x509 || (type GEN_OTHERNAME) || (type == GEN_OTHERNAME !oid) ||
+(type GEN_RID) || (idx -1) ||
!(*entries = apr_array_make(p, 0, sizeof(char * {
*entries = NULL;
return FALSE;
@@ -277,33 +304,43 @@ char *modssl_X509_NAME_to_string(apr_pool_t *p, X5
for (i = 0; i sk_GENERAL_NAME_num(names); i++) {
name = sk_GENERAL_NAME_value(names, i);
-if (name-type == type) {
-if ((idx == -1) || (n == idx)) {
-switch (type) {
-case GEN_EMAIL:
-case GEN_DNS:
-utf8str = asn1_string_to_utf8(p, name-d.ia5);
-if (utf8str) {
-APR_ARRAY_PUSH(*entries, const char *) = utf8str;
-}
-break;
-default:
-/*
- * Not
Re: Using UPN from subjectAltName with SSLUserName
On Mon, Jun 29, 2015 at 01:47:45PM +0200, Jan Pazdziora wrote:
On Sun, Jun 28, 2015 at 05:11:57PM +0200, Kaspar Brand wrote:
On 22.06.2015 10:37, Jan Pazdziora wrote:
Please find a new patch attached which I hope covers all the
parts you've outlined, for SSL_CLIENT_SAN_OTHER_msUPN_*.
Thanks. Your implementation assumes that only a single otherName form
(msUPN) needs to be supported, but I would prefer to code it in a
somewhat more extensible way.
Does the attached patch work for you? As a practical way of
Yes it does.
My only question (and comments bellow) is about passing the oid rather
than nid through the functions. I can see the string id-on-dnsSRV
used twice in the patch in call OBJ_txt2nid and twice in call OBJ_txt2obj
as well when ideally all which should be needed one
OBJ_txt2nid(id-on-dnsSRV) and one OBJ_create if not found -- the rest
could be done by comparing integers (nid). Unless I'm missing something
about the oid/nid interaction.
Ah, now I see it -- you look at the semantics of oid to compare
value-type so nid would not be enough.
How about just passing char * and doing all the mapping logic
including possible OBJ_create in parse_otherName_value? My goal here
is to have all the hard work of determining the semantics isolated
in one place.
Please see patch attached.
--
Jan Pazdziora
Senior Principal Software Engineer, Identity Management Engineering, Red Hat
Index: modules/ssl/ssl_engine_init.c
===
--- modules/ssl/ssl_engine_init.c (revision 1688186)
+++ modules/ssl/ssl_engine_init.c (working copy)
@@ -1902,5 +1902,7 @@
free_dh_params();
+OBJ_cleanup();
+
return APR_SUCCESS;
}
Index: modules/ssl/ssl_engine_vars.c
===
--- modules/ssl/ssl_engine_vars.c (revision 1688186)
+++ modules/ssl/ssl_engine_vars.c (working copy)
@@ -665,6 +665,7 @@
{
int type, numlen;
apr_array_header_t *entries;
+const char *id = NULL;
if (strcEQn(var, Email_, 6)) {
type = GEN_EMAIL;
@@ -674,6 +675,20 @@
type = GEN_DNS;
var += 4;
}
+else if (strcEQn(var, OTHER_, 6)) {
+type = GEN_OTHERNAME;
+var += 6;
+if (strEQn(var, msUPN_, 6)) {
+var += 6;
+id = msUPN;
+}
+else if (strEQn(var, dnsSRV_, 7)) {
+var += 7;
+id = id-on-dnsSRV;
+}
+else
+ return NULL;
+}
else
return NULL;
@@ -682,7 +697,7 @@
if ((numlen 1) || (numlen 4) || (numlen != strlen(var)))
return NULL;
-if (modssl_X509_getSAN(p, xs, type, atoi(var), entries))
+if (modssl_X509_getSAN(p, xs, type, id, atoi(var), entries))
/* return the first entry from this 1-element array */
return APR_ARRAY_IDX(entries, 0, char *);
else
@@ -1032,12 +1047,15 @@
/* subjectAltName entries of the server certificate */
xs = SSL_get_certificate(ssl);
if (xs) {
-if (modssl_X509_getSAN(p, xs, GEN_EMAIL, -1, entries)) {
+if (modssl_X509_getSAN(p, xs, GEN_EMAIL, NULL, -1, entries)) {
extract_san_array(t, SSL_SERVER_SAN_Email, entries, p);
}
-if (modssl_X509_getSAN(p, xs, GEN_DNS, -1, entries)) {
+if (modssl_X509_getSAN(p, xs, GEN_DNS, NULL, -1, entries)) {
extract_san_array(t, SSL_SERVER_SAN_DNS, entries, p);
}
+if (modssl_X509_getSAN(p, xs, GEN_DNS, id-on-dnsSRV, -1, entries)) {
+extract_san_array(t, SSL_SERVER_SAN_OTHER_dnsSRV, entries, p);
+}
/* no need to free xs (refcount does not increase) */
}
@@ -1044,12 +1062,15 @@
/* subjectAltName entries of the client certificate */
xs = SSL_get_peer_certificate(ssl);
if (xs) {
-if (modssl_X509_getSAN(p, xs, GEN_EMAIL, -1, entries)) {
+if (modssl_X509_getSAN(p, xs, GEN_EMAIL, NULL, -1, entries)) {
extract_san_array(t, SSL_CLIENT_SAN_Email, entries, p);
}
-if (modssl_X509_getSAN(p, xs, GEN_DNS, -1, entries)) {
+if (modssl_X509_getSAN(p, xs, GEN_DNS, NULL, -1, entries)) {
extract_san_array(t, SSL_CLIENT_SAN_DNS, entries, p);
}
+if (modssl_X509_getSAN(p, xs, GEN_OTHERNAME, msUPN, -1, entries)) {
+extract_san_array(t, SSL_CLIENT_SAN_OTHER_msUPN, entries, p);
+}
X509_free(xs);
}
}
Index: modules/ssl/ssl_util_ssl.c
===
--- modules/ssl/ssl_util_ssl.c (revision 1688186)
+++ modules/ssl/ssl_util_ssl.c (working copy)
@@ -252,6 +252,46 @@
return result;
}
+static BOOL parse_otherName_value(apr_pool_t *p, OTHERNAME *othername,
+ const char *id,
+ apr_array_header_t **entries)
+{
+if (!othername ||
LimitRequestBody is broken in 2.4.13-2.4.15
Hi, LimitRequestBody is broken in the (unreleased) Apache versions 2.4.13-2.4.15 because of this change: http://svn.apache.org/r1684515 In http_filters.c, ap_http_filter(): The variable totalread is uninitialized if readbytes is 0. Messages similar to this one are logged: AH01591: Read content-length of 140067070814864 is larger than the configured limit of 104857600, and then Apache closes the connection. I hope that it's possible to fix this for Apache 2.4.16. Regards, Michael
Re: LimitRequestBody is broken in 2.4.13-2.4.15
On Mon, Jun 29, 2015 at 6:57 PM, Michael Kaufmann m...@michael-kaufmann.ch wrote: LimitRequestBody is broken in the (unreleased) Apache versions 2.4.13-2.4.15 because of this change: http://svn.apache.org/r1684515 In http_filters.c, ap_http_filter(): The variable totalread is uninitialized if readbytes is 0. Messages similar to this one are logged: AH01591: Read content-length of 140067070814864 is larger than the configured limit of 104857600, and then Apache closes the connection. Thanks for reporting this before the testing/release. Fixed in r1688274 (will now propose a backport), and since this is a showstopper, it will be merged (once reviewed) before 2.4.16/2.2.30. Regards, Yann.
Re: svn commit: r1688331 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
On Tue, Jun 30, 2015 at 2:03 AM, William A Rowe Jr wr...@rowe-clan.net wrote:
I can't approve this semantic mess.
EITHER it is inherit_before on trunk-2.4-2.2 with a change of default
behavior, or it is inherit_after, again across all branches with a change of
default behavior. The delta should consist of a one line difference,
evaluating inheritance behavior within the merge.
Well, that's the case already, no?
With 2.4.x patch applied:
--- 2.4.x/modules/filters/mod_substitute.c 2015-06-30
01:52:18.595947091 +0200
+++ trunk/modules/filters/mod_substitute.c 2015-06-30
01:41:18.027679427 +0200
@@ -87,7 +87,7 @@
subst_dir_conf *over = (subst_dir_conf *) overv;
a-inherit_before = (over-inherit_before 0 ||
(over-inherit_before 0
-
base-inherit_before != 0));
+
base-inherit_before 0));
if (a-inherit_before) {
a-patterns = apr_array_append(p, base-patterns,
over-patterns);
Please express your preference and I will offer several style fixes on trunk
that make this easier to follow, but we are not adding one directive to
trunk and a different one to 2.4 2.2 :-/
Same directive in trunk and 2.[24] branches, default only changes, I
don't see what you mean.
This proposal allows to merge the inherit_before flag itself, that may
be confusing / not suitable / overkill (dunno), so feel free to
implement simpler/better code (the default merge-base-before-over
semantic must be preserved for the branches, though).
Re: LimitRequestBody is broken in 2.4.13-2.4.15
On Mon, Jun 29, 2015 at 7:58 PM, Yann Ylavic ylavic@gmail.com wrote: On Mon, Jun 29, 2015 at 6:57 PM, Michael Kaufmann m...@michael-kaufmann.ch wrote: LimitRequestBody is broken in the (unreleased) Apache versions 2.4.13-2.4.15 because of this change: http://svn.apache.org/r1684515 In http_filters.c, ap_http_filter(): The variable totalread is uninitialized if readbytes is 0. Messages similar to this one are logged: AH01591: Read content-length of 140067070814864 is larger than the configured limit of 104857600, and then Apache closes the connection. Thanks for reporting this before the testing/release. Fixed in r1688274 (will now propose a backport), and since this is a showstopper, it will be merged (once reviewed) before 2.4.16/2.2.30. Proposed patch (for backport) is http://people.apache.org/~ylavic/httpd-2.4.x-fix_LimitRequestBody.patch Thanks (again) for testing if that's possible. Regards, Yann.
Re: svn commit: r1688331 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
I can't approve this semantic mess.
EITHER it is inherit_before on trunk-2.4-2.2 with a change of default
behavior, or it is inherit_after, again across all branches with a change
of default behavior. The delta should consist of a one line difference,
evaluating inheritance behavior within the merge.
Please express your preference and I will offer several style fixes on
trunk that make this easier to follow, but we are not adding one directive
to trunk and a different one to 2.4 2.2 :-/
On Jun 29, 2015 6:44 PM, yla...@apache.org wrote:
Author: ylavic
Date: Mon Jun 29 23:44:28 2015
New Revision: 1688331
URL: http://svn.apache.org/r1688331
Log:
mod_substitute: follow up to r1687680.
Fix dir config merger 'over'-write, thanks Bill (again).
Modified:
httpd/httpd/trunk/modules/filters/mod_substitute.c
Modified: httpd/httpd/trunk/modules/filters/mod_substitute.c
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_substitute.c?rev=1688331r1=1688330r2=1688331view=diff
==
--- httpd/httpd/trunk/modules/filters/mod_substitute.c (original)
+++ httpd/httpd/trunk/modules/filters/mod_substitute.c Mon Jun 29 23:44:28
2015
@@ -86,10 +86,9 @@ static void *merge_substitute_dcfg(apr_p
subst_dir_conf *base = (subst_dir_conf *) basev;
subst_dir_conf *over = (subst_dir_conf *) overv;
-if (over-inherit_before 0) {
-over-inherit_before = (base-inherit_before 0);
-}
-if (over-inherit_before) {
+a-inherit_before = (over-inherit_before 0 ||
(over-inherit_before 0
+
base-inherit_before 0));
+if (a-inherit_before) {
a-patterns = apr_array_append(p, base-patterns,
over-patterns);
}
Re: svn commit: r1688331 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
Maybe defining (naming) inherit_before tristate values would help:
Index: modules/filters/mod_substitute.c
===
--- modules/filters/mod_substitute.c(revision 1688331)
+++ modules/filters/mod_substitute.c(working copy)
@@ -68,6 +68,10 @@ typedef struct {
apr_pool_t *tpool;
} substitute_module_ctx;
+#define INHERIT_UNSET -1
+#define INHERIT_OFF 0
+#define INHERIT_ON 1
+
static void *create_substitute_dcfg(apr_pool_t *p, char *d)
{
subst_dir_conf *dcfg =
@@ -75,7 +79,7 @@ static void *create_substitute_dcfg(apr_pool_t *p,
dcfg-patterns = apr_array_make(p, 10, sizeof(subst_pattern_t));
dcfg-max_line_length = AP_SUBST_MAX_LINE_LENGTH;
-dcfg-inherit_before = -1;
+dcfg-inherit_before = INHERIT_UNSET;
return dcfg;
}
@@ -86,8 +90,9 @@ static void *merge_substitute_dcfg(apr_pool_t *p,
subst_dir_conf *base = (subst_dir_conf *) basev;
subst_dir_conf *over = (subst_dir_conf *) overv;
-a-inherit_before = (over-inherit_before 0 ||
(over-inherit_before 0
-
base-inherit_before 0));
+a-inherit_before = (over-inherit_before == INHERIT_ON
+ || (over-inherit_before == INHERIT_UNSET
+ base-inherit_before == INHERIT_ON));
if (a-inherit_before) {
a-patterns = apr_array_append(p, base-patterns,
over-patterns);
?
Which would be:
+a-inherit_before = (over-inherit_before == INHERIT_ON
+ || (over-inherit_before == INHERIT_UNSET
+ base-inherit_before != INHERIT_OFF));
in 2.2 and 2.4.
On Tue, Jun 30, 2015 at 2:50 AM, William A Rowe Jr wr...@rowe-clan.net wrote:
I was literally switching between a dead and live box repairing a corrupted
boot volume, so you may be right or I might have studied a stale patch.
Will refresh trunk in a few minutes here with suggested changes.
On Jun 29, 2015 7:42 PM, Yann Ylavic ylavic@gmail.com wrote:
On Tue, Jun 30, 2015 at 2:03 AM, William A Rowe Jr wr...@rowe-clan.net
wrote:
I can't approve this semantic mess.
EITHER it is inherit_before on trunk-2.4-2.2 with a change of default
behavior, or it is inherit_after, again across all branches with a
change of
default behavior. The delta should consist of a one line difference,
evaluating inheritance behavior within the merge.
Well, that's the case already, no?
With 2.4.x patch applied:
--- 2.4.x/modules/filters/mod_substitute.c 2015-06-30
01:52:18.595947091 +0200
+++ trunk/modules/filters/mod_substitute.c 2015-06-30
01:41:18.027679427 +0200
@@ -87,7 +87,7 @@
subst_dir_conf *over = (subst_dir_conf *) overv;
a-inherit_before = (over-inherit_before 0 ||
(over-inherit_before 0
-
base-inherit_before != 0));
+
base-inherit_before 0));
if (a-inherit_before) {
a-patterns = apr_array_append(p, base-patterns,
over-patterns);
Please express your preference and I will offer several style fixes on
trunk
that make this easier to follow, but we are not adding one directive to
trunk and a different one to 2.4 2.2 :-/
Same directive in trunk and 2.[24] branches, default only changes, I
don't see what you mean.
This proposal allows to merge the inherit_before flag itself, that may
be confusing / not suitable / overkill (dunno), so feel free to
implement simpler/better code (the default merge-base-before-over
semantic must be preserved for the branches, though).
Re: svn commit: r1688339 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
On Mon, Jun 29, 2015 at 9:44 PM, William A Rowe Jr wr...@rowe-clan.net wrote: You ALWAYS preserve unset state. How else do you perform the THIRD merge? To be more specific, httpd is allowed to merge whatever merges it likes. If it wants to optimize for the directory and then merge the base server to the merged directory, that's legit. Overthinking the merge function yields worthless results for any of the possible optimization strategies.
Re: svn commit: r1688339 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
For 2.2/2.4 the delta is a one line change to trunk's behavior;
On Mon, Jun 29, 2015 at 8:27 PM, wr...@apache.org wrote:
Author: wrowe
Date: Tue Jun 30 01:27:42 2015
New Revision: 1688339
URL: http://svn.apache.org/r1688339
Log:
Very difficult to read, and therefore was wrong.
Assert that the SubstituteInheritBefore option was explicitly toggled,
and do not default in 2.x to this legacy behavior.
Modified:
httpd/httpd/trunk/modules/filters/mod_substitute.c
Modified: httpd/httpd/trunk/modules/filters/mod_substitute.c
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_substitute.c?rev=1688339r1=1688338r2=1688339view=diff
==
--- httpd/httpd/trunk/modules/filters/mod_substitute.c (original)
+++ httpd/httpd/trunk/modules/filters/mod_substitute.c Tue Jun 30 01:27:42
2015
@@ -86,9 +86,16 @@ static void *merge_substitute_dcfg(apr_p
subst_dir_conf *base = (subst_dir_conf *) basev;
subst_dir_conf *over = (subst_dir_conf *) overv;
-a-inherit_before = (over-inherit_before 0 ||
(over-inherit_before 0
-
base-inherit_before 0));
-if (a-inherit_before) {
+a-inherit_before = (over-inherit_before != -1)
+? over-inherit_before
+: base-inherit_before;
+/* SubstituteInheritBefore was the default behavior until 2.5.x,
+ * and may be re-enabled as desired; this original default behavior
+ * was to apply inherited subst patterns before locally scoped
patterns.
+ * In later 2.2 and 2.4 versions, SubstituteInheritBefore may be
toggled
+ * 'off' to follow the corrected/expected behavior, without violating
POLS.
+ */
+if (a-inherit_before == 1) {
This becomes
if (a-inherit_before)
both the default case and explicitly toggled case will honor the legacy
2.2/2.4 behavior.
Explicitly toggling 'SubstitueInheritBefore off' will 'upgrade' to the
expected 2.5 behavior.
Re: svn commit: r1688339 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
You ALWAYS preserve unset state. How else do you perform the THIRD merge?
On Mon, Jun 29, 2015 at 9:01 PM, Yann Ylavic ylavic@gmail.com wrote:
This won't work for eg, this second level inheritance: server context
is on, vhost and inner Location are unset.
Location-inherit_before will be unset whereas it should be on.
We should not preserve the unset state IMHO.
On Tue, Jun 30, 2015 at 3:27 AM, wr...@apache.org wrote:
Author: wrowe
Date: Tue Jun 30 01:27:42 2015
New Revision: 1688339
URL: http://svn.apache.org/r1688339
Log:
Very difficult to read, and therefore was wrong.
Assert that the SubstituteInheritBefore option was explicitly toggled,
and do not default in 2.x to this legacy behavior.
Modified:
httpd/httpd/trunk/modules/filters/mod_substitute.c
Modified: httpd/httpd/trunk/modules/filters/mod_substitute.c
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_substitute.c?rev=1688339r1=1688338r2=1688339view=diff
==
--- httpd/httpd/trunk/modules/filters/mod_substitute.c (original)
+++ httpd/httpd/trunk/modules/filters/mod_substitute.c Tue Jun 30
01:27:42 2015
@@ -86,9 +86,16 @@ static void *merge_substitute_dcfg(apr_p
subst_dir_conf *base = (subst_dir_conf *) basev;
subst_dir_conf *over = (subst_dir_conf *) overv;
-a-inherit_before = (over-inherit_before 0 ||
(over-inherit_before 0
-
base-inherit_before 0));
-if (a-inherit_before) {
+a-inherit_before = (over-inherit_before != -1)
+? over-inherit_before
+: base-inherit_before;
+/* SubstituteInheritBefore was the default behavior until 2.5.x,
+ * and may be re-enabled as desired; this original default behavior
+ * was to apply inherited subst patterns before locally scoped
patterns.
+ * In later 2.2 and 2.4 versions, SubstituteInheritBefore may be
toggled
+ * 'off' to follow the corrected/expected behavior, without
violating POLS.
+ */
+if (a-inherit_before == 1) {
a-patterns = apr_array_append(p, base-patterns,
over-patterns);
}
Re: svn commit: r1688331 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
I was literally switching between a dead and live box repairing a corrupted
boot volume, so you may be right or I might have studied a stale patch.
Will refresh trunk in a few minutes here with suggested changes.
On Jun 29, 2015 7:42 PM, Yann Ylavic ylavic@gmail.com wrote:
On Tue, Jun 30, 2015 at 2:03 AM, William A Rowe Jr wr...@rowe-clan.net
wrote:
I can't approve this semantic mess.
EITHER it is inherit_before on trunk-2.4-2.2 with a change of default
behavior, or it is inherit_after, again across all branches with a
change of
default behavior. The delta should consist of a one line difference,
evaluating inheritance behavior within the merge.
Well, that's the case already, no?
With 2.4.x patch applied:
--- 2.4.x/modules/filters/mod_substitute.c 2015-06-30
01:52:18.595947091 +0200
+++ trunk/modules/filters/mod_substitute.c 2015-06-30
01:41:18.027679427 +0200
@@ -87,7 +87,7 @@
subst_dir_conf *over = (subst_dir_conf *) overv;
a-inherit_before = (over-inherit_before 0 ||
(over-inherit_before 0
-
base-inherit_before != 0));
+
base-inherit_before 0));
if (a-inherit_before) {
a-patterns = apr_array_append(p, base-patterns,
over-patterns);
Please express your preference and I will offer several style fixes on
trunk
that make this easier to follow, but we are not adding one directive to
trunk and a different one to 2.4 2.2 :-/
Same directive in trunk and 2.[24] branches, default only changes, I
don't see what you mean.
This proposal allows to merge the inherit_before flag itself, that may
be confusing / not suitable / overkill (dunno), so feel free to
implement simpler/better code (the default merge-base-before-over
semantic must be preserved for the branches, though).
Re: [VOTE] Release Apache httpd 2.4.15 as GA
On Mon, Jun 22, 2015 at 2:01 PM, André Malo n...@perlig.de wrote: * Yann Ylavic wrote: It seems that RedirectMatch isn't documented without the third (URL) argument, unless in Location. Huh? Actually it is (or maybe I'm not getting something here). I checked at least back until 2.0. http://httpd.apache.org/docs/2.4/mod/mod_alias.html#redirect clearly documents this. That's helpful, except that #redirectmatch does not point back to #redirect as authoritative over arguments. We all now agree that the 2.4.12 and prior behavior should be preserved, but that doesn't remedy the documentation.
Re: svn commit: r1688331 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
On Mon, Jun 29, 2015 at 8:06 PM, Yann Ylavic ylavic@gmail.com wrote:
Maybe defining (naming) inherit_before tristate values would help:
Not really...
+a-inherit_before = (over-inherit_before == INHERIT_ON
+ || (over-inherit_before == INHERIT_UNSET
+ base-inherit_before == INHERIT_ON));
if (a-inherit_before) {
This logic was convoluted and therefore resulted in in the old default
behavior if the option wasn't explicitly set. See the most recent trunk
commits for a more legible solution that follows the design pattern used
throughout other core modules.
Your logic above fails to preserve the unset state, and fails to when
consider base-inherit_before is explicitly off. This is why it is
preferred not to invent new wheels when good wheels exist, particularly if
there will be a square side on the new wheel.
Bill
Re: svn commit: r1688339 - /httpd/httpd/trunk/modules/filters/mod_substitute.c
This won't work for eg, this second level inheritance: server context
is on, vhost and inner Location are unset.
Location-inherit_before will be unset whereas it should be on.
We should not preserve the unset state IMHO.
On Tue, Jun 30, 2015 at 3:27 AM, wr...@apache.org wrote:
Author: wrowe
Date: Tue Jun 30 01:27:42 2015
New Revision: 1688339
URL: http://svn.apache.org/r1688339
Log:
Very difficult to read, and therefore was wrong.
Assert that the SubstituteInheritBefore option was explicitly toggled,
and do not default in 2.x to this legacy behavior.
Modified:
httpd/httpd/trunk/modules/filters/mod_substitute.c
Modified: httpd/httpd/trunk/modules/filters/mod_substitute.c
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_substitute.c?rev=1688339r1=1688338r2=1688339view=diff
==
--- httpd/httpd/trunk/modules/filters/mod_substitute.c (original)
+++ httpd/httpd/trunk/modules/filters/mod_substitute.c Tue Jun 30 01:27:42
2015
@@ -86,9 +86,16 @@ static void *merge_substitute_dcfg(apr_p
subst_dir_conf *base = (subst_dir_conf *) basev;
subst_dir_conf *over = (subst_dir_conf *) overv;
-a-inherit_before = (over-inherit_before 0 || (over-inherit_before
0
- base-inherit_before
0));
-if (a-inherit_before) {
+a-inherit_before = (over-inherit_before != -1)
+? over-inherit_before
+: base-inherit_before;
+/* SubstituteInheritBefore was the default behavior until 2.5.x,
+ * and may be re-enabled as desired; this original default behavior
+ * was to apply inherited subst patterns before locally scoped patterns.
+ * In later 2.2 and 2.4 versions, SubstituteInheritBefore may be toggled
+ * 'off' to follow the corrected/expected behavior, without violating
POLS.
+ */
+if (a-inherit_before == 1) {
a-patterns = apr_array_append(p, base-patterns,
over-patterns);
}
