Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Attribute authorization #33

Merged
merged 31 commits into from

6 participants

Jonathan Daugherty Matt Ben Noordhuis pames Max Bowsher Patrick O'Connor
Jonathan Daugherty

This pull request contains the original attribute authorization work by Christian Folini with additional changes, improvements, fixes, and tests by Josh Hoyt, Jonathan Daugherty, and Dylan McNamee at Galois, Inc. For reference, it has been discussed here:

https://issues.jasig.org/browse/MAS-60

Josh Hoyt and others added some commits
Josh Hoyt Applied Christian Folini's SAML attribute authorization patch 179c68f
Josh Hoyt Fix NULL dereference on "Require cas-attribute ..." when no attribute…
…s are present
1f0007d
Josh Hoyt Improvements to Christian Folini's attribute authorization patch
<https://issues.jasig.org/browse/MAS-60>:

* Use ap_getword_conf to tokenize the Require directive. This means
  that attribute specifications can use any character (including
  whitespace). For example:

    Require cas-attribute "last-name:de Soto"

* Pass a reference to the struct of attributes that's parsed out of
  the SAML response in order to do authorization rather than using
  attributes serialized into a notes field. This eliminates ambiguity
  due to serialization, as well as being more efficient.

* Factor out the routine that evaluates an attribute specification
  against the set of attributes. This improves readability as well as
  testability. Also, be explicit about exactly what will match an
  attribute specification, and the behavior in the face of ambiguity.
3676a45
Josh Hoyt Formatting and adding const where applicable eaba372
Josh Hoyt Encapsulate the attribute storage into an API f2344e3
Josh Hoyt Fix the argument passed to ap_get/set_module_config in cas_get/set_at…
…tributes

The parameter was incorrectly set to the request_rec pointer itself
rather than the request_config field of the request. Depending on
Apache's configuration, this resulted in different parts of the
request structure being overwritten with the attribute pointer. In
many cases, it worked correctly by accident. This change correctly
uses the request_config conf_vector for ap_get/set_module_config.

In addition to that change, cas_get_attributes also works better in a
subrequest: if no attributes are found in the subrequest, it will look
in the parent request for attributes.
b55b7dc
Josh Hoyt Applied Christian Folini's SAML attribute authorization patch 2bcb1b2
Josh Hoyt Fix NULL dereference on "Require cas-attribute ..." when no attribute…
…s are present
3b72dd3
Josh Hoyt Improvements to Christian Folini's attribute authorization patch
<https://issues.jasig.org/browse/MAS-60>:

* Use ap_getword_conf to tokenize the Require directive. This means
  that attribute specifications can use any character (including
  whitespace). For example:

    Require cas-attribute "last-name:de Soto"

* Pass a reference to the struct of attributes that's parsed out of
  the SAML response in order to do authorization rather than using
  attributes serialized into a notes field. This eliminates ambiguity
  due to serialization, as well as being more efficient.

* Factor out the routine that evaluates an attribute specification
  against the set of attributes. This improves readability as well as
  testability. Also, be explicit about exactly what will match an
  attribute specification, and the behavior in the face of ambiguity.
68608a2
Josh Hoyt Formatting and adding const where applicable 0388156
Josh Hoyt Encapsulate the attribute storage into an API 496d836
Josh Hoyt Fix the argument passed to ap_get/set_module_config in cas_get/set_at…
…tributes

The parameter was incorrectly set to the request_rec pointer itself
rather than the request_config field of the request. Depending on
Apache's configuration, this resulted in different parts of the
request structure being overwritten with the attribute pointer. In
many cases, it worked correctly by accident. This change correctly
uses the request_config conf_vector for ap_get/set_module_config.

In addition to that change, cas_get_attributes also works better in a
subrequest: if no attributes are found in the subrequest, it will look
in the parent request for attributes.
60eede2
root more explicit about what the cas-attribute directive does 83842a5
Dylan McNamee mod_auth_cas now logs a warning if Require cas-attribute is present i…
…n the

config file, but no actual attributes are specified.

had to add <openssl/crypto.h> in includes to get module to compile on 64-bit
system (regression testing needed - did I break 32-bit?)

started adding hooks for unit testing cas-attribute parsing and enforcement
69ff9a4
Dylan McNamee Merge branch 'folini-authz' of https://github.com/jtdaugherty/mod_aut…
…h_cas into folini-authz

Conflicts:
	README
	src/mod_auth_cas.c
	src/mod_auth_cas.h
3b26e29
Dylan McNamee Unit test for cas_authorize. Cleanup of cas_saml_attr_test for warnin…
…g-free GCC compiles.

Had to pull in some attribute parsing code from apache to get authz unit tests to go through.
31c4d4a
Dylan McNamee removing warnings... 8fae170
Dylan McNamee fixing warnings. 7b1fb97
Dylan McNamee tweak to remove another warning 8ce0dc1
Dylan McNamee removing one last debugging printf a95f2cf
Jonathan Daugherty jtdaugherty Merge upstream master into folini-authz 3c703c4
Jonathan Daugherty jtdaugherty tests: fix stub for ap_hook_auth_checker 13be05e
Jonathan Daugherty jtdaugherty tests: fix return type of ap_hook_auth_checker f3e7c7e
Jonathan Daugherty jtdaugherty tests: fix warnings and errors from merge 87b7ced
Jonathan Daugherty jtdaugherty tests: fix null terminator eba1442
Matt
Owner

Excellent, happy to see this functionality! Sizable pull though, history goes back ~8 months, so this will take a bit of review. I'll start poking later this week.

Jonathan Daugherty
Matt
Owner

Don't worry about the rebase - I've got some other work to do in here anyway, so I'll try to merge as I go.

Jonathan Daugherty

Hi. How is the merge going? Do you have any questions?

Matt
Owner

Doing ok. This merge exposed another bug or two that I'm also working through, slow going .. but it is still going.

Matt forsetti was assigned
Matt
Owner

Merged into feature branch (jasig/attz). Please check out branch and test. I would like an LGTM from at least @jtdaugherty and 2 of @pames, @smaresca, or @bnoordhuis before merging to master, to confirm that the new functionality works as expected, and that none of the Apache 2.4 work has been broken,

src/mod_auth_cas.c
((136 lines not shown))
+ /* Go through applicable Require directives */
+ for (i = 0; i < nelts; ++i) {
+ /* Ignore this Require if it's in a <Limit> section
+ * that exclude this method
+ */
+
+ if (!(reqs[i].method_mask & (AP_METHOD_BIT << m))) {
+ continue;
+ }
+
+ /* ignore if it's not a "Require cas-attribute ..." */
+ requirement = reqs[i].requirement;
+
+ token = ap_getword_white(r->pool, &requirement);
+
+ if (strcasecmp(token, "cas-attribute")) {
pames Owner
pames added a note

Should this use apr_strnatcasecmp for consistency?

Matt Owner
forsetti added a note

@jtdaugherty Please see notes from @pames

I don't know the answer to this. Is apr_strnatcasecmp an Apache function to be preferred in place of a C library version for portability reasons? If so, I'm all for it. :)

I see now that code we added elsewhere does use apr_strnatcasecmp, so yes, we should have used that here, too. Thanks for spotting this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
pames pames commented on the diff
src/mod_auth_cas.c
((119 lines not shown))
+
+ return (cas_authorize_worker(r, attrs, reqs, reqs_arr->nelts, c));
+}
+
+/* Pulled out from cas_authorize to enable unit-testing */
+
+int cas_authorize_worker(request_rec *r, const cas_saml_attr *const attrs, const require_line *const reqs, int nelts, const cas_cfg *const c)
+{
+ const int m = r->method_number;
+ const char *token;
+ const char *requirement;
+ int i;
+ int have_casattr = 0;
+ int count_casattr = 0;
+
+ // Q: why don't we use ap_some_auth_required here?? performance?
pames Owner
pames added a note

Any answer to this question? Seems like you might be duplicating some level of checks that ap_some_auth_required would run below.

Matt Owner
forsetti added a note

@jtdaugherty Please see notes from @pames

During development we encountered that function elsewhere and it looked like something we ought to use, but we aren't entirely sure if it's the right thing to do. Some advice would be good. If you aren't sure either, I'd say the comment is spurious (as the tests pass).

Ben Noordhuis Owner

You're duplicating core httpd functionality now. Using ap_some_auth_required() here is the right thing.

Can you help me understand what I need to replace? The docs on that function are pretty opaque and its implementation is just an indirection layer, so it's not clear to me what, precisely, is being duplicated.

Ben Noordhuis Owner
+    if (!(reqs[i].method_mask & (AP_METHOD_BIT << m))) {
+      continue;
+    }

That bit is lifted almost verbatim from the 2.2 ap_some_auth_required() (it hooks into the authz layer in 2.4).

Thanks for the clarification. The difference between ap_some_auth_required and the code in the loop mentioned here is that ap_some_auth_required returns true if there are any require lines in the request which match the method, whereas the loop is specifically checking to see whether each require should be considered on that basis. As a result we can't use ap_some_auth_required in the loop because it isn't specific enough, and if we move it outside the loop that will be fine except inside the loop we'll be considering require lines that we shouldn't be looking at, so the code that is there right now will still be required. Does that make sense?

Ben Noordhuis Owner

Does that make sense?

Yes, thanks for clarifying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/mod_auth_cas_test.c
((35 lines not shown))
+ i++;
+ }
+
+ c = ap_get_module_config(request->server->module_config,
+ &auth_cas_module);
+
+ /* manually create two require lines */
+ r = &(require_line_array[0]);
+ r->method_mask = AP_METHOD_BIT;
+ r->requirement = apr_pstrdup(pool, "cas-attribute hopefully:fail");
+
+ r = &(require_line_array[1]);
+ r->method_mask = AP_METHOD_BIT;
+ r->requirement = apr_pstrdup(pool, "cas-attribute should:succeed");
+
+ c->CASAuthoritative = 1;
pames Owner
pames added a note

Can you add a little bit of description (comments are fine) as to why the outcome of each test is as you expect? i.e. "should fail" because authoritative and attribute not reachable, should decline because not authoritative and not reachable, etc. so there's a bit more human-readable understanding of why these cases were selected?

also, minor nit, this currently doesn't test that an authoritative, present, attribute would result in authentication success but I agree it's unlikely that it would fail where as the non-authoritative would succeed.

Matt Owner
forsetti added a note

@jtdaugherty Please see notes from @pames

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Jonathan Daugherty jtdaugherty commented on the diff
src/mod_auth_cas.c
((136 lines not shown))
+ /* Go through applicable Require directives */
+ for (i = 0; i < nelts; ++i) {
+ /* Ignore this Require if it's in a <Limit> section
+ * that exclude this method
+ */
+
+ if (!(reqs[i].method_mask & (AP_METHOD_BIT << m))) {
+ continue;
+ }
+
+ /* ignore if it's not a "Require cas-attribute ..." */
+ requirement = reqs[i].requirement;
+
+ token = ap_getword_white(r->pool, &requirement);
+
+ if (apr_strnatcasecmp(token, "cas-attribute") != 0) {

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Jonathan Daugherty jtdaugherty commented on the diff
tests/mod_auth_cas_test.c
((70 lines not shown))
+ should_succeed2 = cas_authorize_worker(request, attrs, &(require_line_array[0]), 2, c);
+
+ // Regardless of whether mod_auth_cas is authoritative, the empty
+ // list of Require directives means mod_auth_cas has no policy to
+ // check and should DECLINE.
+ c->CASAuthoritative = 1;
+ should_decline1 = cas_authorize_worker(request, attrs, NULL, 0, c);
+ c->CASAuthoritative = 0;
+ should_decline2 = cas_authorize_worker(request, attrs, NULL, 0, c);
+
+ fail_unless((should_fail == HTTP_UNAUTHORIZED) &&
+ (should_succeed1 == OK) &&
+ (should_succeed2 == OK) &&
+ (should_decline1 == DECLINED) &&
+ (should_decline2 == DECLINED));
+}

Comments and more tests added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Matt forsetti commented on the diff
src/mod_auth_cas.h
((5 lines not shown))
-#define CAS_MAX_RESPONSE_SIZE 16384
+#define CAS_MAX_RESPONSE_SIZE 65536
Matt Owner

@jtdaugherty Why the increase from 16k to 64k? Did you find that you were producing large SAML messages?

@pames What would you think of a realloc loop dynamically resizing up to CAS_MAX_RESPONSE_SIZE ? We could start at (e.g.) 4k, resize to 64k if necessary, reducing memory overhead. Given that this is an HTTP read, I don't think the performance hit of realloc would be noticed.

pames Owner
pames added a note

Nice thing about keeping it in the stack frame is that it just gets allocated inherently. Simpler code overall, and we don't introduce any overhead to reallocate, etc. Plus if someone is shipping you more than, e.g. 16k of data then there's probably something very wrong (or you have a ridiculous amount of attributes).

I don't think the overhead would be much but I don't think the extra complexity really warrants it when we're talking about a fairly well defined response with a size limit we can pretty reasonably cap.

That change is from Christian Folini's original patch so I can't comment on the change. I can say, at least, that it was a change from 4k to 64k in the original patch. 4k does strike me as a bit small in any case, considering the messages involved, but 64k is overkill. I'm happy to revert this to 16k; I don't think that will hurt anything.

Matt Owner
pames Owner
pames added a note
Matt Owner

Can't find an authoritative spec - just a couple examples generally starting ~64K and allowing increase to 1M.

So -- I'd recommend we start with 16k, and if we have to go larger, I'll work up the realloc loop (+ cleanup) for dynamic growth to a larger max. Thoughts?

pames Owner
pames added a note

If everyone else uses 64k then it's fine to leave at 64k. It just seems excessive to me, particularly given the practical experience we have here.

Ben Noordhuis Owner

At my last job we had a kind of ersatz attribute management that hit the 16k limit with some users so I bumped it to 64k on-stack with no ill effect.

I suggest raising the limit and keeping it on-stack for simplicity's sake. A 64k stack frame is not a problem on modern systems (the ones people actually use) so I don't see a downside.

Ben Noordhuis Owner

We could also switch to SAX parsing with apr_xml_parser_feed + apr_xml_parser_done. No allocation - on-stack, heap or otherwise - required in m-a-c.

pames Owner
pames added a note

FWIW I like the simplicity of the current approach, and I agree with your assessment about modern stack sizes (for instance RLIMIT_STACK on my box is 8MB - depending on the Apache threading model maybe that value is significantly less per thread but I doubt it will be an issue in practice due to the nature of the function call in question).

In any case I think far too many man-hours have been expended debating this by now so let's just leave it at 64k :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/mod_auth_cas_test.c
((48 lines not shown))
+ r = &(require_line_array[0]);
+ r->method_mask = AP_METHOD_BIT;
+ r->requirement = apr_pstrdup(pool, "cas-attribute hopefully:fail");
+
+ r = &(require_line_array[1]);
+ r->method_mask = AP_METHOD_BIT;
+ r->requirement = apr_pstrdup(pool, "cas-attribute should:succeed");
+
+ // When mod_auth_cas is authoritative, an attribute payload which
+ // fails to pass the policy check should result in
+ // HTTP_UNAUTHORIZED.
+ c->CASAuthoritative = 1;
+ should_fail = cas_authorize_worker(request, attrs, &(require_line_array[0]), 1, c);
+
+ // When mod_auth_cas is authoritative, an attribute payload which
+ // does pass the policy check should succeed.

Fixed a typo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Ben Noordhuis bnoordhuis commented on the diff
src/mod_auth_cas.c
@@ -375,6 +377,14 @@ const char *cfg_readCASParameter(cmd_parms *cmd, void *cfg, const char *value)
else
return(apr_psprintf(cmd->pool, "MOD_AUTH_CAS: Invalid argument to CASSSOEnabled - must be 'On' or 'Off'"));
break;
+ case cmd_authoritative:
+ if(apr_strnatcasecmp(value, "On") == 0)
+ c->CASAuthoritative = TRUE;
+ else if(apr_strnatcasecmp(value, "Off") == 0)
+ c->CASAuthoritative = FALSE;
+ else
+ return(apr_psprintf(cmd->pool, "MOD_AUTH_CAS: Invalid argument to CASAuthoritative - must be 'On' or 'Off'"));
Ben Noordhuis Owner

I know you're not the one who introduced this pattern but this really should be a AP_INIT_FLAG directive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/ap_stubs.c
((12 lines not shown))
+ * allowed us to test most of the authz functions fairly thoroughly.
+ */
+#define apr_isspace(c) (isspace(((unsigned char)(c))))
+
+AP_DECLARE(char *) ap_getword_white(apr_pool_t *p, const char **line) {
+
+ const char *pos = *line;
+ int len;
+ char *res;
+
+ while (!apr_isspace(*pos) && *pos) {
+ ++pos;
+ }
+
+ len = pos - *line;
+ res = (char *)calloc(sizeof(char), len + 1);
Ben Noordhuis Owner

Superfluous cast + sizeof(char) is always 1.

Fixed.

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

LGTM FWIW

tests/mod_auth_cas_test.c
@@ -751,14 +761,99 @@ char *get_attr(cas_cfg *c, cas_saml_attr *attrs, const char *attr) {
}
END_TEST
+START_TEST(cas_attribute_authz_test) {
+ int should_fail, should_succeed1, should_succeed2,
+ should_decline1, should_decline2;
+ cas_saml_attr *attrs = NULL;
+ cas_attr_builder *builder;
+ require_line require_line_array[2];
+ cas_cfg *c;
+ // cas_dir_cfg *d;
pames Owner
pames added a note

remove?

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/mod_auth_cas_test.c
@@ -751,14 +761,99 @@ char *get_attr(cas_cfg *c, cas_saml_attr *attrs, const char *attr) {
}
END_TEST
+START_TEST(cas_attribute_authz_test) {
+ int should_fail, should_succeed1, should_succeed2,
+ should_decline1, should_decline2;
+ cas_saml_attr *attrs = NULL;
+ cas_attr_builder *builder;
+ require_line require_line_array[2];
+ cas_cfg *c;
+ // cas_dir_cfg *d;
+ int i;
+ require_line *r;
+
+ /* Manually create some SAML attributes. These attributes represent
pames Owner
pames added a note

nit: can you change this to use consistent comment formatting for the multiline comments throughout, i.e.:

/*

  • blah blah */

(github is screwing up the formatting, but hopefully you get the point)

Fixed, along with the other block comments in this function.

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

LGTM as well aside from a few minor nits.

Thanks again for contributing this.

Matt
Owner

Pushed additions to jasig:attz -- please take a look

Jonathan Daugherty

Hi, guys; what's the status of this merge?

Matt
Owner
Jonathan Daugherty

Hello, again; any updates on this?

Max Bowsher

I'm really interested in this feature, is there anything I can do to help it along?

Patrick O'Connor

| I'm really interested in this feature, is there anything I can do to help it along?
I second this

Matt forsetti merged commit 2fa882d into from
Matt
Owner

Merged -> master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 20, 2011
  1. Fix NULL dereference on "Require cas-attribute ..." when no attribute…

    Josh Hoyt authored
    …s are present
  2. Improvements to Christian Folini's attribute authorization patch

    Josh Hoyt authored
    <https://issues.jasig.org/browse/MAS-60>:
    
    * Use ap_getword_conf to tokenize the Require directive. This means
      that attribute specifications can use any character (including
      whitespace). For example:
    
        Require cas-attribute "last-name:de Soto"
    
    * Pass a reference to the struct of attributes that's parsed out of
      the SAML response in order to do authorization rather than using
      attributes serialized into a notes field. This eliminates ambiguity
      due to serialization, as well as being more efficient.
    
    * Factor out the routine that evaluates an attribute specification
      against the set of attributes. This improves readability as well as
      testability. Also, be explicit about exactly what will match an
      attribute specification, and the behavior in the face of ambiguity.
  3. Formatting and adding const where applicable

    Josh Hoyt authored
  4. Encapsulate the attribute storage into an API

    Josh Hoyt authored
Commits on Sep 21, 2011
  1. Fix the argument passed to ap_get/set_module_config in cas_get/set_at…

    Josh Hoyt authored
    …tributes
    
    The parameter was incorrectly set to the request_rec pointer itself
    rather than the request_config field of the request. Depending on
    Apache's configuration, this resulted in different parts of the
    request structure being overwritten with the attribute pointer. In
    many cases, it worked correctly by accident. This change correctly
    uses the request_config conf_vector for ap_get/set_module_config.
    
    In addition to that change, cas_get_attributes also works better in a
    subrequest: if no attributes are found in the subrequest, it will look
    in the parent request for attributes.
Commits on Dec 5, 2011
  1. Jonathan Daugherty
  2. Jonathan Daugherty

    Fix NULL dereference on "Require cas-attribute ..." when no attribute…

    Josh Hoyt authored jtdaugherty committed
    …s are present
Commits on Dec 6, 2011
  1. Jonathan Daugherty

    Improvements to Christian Folini's attribute authorization patch

    Josh Hoyt authored jtdaugherty committed
    <https://issues.jasig.org/browse/MAS-60>:
    
    * Use ap_getword_conf to tokenize the Require directive. This means
      that attribute specifications can use any character (including
      whitespace). For example:
    
        Require cas-attribute "last-name:de Soto"
    
    * Pass a reference to the struct of attributes that's parsed out of
      the SAML response in order to do authorization rather than using
      attributes serialized into a notes field. This eliminates ambiguity
      due to serialization, as well as being more efficient.
    
    * Factor out the routine that evaluates an attribute specification
      against the set of attributes. This improves readability as well as
      testability. Also, be explicit about exactly what will match an
      attribute specification, and the behavior in the face of ambiguity.
  2. Jonathan Daugherty

    Formatting and adding const where applicable

    Josh Hoyt authored jtdaugherty committed
  3. Jonathan Daugherty

    Encapsulate the attribute storage into an API

    Josh Hoyt authored jtdaugherty committed
  4. Jonathan Daugherty

    Fix the argument passed to ap_get/set_module_config in cas_get/set_at…

    Josh Hoyt authored jtdaugherty committed
    …tributes
    
    The parameter was incorrectly set to the request_rec pointer itself
    rather than the request_config field of the request. Depending on
    Apache's configuration, this resulted in different parts of the
    request structure being overwritten with the attribute pointer. In
    many cases, it worked correctly by accident. This change correctly
    uses the request_config conf_vector for ap_get/set_module_config.
    
    In addition to that change, cas_get_attributes also works better in a
    subrequest: if no attributes are found in the subrequest, it will look
    in the parent request for attributes.
Commits on Dec 23, 2011
  1. mod_auth_cas now logs a warning if Require cas-attribute is present i…

    Dylan McNamee authored
    …n the
    
    config file, but no actual attributes are specified.
    
    had to add <openssl/crypto.h> in includes to get module to compile on 64-bit
    system (regression testing needed - did I break 32-bit?)
    
    started adding hooks for unit testing cas-attribute parsing and enforcement
  2. Merge branch 'folini-authz' of https://github.com/jtdaugherty/mod_aut…

    Dylan McNamee authored
    …h_cas into folini-authz
    
    Conflicts:
    	README
    	src/mod_auth_cas.c
    	src/mod_auth_cas.h
Commits on Dec 28, 2011
  1. Unit test for cas_authorize. Cleanup of cas_saml_attr_test for warnin…

    Dylan McNamee authored
    …g-free GCC compiles.
    
    Had to pull in some attribute parsing code from apache to get authz unit tests to go through.
  2. removing warnings...

    Dylan McNamee authored
  3. fixing warnings.

    Dylan McNamee authored
  4. tweak to remove another warning

    Dylan McNamee authored
  5. removing one last debugging printf

    Dylan McNamee authored
Commits on Mar 1, 2012
  1. Jonathan Daugherty
  2. Jonathan Daugherty
  3. Jonathan Daugherty
  4. Jonathan Daugherty
  5. Jonathan Daugherty
Commits on May 8, 2012
  1. Jonathan Daugherty
  2. Jonathan Daugherty
Commits on May 10, 2012
  1. Jonathan Daugherty

    Typo

    jtdaugherty authored
Commits on May 11, 2012
  1. Jonathan Daugherty
  2. Jonathan Daugherty
  3. Jonathan Daugherty
This page is out of date. Refresh to see the latest.
15 README
View
@@ -281,6 +281,13 @@ Description: Set the optional 'HttpOnly' flag for cookies issues by mod_auth_cas
for more information. Please note this feature is not honored by all
browsers.
+Directive: CASAuthoritative
+Default: Off
+Description: This directive determines whether an optional authorization directive
+ (see 'Require cas-attribute') is authoritative and thus binding or
+ if other authorization modules will also be applied.
+ 'On' means authoritative, 'Off' means not authoritative.
+
Valid Directory/.htaccess Directives
------------------------------------
Directive: CASScope
@@ -363,6 +370,14 @@ Description: mod_auth_cas will strip request inbound request headers that may ha
special meaning, such as those set with the CASAttributePrefix or the
CASAuthNHeader value.
+Directive: Require cas-attribute <attribute>:<value>
+Default: NULL
+Description: Use this directive to authorize based on SAML cas attributes
+ returned via the session validation call. Multiple directives
+ are OR-ed. If directive is present with no attributes defined,
+ the request is declined. If value has spaces, wrap the pair in quotes.
+ See also CASAuthoritative.
+
====================================================================
CONTACT INFORMATION AND WEBSITE
====================================================================
244 src/mod_auth_cas.c
View
@@ -95,6 +95,7 @@ void *cas_create_server_config(apr_pool_t *pool, server_rec *svr)
c->CASValidateSAML = CAS_DEFAULT_VALIDATE_SAML;
c->CASAttributeDelimiter = CAS_DEFAULT_ATTRIBUTE_DELIMITER;
c->CASAttributePrefix = CAS_DEFAULT_ATTRIBUTE_PREFIX;
+ c->CASAuthoritative = CAS_DEFAULT_AUTHORITATIVE;
cas_setURL(pool, &(c->CASLoginURL), CAS_DEFAULT_LOGIN_URL);
cas_setURL(pool, &(c->CASValidateURL), CAS_DEFAULT_VALIDATE_URL);
@@ -128,6 +129,7 @@ void *cas_merge_server_config(apr_pool_t *pool, void *BASE, void *ADD)
c->CASCookieHttpOnly = (add->CASCookieHttpOnly != CAS_DEFAULT_COOKIE_HTTPONLY ? add->CASCookieHttpOnly : base->CASCookieHttpOnly);
c->CASSSOEnabled = (add->CASSSOEnabled != CAS_DEFAULT_SSO_ENABLED ? add->CASSSOEnabled : base->CASSSOEnabled);
c->CASValidateSAML = (add->CASValidateSAML != CAS_DEFAULT_VALIDATE_SAML ? add->CASValidateSAML : base->CASValidateSAML);
+ c->CASAuthoritative = (add->CASAuthoritative != CAS_DEFAULT_AUTHORITATIVE ? add->CASAuthoritative : base->CASAuthoritative);
c->CASAttributeDelimiter = (apr_strnatcasecmp(add->CASAttributeDelimiter, CAS_DEFAULT_ATTRIBUTE_DELIMITER) != 0 ? add->CASAttributeDelimiter : base->CASAttributeDelimiter);
c->CASAttributePrefix = (apr_strnatcasecmp(add->CASAttributePrefix, CAS_DEFAULT_ATTRIBUTE_PREFIX) != 0 ? add->CASAttributePrefix : base->CASAttributePrefix);
@@ -375,6 +377,14 @@ const char *cfg_readCASParameter(cmd_parms *cmd, void *cfg, const char *value)
else
return(apr_psprintf(cmd->pool, "MOD_AUTH_CAS: Invalid argument to CASSSOEnabled - must be 'On' or 'Off'"));
break;
+ case cmd_authoritative:
+ if(apr_strnatcasecmp(value, "On") == 0)
+ c->CASAuthoritative = TRUE;
+ else if(apr_strnatcasecmp(value, "Off") == 0)
+ c->CASAuthoritative = FALSE;
+ else
+ return(apr_psprintf(cmd->pool, "MOD_AUTH_CAS: Invalid argument to CASAuthoritative - must be 'On' or 'Off'"));
Ben Noordhuis Owner

I know you're not the one who introduced this pattern but this really should be a AP_INIT_FLAG directive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ break;
default:
/* should not happen */
return(apr_psprintf(cmd->pool, "MOD_AUTH_CAS: invalid command '%s'", cmd->directive->directive));
@@ -1985,6 +1995,8 @@ int cas_authenticate(request_rec *r)
remoteUser = NULL;
}
+ cas_set_attributes(r, attrs);
+
if(remoteUser) {
r->user = remoteUser;
if(d->CASAuthNHeader != NULL) {
@@ -2018,6 +2030,230 @@ int cas_authenticate(request_rec *r)
return HTTP_UNAUTHORIZED;
}
+/* Store a reference to the request's attributes for later use.
+ * Subsequent calls to cas_get_attributes() with the same request
+ * object will return this same set of attributes. Note that the
+ * attributes are stored directly, and not copied. In particular,
+ * beware that the attributes must live at least as long as the
+ * specified request. */
+void cas_set_attributes(request_rec *r, cas_saml_attr *const attrs) {
+ /* Always set the attributes in the current request, even if
+ * it is a subrequest, because we always allocate memory in
+ * the current request, so we run the risk of accessing freed
+ * memory if we were to set it in the main request. */
+ ap_set_module_config(r->request_config, &auth_cas_module, attrs);
+}
+
+/* Get a reference to the attributes that were previously stored for
+ * this request (or its main request). If no attributes have been
+ * stored, this function will return NULL.
+ */
+const cas_saml_attr *cas_get_attributes(request_rec *r) {
+ /* If we have attribute stored in this request, then use them. If
+ * not, then check the main request (if any). */
+ const cas_saml_attr *attrs = ap_get_module_config(r->request_config,
+ &auth_cas_module);
+ if (attrs == NULL && r->main != NULL) {
+ return cas_get_attributes(r->main);
+ } else {
+ return attrs;
+ }
+}
+
+/* Look for an attribute that matches the given attribute spec (e.g.
+ * from a Require directive)
+ *
+ * An attribute spec is a string containing an attribute name and an
+ * attribute value separated by a colon. This means that attribute
+ * specification strings containing more than one colon produce ambiguous
+ * specifications that match multiple attributes. For instance:
+ *
+ * spec = "foo:bar:baz"
+ *
+ * matches both:
+ *
+ * attr1 = "foo" "bar:baz"
+ * attr2 = "foo:bar" "baz"
+ *
+ * Attribute name matching is exact, and value matching has some
+ * leeway. Value matching uses apr_strnatcmp to determine equality, so
+ * whitespace is ignored and decimal numbers can have differing
+ * representations. See the documentation of apr_strnatcmp for
+ * details.
+ */
+int cas_match_attribute(const char *const attr_spec, const cas_saml_attr *const attributes, struct request_rec *r) {
+ const cas_saml_attr *attr = attributes;
+
+ /* Loop over all of the user attributes */
+ for ( ; attr; attr = attr->next ) {
+
+ const char *attr_c = attr->attr;
+ const char *spec_c = attr_spec;
+
+ /* Walk both strings until we get to the end of either or we
+ * find a differing character */
+ while ((*attr_c) &&
+ (*spec_c) &&
+ (*attr_c) == (*spec_c)) {
+ attr_c++;
+ spec_c++;
+ }
+
+ /* The match is a success if we walked the whole attribute
+ * name and the attr_spec is at a colon. */
+ if (!(*attr_c) && (*spec_c) == ':') {
+ const cas_saml_attr_val *val;
+
+ /* Skip the colon */
+ spec_c++;
+
+ /* Compare the attribute values */
+ val = attr->values;
+ for ( ; val; val = val->next ) {
+
+ /* Approximately compare the attribute value (ignoring
+ * whitespace). At this point, spec_c points to the
+ * NULL-terminated value pattern. */
+ if (apr_strnatcmp(val->value, spec_c) == 0) {
+ return CAS_ATTR_MATCH;
+ }
+ }
+ }
+ }
+ return CAS_ATTR_NO_MATCH;
+}
+
+
+/* CAS authorization module, code adopted from Nick Kew's Apache Modules Book, 2007, p. 190f */
+int cas_authorize(request_rec *r)
+{
+ const cas_saml_attr *const attrs = cas_get_attributes(r);
+
+ const apr_array_header_t *const reqs_arr = ap_requires(r);
+ const require_line *const reqs =
+ reqs_arr ? (require_line *) reqs_arr->elts : NULL;
+ const cas_cfg *const c =
+ ap_get_module_config(r->server->module_config,
+ &auth_cas_module);
+
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+ "Entering cas_authorize.");
+
+ if (!reqs_arr) {
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+ "No require statements found, "
+ "so declining to perform authorization.");
+ return DECLINED;
+ }
+
+ return (cas_authorize_worker(r, attrs, reqs, reqs_arr->nelts, c));
+}
+
+/* Pulled out from cas_authorize to enable unit-testing */
+
+int cas_authorize_worker(request_rec *r, const cas_saml_attr *const attrs, const require_line *const reqs, int nelts, const cas_cfg *const c)
+{
+ const int m = r->method_number;
+ const char *token;
+ const char *requirement;
+ int i;
+ int have_casattr = 0;
+ int count_casattr = 0;
+
+ // Q: why don't we use ap_some_auth_required here?? performance?
pames Owner
pames added a note

Any answer to this question? Seems like you might be duplicating some level of checks that ap_some_auth_required would run below.

Matt Owner
forsetti added a note

@jtdaugherty Please see notes from @pames

During development we encountered that function elsewhere and it looked like something we ought to use, but we aren't entirely sure if it's the right thing to do. Some advice would be good. If you aren't sure either, I'd say the comment is spurious (as the tests pass).

Ben Noordhuis Owner

You're duplicating core httpd functionality now. Using ap_some_auth_required() here is the right thing.

Can you help me understand what I need to replace? The docs on that function are pretty opaque and its implementation is just an indirection layer, so it's not clear to me what, precisely, is being duplicated.

Ben Noordhuis Owner
+    if (!(reqs[i].method_mask & (AP_METHOD_BIT << m))) {
+      continue;
+    }

That bit is lifted almost verbatim from the 2.2 ap_some_auth_required() (it hooks into the authz layer in 2.4).

Thanks for the clarification. The difference between ap_some_auth_required and the code in the loop mentioned here is that ap_some_auth_required returns true if there are any require lines in the request which match the method, whereas the loop is specifically checking to see whether each require should be considered on that basis. As a result we can't use ap_some_auth_required in the loop because it isn't specific enough, and if we move it outside the loop that will be fine except inside the loop we'll be considering require lines that we shouldn't be looking at, so the code that is there right now will still be required. Does that make sense?

Ben Noordhuis Owner

Does that make sense?

Yes, thanks for clarifying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ /* Go through applicable Require directives */
+ for (i = 0; i < nelts; ++i) {
+ /* Ignore this Require if it's in a <Limit> section
+ * that exclude this method
+ */
+
+ if (!(reqs[i].method_mask & (AP_METHOD_BIT << m))) {
+ continue;
+ }
+
+ /* ignore if it's not a "Require cas-attribute ..." */
+ requirement = reqs[i].requirement;
+
+ token = ap_getword_white(r->pool, &requirement);
+
+ if (apr_strnatcasecmp(token, "cas-attribute") != 0) {

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ continue;
+ }
+
+ /* OK, we have a "Require cas-attribute" to satisfy */
+ have_casattr = 1;
+
+ /* If we have an applicable cas-attribute, but no
+ * attributes were sent in the request, then we can
+ * just stop looking here, because it's not
+ * satisfiable. The code after this loop will give the
+ * appropriate response. */
+ if (!attrs) {
+ break;
+ }
+
+ /* Iterate over the attribute specification strings in this
+ * require directive searching for a specification that
+ * matches one of the attributes. */
+ while (*requirement) {
+ token = ap_getword_conf(r->pool, &requirement);
+ count_casattr++;
+
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+ "Evaluating attribute specification: %s",
+ token);
+
+ if (cas_match_attribute(token, attrs, r) ==
+ CAS_ATTR_MATCH) {
+
+ /* If *any* attribute matches, then
+ * authorization has succeeded and all
+ * of the others are ignored. */
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+ "Require cas-attribute "
+ "'%s' matched", token);
+ return OK;
+ }
+ }
+ }
+
+ /* If there weren't any "Require cas-attribute" directives,
+ * we're irrelevant.
+ */
+ if (!have_casattr) {
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+ "No cas-attribute statements found. "
+ "Not performing authZ.");
+ return DECLINED;
+ }
+ /* If there was a "Require cas-attribute", but no actual attributes,
+ * that's cause to warn the admin of an iffy configuration.
+ */
+ if (count_casattr == 0) {
+ ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
+ "'Require cas-attribute' missing specification(s) in configuration. Declining.");
+ return DECLINED;
+ }
+
+ /* If we're not authoritative, hand over to other authz modules */
+ if (!c->CASAuthoritative) {
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+ "Authorization failed, but we are not "
+ "authoritative, thus handing over to other "
+ "module(s).");
+ return DECLINED;
+ }
+
+ /* OK, our decision is final and binding */
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+ "Authorization denied for client session");
+
+ ap_note_auth_failure(r);
+
+ return HTTP_UNAUTHORIZED;
+}
+
#if (defined(OPENSSL_THREADS) && APR_HAS_THREADS)
/* shamelessly based on code from mod_ssl */
@@ -2236,8 +2472,13 @@ apr_status_t cas_in_filter(ap_filter_t *f, apr_bucket_brigade *bb, ap_input_mode
void cas_register_hooks(apr_pool_t *p)
{
+ /* make sure we run before mod_authz_user so that a "require valid-user"
+ * directive doesn't just automatically pass us. */
+ static const char *const authzSucc[] = { "mod_authz_user.c", NULL };
+
ap_hook_post_config(cas_post_config, NULL, NULL, APR_HOOK_LAST);
ap_hook_check_user_id(cas_authenticate, NULL, NULL, APR_HOOK_MIDDLE);
+ ap_hook_auth_checker(cas_authorize, NULL, authzSucc, APR_HOOK_MIDDLE);
ap_register_input_filter("CAS", cas_in_filter, NULL, AP_FTYPE_RESOURCE);
}
@@ -2279,6 +2520,9 @@ const command_rec cas_cmds [] = {
AP_INIT_TAKE1("CASCacheCleanInterval", cfg_readCASParameter, (void *) cmd_cache_interval, RSRC_CONF, "Amount of time (in seconds) between cache cleanups. This value is checked when a new local ticket is issued or when a ticket expires."),
AP_INIT_TAKE1("CASRootProxiedAs", cfg_readCASParameter, (void *) cmd_root_proxied_as, RSRC_CONF, "URL used to access the root of the virtual server (only needed when the server is proxied)"),
AP_INIT_TAKE1("CASScrubRequestHeaders", ap_set_string_slot, (void *) APR_OFFSETOF(cas_dir_cfg, CASScrubRequestHeaders), ACCESS_CONF, "Scrub CAS user name and SAML attribute headers from the user's request."),
+ /* authorization options */
+ AP_INIT_TAKE1("CASAuthoritative", cfg_readCASParameter, (void *) cmd_authoritative, RSRC_CONF, "Set 'On' to reject if access isn't allowed based on our rules; 'Off' (default) to allow checking against other modules too."),
+
AP_INIT_TAKE1(0, 0, 0, 0, 0)
};
22 src/mod_auth_cas.h
View
@@ -26,9 +26,11 @@
#define MOD_AUTH_CAS_H
#include <stddef.h>
+#include <http_core.h>
#include "ap_release.h"
#define OPENSSL_THREAD_DEFINES
+#include <openssl/crypto.h>
#include <openssl/opensslconf.h>
#include <openssl/crypto.h>
@@ -92,11 +94,15 @@
#define CAS_DEFAULT_AUTHN_HEADER NULL
#define CAS_DEFAULT_SCRUB_REQUEST_HEADERS NULL
#define CAS_DEFAULT_SSO_ENABLED FALSE
+#define CAS_DEFAULT_AUTHORITATIVE FALSE
-#define CAS_MAX_RESPONSE_SIZE 16384
+#define CAS_MAX_RESPONSE_SIZE 65536
Matt Owner

@jtdaugherty Why the increase from 16k to 64k? Did you find that you were producing large SAML messages?

@pames What would you think of a realloc loop dynamically resizing up to CAS_MAX_RESPONSE_SIZE ? We could start at (e.g.) 4k, resize to 64k if necessary, reducing memory overhead. Given that this is an HTTP read, I don't think the performance hit of realloc would be noticed.

pames Owner
pames added a note

Nice thing about keeping it in the stack frame is that it just gets allocated inherently. Simpler code overall, and we don't introduce any overhead to reallocate, etc. Plus if someone is shipping you more than, e.g. 16k of data then there's probably something very wrong (or you have a ridiculous amount of attributes).

I don't think the overhead would be much but I don't think the extra complexity really warrants it when we're talking about a fairly well defined response with a size limit we can pretty reasonably cap.

That change is from Christian Folini's original patch so I can't comment on the change. I can say, at least, that it was a change from 4k to 64k in the original patch. 4k does strike me as a bit small in any case, considering the messages involved, but 64k is overkill. I'm happy to revert this to 16k; I don't think that will hurt anything.

Matt Owner
pames Owner
pames added a note
Matt Owner

Can't find an authoritative spec - just a couple examples generally starting ~64K and allowing increase to 1M.

So -- I'd recommend we start with 16k, and if we have to go larger, I'll work up the realloc loop (+ cleanup) for dynamic growth to a larger max. Thoughts?

pames Owner
pames added a note

If everyone else uses 64k then it's fine to leave at 64k. It just seems excessive to me, particularly given the practical experience we have here.

Ben Noordhuis Owner

At my last job we had a kind of ersatz attribute management that hit the 16k limit with some users so I bumped it to 64k on-stack with no ill effect.

I suggest raising the limit and keeping it on-stack for simplicity's sake. A 64k stack frame is not a problem on modern systems (the ones people actually use) so I don't see a downside.

Ben Noordhuis Owner

We could also switch to SAX parsing with apr_xml_parser_feed + apr_xml_parser_done. No allocation - on-stack, heap or otherwise - required in m-a-c.

pames Owner
pames added a note

FWIW I like the simplicity of the current approach, and I agree with your assessment about modern stack sizes (for instance RLIMIT_STACK on my box is 8MB - depending on the Apache threading model maybe that value is significantly less per thread but I doubt it will be an issue in practice due to the nature of the function call in question).

In any case I think far too many man-hours have been expended debating this by now so let's just leave it at 64k :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
#define CAS_MAX_ERROR_SIZE 1024
#define CAS_MAX_XML_SIZE 1024
+#define CAS_ATTR_MATCH 0
+#define CAS_ATTR_NO_MATCH 1
+
#define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0]))
typedef struct cas_cfg {
@@ -113,6 +119,7 @@ typedef struct cas_cfg {
unsigned int CASIdleTimeout;
unsigned int CASCookieHttpOnly;
unsigned int CASSSOEnabled;
+ unsigned int CASAuthoritative;
unsigned int CASValidateSAML;
char *CASCertificatePath;
char *CASCookiePath;
@@ -156,7 +163,8 @@ typedef enum {
cmd_version, cmd_debug, cmd_validate_server, cmd_validate_depth, cmd_wildcard_cert,
cmd_ca_path, cmd_cookie_path, cmd_loginurl, cmd_validateurl, cmd_proxyurl, cmd_cookie_entropy,
cmd_session_timeout, cmd_idle_timeout, cmd_cache_interval, cmd_cookie_domain, cmd_cookie_httponly,
- cmd_sso, cmd_validate_saml, cmd_attribute_delimiter, cmd_attribute_prefix, cmd_root_proxied_as
+ cmd_sso, cmd_validate_saml, cmd_attribute_delimiter, cmd_attribute_prefix, cmd_root_proxied_as,
+ cmd_authoritative
} valid_cmds;
module AP_MODULE_DECLARE_DATA auth_cas_module;
@@ -205,7 +213,6 @@ void cas_ssl_id_callback(CRYPTO_THREADID *id);
int cas_post_config(apr_pool_t *pool, apr_pool_t *p1, apr_pool_t *p2, server_rec *s);
void cas_register_hooks(apr_pool_t *p);
-
char *getCASScope(request_rec *r);
void expireCASST(request_rec *r, const char *ticketname);
void cas_scrub_request_headers(request_rec *r, const cas_cfg *const c, const cas_dir_cfg *const d);
@@ -218,6 +225,15 @@ int merged_vhost_configs_exist(server_rec *s);
#if (defined(OPENSSL_THREADS) && APR_HAS_THREADS)
void cas_ssl_locking_callback(int mode, int type, const char *file, int line);
#endif
+/* Access per-request CAS SAML attributes */
+void cas_set_attributes(request_rec *r, cas_saml_attr *const attrs);
+const cas_saml_attr *cas_get_attributes(request_rec *r);
+int cas_match_attribute(const char *const attr_spec, const cas_saml_attr *const attributes, struct request_rec *r);
+
+/* Authorization check */
+int cas_authorize(request_rec *r);
+int cas_authorize_worker(request_rec *r, const cas_saml_attr *const attrs, const require_line *const reqs, int nelts, const cas_cfg *const c);
+
/* apr forward compatibility */
#ifndef APR_FOPEN_READ
109 tests/ap_stubs.c
View
@@ -5,6 +5,7 @@
#include "http_log.h"
#include "http_request.h"
#include "util_filter.h"
+#include <ctype.h>
#include "util_md5.h"
@@ -102,3 +103,111 @@ AP_DECLARE(int) ap_unescape_url(char *url) {
return 0;
}
+
+void ap_hook_auth_checker(int (*pf)(request_rec *),
+ const char * const *c1,
+ const char * const *c2,
+ int nOrder) {
+}
+
+/* Perhaps this is the top of a slippery slope, but pulling these in
+ * allowed us to test most of the authz functions fairly thoroughly.
+ */
+#define apr_isspace(c) (isspace(((unsigned char)(c))))
+
+AP_DECLARE(char *) ap_getword_white(apr_pool_t *p, const char **line) {
+
+ const char *pos = *line;
+ int len;
+ char *res;
+
+ while (!apr_isspace(*pos) && *pos) {
+ ++pos;
+ }
+
+ len = pos - *line;
+ res = calloc(1, len + 1);
+ memcpy(res, *line, len);
+ res[len] = 0;
+
+ while (apr_isspace(*pos)) {
+ ++pos;
+ }
+
+ *line = pos;
+
+ return res;
+}
+
+static char *substring_conf(apr_pool_t *p, const char *start, int len,
+ char quote)
+{
+ char *result = calloc(sizeof(char), len + 2);
+ char *resp = result;
+ int i;
+
+ for (i = 0; i < len; ++i) {
+ if (start[i] == '\\' && (start[i + 1] == '\\'
+ || (quote && start[i + 1] == quote)))
+ *resp++ = start[++i];
+ else
+ *resp++ = start[i];
+ }
+
+ *resp++ = '\0';
+ return result;
+}
+
+AP_DECLARE(char *) ap_getword_conf(apr_pool_t *p, const char **line) {
+
+ const char *str = *line, *strend;
+ char *res;
+ char quote;
+
+ while (*str && apr_isspace(*str))
+ ++str;
+
+ if (!*str) {
+ *line = str;
+ return "";
+ }
+
+ if ((quote = *str) == '"' || quote == '\'') {
+ strend = str + 1;
+ while (*strend && *strend != quote) {
+ if (*strend == '\\' && strend[1] &&
+ (strend[1] == quote || strend[1] == '\\')) {
+ strend += 2;
+ }
+ else {
+ ++strend;
+ }
+ }
+ res = substring_conf(p, str + 1, strend - str - 1, quote);
+
+ if (*strend == quote)
+ ++strend;
+ }
+ else {
+ strend = str;
+ while (*strend && !apr_isspace(*strend))
+ ++strend;
+
+ res = substring_conf(p, str, strend - str, 0);
+ }
+
+ while (*strend && apr_isspace(*strend))
+ ++strend;
+ *line = strend;
+ return res;
+}
+
+AP_DECLARE(void) ap_note_auth_failure(request_rec *r) {
+
+ return;
+}
+
+AP_DECLARE(const apr_array_header_t *) ap_requires(request_rec *r) {
+
+ return NULL;
+}
106 tests/mod_auth_cas_test.c
View
@@ -27,6 +27,16 @@ Suite *mod_auth_cas_suite(void);
request_rec *request;
apr_pool_t *pool;
+/* Function prototypes to make gcc happy */
+int find_entries_in_list(void *rec, const char *key, const char *val);
+char *get_attr(cas_cfg *c, cas_saml_attr *attrs, const char *attr);
+char *rand_str(apr_pool_t *p, unsigned int length_limit);
+void core_setup(void);
+void core_teardown(void);
+Suite *mod_auth_cas_suite(void);
+
+
+/* The tests */
START_TEST(cas_merge_server_config_test) {
cas_cfg *base = cas_create_server_config(request->pool, NULL);
cas_cfg *add = cas_create_server_config(request->pool, NULL);
@@ -751,14 +761,98 @@ START_TEST(cas_register_hooks_test) {
}
END_TEST
+START_TEST(cas_attribute_authz_test) {
+ int should_fail, should_succeed1, should_succeed2,
+ should_decline1, should_decline2;
+ cas_saml_attr *attrs = NULL;
+ cas_attr_builder *builder;
+ require_line require_line_array[2];
+ cas_cfg *c;
+ int i;
+ require_line *r;
+
+ /* Manually create some SAML attributes. These attributes represent
+ * a CAS attribute payload returned by CAS. This test will apply an
+ * authorization policy to these attributes to test its behavior.
+ */
+ struct test_data {
+ const char *const k;
+ const char *const v;
+ } test_data_list[] = {
+ {"key1", "val1"},
+ {"key1", "val2"},
+ {"key2", "val3"},
+ {"should", "succeed"},
+ {0, 0} /* NULL terminator */
+ };
+
+ // Build a CAS attribute structure.
+ builder = cas_attr_builder_new(pool, &attrs);
+ i = 0;
+ while (1) {
+ struct test_data d = test_data_list[i];
+ if (d.v == NULL) break;
+
+ cas_attr_builder_add(builder, d.k, d.v);
+ i++;
+ }
+
+ c = ap_get_module_config(request->server->module_config,
+ &auth_cas_module);
+
+ /* Create two 'Require' config structures representing the
+ * configured authorization policy. Although we create two, we'll
+ * apply different combinations of them in the tests which
+ * follow. */
+ r = &(require_line_array[0]);
+ r->method_mask = AP_METHOD_BIT;
+ r->requirement = apr_pstrdup(pool, "cas-attribute hopefully:fail");
+
+ r = &(require_line_array[1]);
+ r->method_mask = AP_METHOD_BIT;
+ r->requirement = apr_pstrdup(pool, "cas-attribute should:succeed");
+
+ /* When mod_auth_cas is authoritative, an attribute payload which
+ * fails to pass the policy check should result in
+ * HTTP_UNAUTHORIZED. */
+ c->CASAuthoritative = 1;
+ should_fail = cas_authorize_worker(request, attrs, &(require_line_array[0]), 1, c);
+
+ /* When mod_auth_cas is authoritative, an attribute payload which
+ * does pass the policy check should succeed. */
+ c->CASAuthoritative = 1;
+ should_succeed1 = cas_authorize_worker(request, attrs, &(require_line_array[1]), 1, c);
+
+ /* When mod_auth_cas is *not* authoritative, an attribute payload
+ * which does pass the policy check should succeed. */
+ c->CASAuthoritative = 0;
+ should_succeed2 = cas_authorize_worker(request, attrs, &(require_line_array[0]), 2, c);
+
+ /* Regardless of whether mod_auth_cas is authoritative, the empty
+ * list of Require directives means mod_auth_cas has no policy to
+ * check and should DECLINE. */
+ c->CASAuthoritative = 1;
+ should_decline1 = cas_authorize_worker(request, attrs, NULL, 0, c);
+ c->CASAuthoritative = 0;
+ should_decline2 = cas_authorize_worker(request, attrs, NULL, 0, c);
+
+ fail_unless((should_fail == HTTP_UNAUTHORIZED) &&
+ (should_succeed1 == OK) &&
+ (should_succeed2 == OK) &&
+ (should_decline1 == DECLINED) &&
+ (should_decline2 == DECLINED));
+}

Comments and more tests added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+END_TEST
+
/* Generate a null-terminated string of random bytes between one and
* length_limit characters */
char *rand_str(apr_pool_t *p, unsigned int length_limit) {
/* Generate a random length from one to length_limit, inclusive.
* This method for choosing a length is biased, but it should be
* fine for testing purposes. */
- char *ans;
unsigned int len;
+ char *ans;
+
if (length_limit < 1) {
len = 1;
} else {
@@ -798,7 +892,7 @@ START_TEST(cas_strnenvcmp_test) {
int l1 = strlen(rnd1);
int l2 = strlen(rnd2);
int l = l1 > l2 ? l1 : l2;
- int i, a, b;
+ int i;
/* Comparing zero characters yields equal, regardless of the other
* inputs. */
@@ -816,6 +910,7 @@ START_TEST(cas_strnenvcmp_test) {
/* For all lengths up to the length of the longer string */
for (i = 0; i <= l; i++) {
+ int a, b;
/* A string always compares equal to itself */
assert_snecmp_eqn(rnd1, rnd1, i);
@@ -877,7 +972,7 @@ START_TEST(cas_strnenvcmp_test) {
}
END_TEST
-void core_setup() {
+void core_setup(void) {
const unsigned int kIdx = 0;
const unsigned int kEls = kIdx + 1;
apr_uri_t login;
@@ -934,7 +1029,7 @@ void core_setup() {
ap_set_module_config(request->per_dir_config, &auth_cas_module, d_cfg);
}
-void core_teardown() {
+void core_teardown(void) {
// created by various cookie test functions above
apr_file_remove("/tmp/.metadata", request->pool);
apr_file_remove("/tmp/.md5", request->pool);
@@ -947,7 +1042,7 @@ void core_teardown() {
free(request);
}
-Suite *mod_auth_cas_suite() {
+Suite *mod_auth_cas_suite(void) {
Suite *s = suite_create("mod_auth_cas");
TCase *tc_core = tcase_create("core");
@@ -1001,6 +1096,7 @@ Suite *mod_auth_cas_suite() {
tcase_add_test(tc_core, cas_post_config_test);
tcase_add_test(tc_core, cas_in_filter_test);
tcase_add_test(tc_core, cas_register_hooks_test);
+ tcase_add_test(tc_core, cas_attribute_authz_test);
suite_add_tcase(s, tc_core);
suite_add_tcase(s, cas_saml_attr_tcase());
Something went wrong with that request. Please try again.