Closed GoogleCodeExporter closed 9 years ago
Previous file wasn't viewable.
Original comment by schofi...@terena.org
on 6 Nov 2013 at 9:37
Deleted to avoid confusion. Don't know why "samlparser-r3288.patch" doesn't
generate a view link.
Original comment by schofi...@terena.org
on 6 Nov 2013 at 9:39
Improved documentation.
Original comment by schofi...@terena.org
on 6 Nov 2013 at 1:26
config-attributelimit.php examples are commented by default.
Original comment by schofi...@terena.org
on 6 Nov 2013 at 3:45
An updated reference to the page on EntityAttributes.
Original comment by schofi...@terena.org
on 6 Nov 2013 at 4:21
Newest version of the patchh + config-templates/config-attributelimit.php for
your consideration.
Original comment by schofi...@terena.org
on 11 Nov 2013 at 1:36
Original comment by schofi...@terena.org
on 11 Nov 2013 at 1:39
Removed "continue" option for rules.
Original comment by schofi...@terena.org
on 11 Nov 2013 at 2:00
Attachments:
Updated configuration file.
Original comment by schofi...@terena.org
on 11 Nov 2013 at 2:01
Attachments:
Hi Brook,
Thanks for the work :-)
First, a couple of general comments on your suggested implementation:
* We think that this shouldn't be part of the AttributeLimit. AttributeLimit is
a very simple filter right now, and we would like it to continue like that.
What you want to achieve is going a bit further than just limiting the
attributes received by an SP, but defining and enforcing attribute policies
instead. I would suggest then creating a new processing filter (the core module
is fine, I think) named AttributeReleasePolicy, for instance.
* We find the configuration a bit complicated, and prone to making
configuration errors. Our idea is to do it as simple as possible, so the
configuration would be sequential (the policies are evaluated sequentially,
starting from the first one defined) and one-match (that is, whenever a policy
matches the current context, enforce it and stop evaluation), This way it is
really simple to follow and understand how the configuration works. No possible
loops, no branches, no additional conditions apart from "does this policy match
or not?". When a policy matches, the outcome is a set of attributes allowed by
that policy. That set is intersected with the set of attributes explicitly
requested by the SP (in the metadata), and the result of the intersection is
the set of attributes that are actually sent. Here is an example of such a
configuration:
---8<---
$config = array(
// The configuration is an array of policies evaluated sequentially
array( // policy #1
// Match only the following entity ID. This is a policy for one specific SP.
// Specific policies must be defined FIRST.
'entityID' => 'https://sp.example.org/',
// All attributes are allowed by policy. Can still be limited by what attributes the SP actually wants.
'attributes' => NULL,
// Set to TRUE to release only the attributes that have isRequired="true".
// Defaults to FALSE
'release_only_required' => TRUE,
// Set to TRUE to release all allowed attributes if the SP doesn't request any specific attributes in its metadata.
// Defaults to FALSE
'release_all_if_none_requested' => TRUE,
),
array( // policy #2
// Allowed RegistrationAuthority value. If it is an array, all values must match, so don't.
// If you want an OR, define independent policies for each RegistrationAuthority.
'RegistrationAuthority' => 'http://www.swamid.se/',
// Required EntityCategory values. ALL values must match.
// If you want an OR, define independent policies for each EntityCategory.
'EntityCategory' => array(
'http://www.swamid.se/category/nren-service',
'http://www.swamid.se/category/research-and-education',
),
// List of attributes the SPs are allowed to receive by policy.
'attributes' => array(
'eduPersonPrincipalName',
'displayName',
),
// Set to TRUE to release only the attributes that have isRequired="true".
// Defaults to FALSE
'release_only_required' => TRUE,
// Set to TRUE to release all allowed attributes if the SP doesn't request any specific attributes in its metadata.
// Defaults to FALSE
'release_all_if_none_requested' => TRUE,
),
// Default policy -- don't release any attributes.
array(
// No matching rule, so it matches every SP.
// No attributes are allowed by policy.
'attributes' => array(),
),
);
--->8---
Some important aspects to keep in mind with this configuration:
* It's a good idea to try to be coherent and once you go for a specific way to
do things, keep it. Therefore, all matching-rules defined in a policy must
match in order for the policy to match (AND). If a policy defines an entityID,
a RegistrationAuthority and several values in EntityCategory, all of them must
match. Each individual rule must behave the same way, then (AND). So if you
define a policy with several values in EntityCategory, all of them must match.
Same for RegistrationAuthority (which probably doesn't make sense, so in that
case it'd be a good idea to keep it as a string). Same for entityID (which
doesn't make any sense at all, so keep it as a string).
* Be coherent also with how the rest of SSP is configured. The entity ID should
be "entityID" therefore. The attributes option should be all lowercase. Both
EntityCategory and RegistrationAuthority should be capitalized to match the
SAML naming.
* Configure the attributes the same way SSP does in other places. If you want
to allow ALL in a policy, set 'attributes' equal to NULL (no filter). If you
want to allow NO attributes, set it to an empty array.
* Inside the 'attributes' option should only be attributes. Any flags should be
in the policy therefore.
* Avoid indexing the array. SSP does it in many places, true, and that's a bad
practice that we would like to remove. Using indexes allows you to configure it
in a way were the order in the configuration file does not match the order
followed when evaluating the rules, by doing something like: 20 => array(), 10
=> array(), 30 => array(). This is hard to follow and prone to
misconfigurations, so it's better if we stop doing that.
Now, apart from that, a few comments on the code itself:
* You have quite a few nested ifs in the code, many of them having an else
condition that stops the current execution path (i.e. a "continue" statement).
Example:
---8<---
if (some condition) {
if (some other condition) {
do something
} else {
continue;
}
} else {
continue;
}
--->8---
As a rule of thumb, it's much better if you invert the expression being
evaluated in the condition, so that you don't have to nest ifs and you can
write the code linearly. Example:
---8<---
if (not some condition) { // outermost condition goes first
continue;
}
if (not some other condition) { // then the next inner condition, and so on
continue;
}
do something
--->8----
This is easier to follow, as you can forget about nesting (you don't need to
remember all the conditions that you have passed up until that point), and also
the code is cleaner as you remove a huge amount of indents.
* In modules/saml/www/sp/metadata.php, you wrap the statement where you get the
'attributes.required' option with an if clause, so that in the end you don't
need to unset it from $metaArray20 if it was not set in the first place. It is
better to wrap the latter, rather than the former. The reason for this is the
getArray() was originally called with an empty array as a default value. This
means if 'attributes.required' is not set, getArray() will return the empty
array and that's what you'll get in $metaArray20['attributes.required']. This
is a guarantee for the rest of the code: you can forget about it being set or
not, it will always be set. You just need to see the contents (as you would do
anyway). So no possible errors when accessing an undefined index with no
wrapping code around the statement accessing the hashed array. It should read:
---8<---
$metaArray20['attributes.required'] =
$spconfig->getArray('attributes.required', array());
[...]
if (empty($metaArray20['attributes.required'])) {
unset($metaArray20['attributes.required']);
}
--->8----
* Finally, if you have an if statement with a condition so complex that you
need to add a multi-line comment before the if to explain when does it take the
if or the else branch, then it is probably poorly written. In that case it's
usually much better to split it into several nested ifs (keeping in mind that
stopping statements should go first). It is also a common practice to avoid
negated conditions, as they are difficult to read when they are more complex
than a simple !empty($array).
Hope our comments help :-)
Original comment by jaim...@gmail.com
on 22 Nov 2013 at 10:52
Hi Brook!
Any news on this?
Happy new year, btw :-)
Original comment by jaim...@gmail.com
on 6 Jan 2014 at 8:17
Original comment by jaim...@gmail.com
on 26 Feb 2014 at 2:25
Closing the issue here, moved to:
https://github.com/simplesamlphp/simplesamlphp/issues/49
Original comment by jaim...@gmail.com
on 27 Feb 2014 at 7:21
Original issue reported on code.google.com by
schofi...@terena.org
on 6 Nov 2013 at 9:28