From: Scott Cantor Date: Mon, 23 Feb 2009 21:52:59 +0000 (+0000) Subject: Add policy rules for SAML 1 SSO and SAML 2 Bearer confirmation, with unit tests. X-Git-Tag: 2.2.0~32 X-Git-Url: http://www.project-moonshot.org/gitweb/?p=shibboleth%2Fcpp-opensaml.git;a=commitdiff_plain;h=69a716dedfd9e239bcc9206a7b8dc137b43f5f89;hp=df39928338a40f7a2980406e9737893289673611 Add policy rules for SAML 1 SSO and SAML 2 Bearer confirmation, with unit tests. --- diff --git a/.cproject b/.cproject index b224a74..55b779c 100644 --- a/.cproject +++ b/.cproject @@ -63,6 +63,7 @@ + @@ -79,7 +80,7 @@ - + @@ -89,6 +90,7 @@ + @@ -96,6 +98,7 @@ + diff --git a/saml/Makefile.am b/saml/Makefile.am index eb19340..4d859d6 100644 --- a/saml/Makefile.am +++ b/saml/Makefile.am @@ -132,8 +132,9 @@ libsaml_la_SOURCES = \ saml1/binding/impl/SAML1SOAPDecoder.cpp \ saml1/binding/impl/SAML1SOAPEncoder.cpp \ saml1/binding/impl/SAML1SOAPClient.cpp \ - saml1/profile/AssertionValidator.cpp \ - saml1/profile/BrowserSSOProfileValidator.cpp \ + saml1/profile/impl/AssertionValidator.cpp \ + saml1/profile/impl/BrowserSSOProfileValidator.cpp \ + saml1/profile/impl/SAML1BrowserSSORule.cpp \ saml2/core/impl/Assertions.cpp \ saml2/core/impl/Assertions20Impl.cpp \ saml2/core/impl/Assertions20SchemaValidators.cpp \ @@ -166,9 +167,10 @@ libsaml_la_SOURCES = \ saml2/binding/impl/SAML2SOAPDecoder.cpp \ saml2/binding/impl/SAML2SOAPEncoder.cpp \ saml2/binding/impl/SAML2SOAPClient.cpp \ - saml2/profile/Assertion20Validator.cpp \ - saml2/profile/BrowserSSOProfile20Validator.cpp \ - saml2/profile/SAML2AssertionPolicy.cpp \ + saml2/profile/impl/Assertion20Validator.cpp \ + saml2/profile/impl/BrowserSSOProfile20Validator.cpp \ + saml2/profile/impl/BearerConfirmationRule.cpp \ + saml2/profile/impl/SAML2AssertionPolicy.cpp \ encryption/EncryptedKeyResolver.cpp \ signature/ContentReference.cpp \ signature/SignatureProfileValidator.cpp \ diff --git a/saml/binding/SecurityPolicy.h b/saml/binding/SecurityPolicy.h index 2dd16c2..6a5aac7 100644 --- a/saml/binding/SecurityPolicy.h +++ b/saml/binding/SecurityPolicy.h @@ -160,6 +160,16 @@ namespace opensaml { } /** + * Returns the message identifier to which the message being evaluated + * is a response. + * + * @return correlated message identifier + */ + const XMLCh* getCorrelationID() const { + return m_correlationID; + } + + /** * Gets a mutable array of installed policy rules. * *

If adding rules, their lifetime must be at least as long as the policy object. @@ -242,6 +252,16 @@ namespace opensaml { } /** + * Sets the message identifier to which the message being evaluated + * is a response. + * + * @param correlationID correlated message identifier + */ + void setCorrelationID(const XMLCh* correlationID) { + m_correlationID = correlationID; + } + + /** * Evaluates the policy against the given request and message, * possibly populating message information in the policy object. * @@ -450,6 +470,7 @@ namespace opensaml { // contextual information mutable time_t m_ts; + const XMLCh* m_correlationID; std::vector m_audiences; }; diff --git a/saml/binding/SecurityPolicyRule.h b/saml/binding/SecurityPolicyRule.h index 003e7ec..f4b746d 100644 --- a/saml/binding/SecurityPolicyRule.h +++ b/saml/binding/SecurityPolicyRule.h @@ -135,6 +135,24 @@ namespace opensaml { * over the message. The transport layer is not considered. */ #define XMLSIGNING_POLICY_RULE "XMLSigning" + + /** + * SecurityPolicyRule for SAML 1.x Browser SSO profile validation. + * + * Enforces presence of time conditions and proper subject confirmation. + */ + #define SAML1BROWSERSSO_POLICY_RULE "SAML1BrowserSSO" + + /** + * SecurityPolicyRule for SAML 2.0 bearer SubjectConfirmation. + * + *

Optionally enforces message delivery requirements based on SubjectConfirmationData. + * + *

The XML attributes "checkValidity", "checkRecipient", and "checkCorrelation" can be set + * "false" to disable checks of NotBefore/NotOnOrAfter, Recipient, and InResponseTo confirmation + * data respectively. + */ + #define BEARER_POLICY_RULE "Bearer" }; #endif /* __saml_secrule_h__ */ diff --git a/saml/binding/impl/SecurityPolicy.cpp b/saml/binding/impl/SecurityPolicy.cpp index bae5447..6603e3b 100644 --- a/saml/binding/impl/SecurityPolicy.cpp +++ b/saml/binding/impl/SecurityPolicy.cpp @@ -40,6 +40,14 @@ namespace opensaml { SAML_DLLLOCAL PluginManager::Factory NullSecurityRuleFactory; SAML_DLLLOCAL PluginManager::Factory SimpleSigningRuleFactory; SAML_DLLLOCAL PluginManager::Factory XMLSigningRuleFactory; + + namespace saml1 { + SAML_DLLLOCAL PluginManager::Factory BrowserSSORuleFactory; + } + + namespace saml2 { + SAML_DLLLOCAL PluginManager::Factory BearerConfirmationRuleFactory; + } }; void SAML_API opensaml::registerSecurityPolicyRules() @@ -53,6 +61,8 @@ void SAML_API opensaml::registerSecurityPolicyRules() conf.SecurityPolicyRuleManager.registerFactory(NULLSECURITY_POLICY_RULE, NullSecurityRuleFactory); conf.SecurityPolicyRuleManager.registerFactory(SIMPLESIGNING_POLICY_RULE, SimpleSigningRuleFactory); conf.SecurityPolicyRuleManager.registerFactory(XMLSIGNING_POLICY_RULE, XMLSigningRuleFactory); + conf.SecurityPolicyRuleManager.registerFactory(SAML1BROWSERSSO_POLICY_RULE, saml1::BrowserSSORuleFactory); + conf.SecurityPolicyRuleManager.registerFactory(BEARER_POLICY_RULE, saml2::BearerConfirmationRuleFactory); } SecurityPolicy::IssuerMatchingPolicy SecurityPolicy::m_defaultMatching; @@ -74,7 +84,8 @@ SecurityPolicy::SecurityPolicy( m_trust(trustEngine), m_validate(validate), m_entityOnly(true), - m_ts(0) + m_ts(0), + m_correlationID(NULL) { if (role) m_role = new xmltooling::QName(*role); diff --git a/saml/saml.vcproj b/saml/saml.vcproj index ab67f1f..58783f5 100644 --- a/saml/saml.vcproj +++ b/saml/saml.vcproj @@ -436,14 +436,22 @@ - - - - + + + + + + + - - - - - - - - - - - - - - - - - - - + - - - + - - - + - - - - - + + & attr = assertion.getAttributeStatements(); for_each(attr.begin(), attr.end(), _checkMethod()); - const vector& sub = assertion.getSubjectStatements(); - for_each(sub.begin(), sub.end(), _checkMethod()); // Pass up for additional checking. AssertionValidator::validateAssertion(assertion); diff --git a/saml/saml1/profile/impl/SAML1BrowserSSORule.cpp b/saml/saml1/profile/impl/SAML1BrowserSSORule.cpp new file mode 100644 index 0000000..1b1cc4f --- /dev/null +++ b/saml/saml1/profile/impl/SAML1BrowserSSORule.cpp @@ -0,0 +1,101 @@ +/* + * Copyright 2009 Internet2 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * SAML1BrowserSSORule.cpp + * + * SAML 1.x Browser SSO Profile SecurityPolicyRule + */ + +#include "internal.h" +#include "exceptions.h" +#include "binding/SecurityPolicyRule.h" +#include "saml1/core/Assertions.h" + +#include + +using namespace opensaml::saml1; +using namespace xmltooling::logging; +using namespace xmltooling; +using namespace std; + +namespace opensaml { + namespace saml1 { + + class SAML_DLLLOCAL BrowserSSORule : public opensaml::SecurityPolicyRule + { + public: + BrowserSSORule(const DOMElement* e) {} + + virtual ~BrowserSSORule() { + } + const char* getType() const { + return SAML1BROWSERSSO_POLICY_RULE; + } + bool evaluate(const XMLObject& message, const GenericRequest* request, opensaml::SecurityPolicy& policy) const; + }; + + opensaml::SecurityPolicyRule* SAML_DLLLOCAL BrowserSSORuleFactory(const DOMElement* const & e) + { + return new BrowserSSORule(e); + } + + class SAML_DLLLOCAL _checkMethod : public unary_function, + public unary_function + { + public: + void operator()(const SubjectStatement* s) const { + const Subject* sub = s->getSubject(); + if (s) { + const SubjectConfirmation* sc = sub->getSubjectConfirmation(); + if (sc) { + const vector& methods = sc->getConfirmationMethods(); + if (find_if(methods.begin(), methods.end(), _checkMethod())!=methods.end()) + return; // methods checked out + } + } + throw SecurityPolicyException("Assertion contained a statement without a supported ConfirmationMethod."); + } + + bool operator()(const ConfirmationMethod* cm) const { + const XMLCh* m = cm->getMethod(); + return (XMLString::equals(m,SubjectConfirmation::BEARER) || + XMLString::equals(m,SubjectConfirmation::ARTIFACT) || + XMLString::equals(m,SubjectConfirmation::ARTIFACT01)); + } + }; + }; +}; + +bool BrowserSSORule::evaluate(const XMLObject& message, const GenericRequest* request, opensaml::SecurityPolicy& policy) const +{ + const Assertion* a=dynamic_cast(&message); + if (!a) + return false; + + // Make sure the assertion is bounded. + const Conditions* conds = a->getConditions(); + if (!conds || !conds->getNotBefore() || !conds->getNotOnOrAfter()) + throw SecurityPolicyException("Browser SSO assertions MUST contain NotBefore/NotOnOrAfter attributes."); + + // Each statement MUST have proper confirmation requirements. + const vector& authn = a->getAuthenticationStatements(); + for_each(authn.begin(), authn.end(), _checkMethod()); + const vector& attr = a->getAttributeStatements(); + for_each(attr.begin(), attr.end(), _checkMethod()); + + return true; +} diff --git a/saml/saml2/profile/BrowserSSOProfileValidator.h b/saml/saml2/profile/BrowserSSOProfileValidator.h index 460c99d..9af864c 100644 --- a/saml/saml2/profile/BrowserSSOProfileValidator.h +++ b/saml/saml2/profile/BrowserSSOProfileValidator.h @@ -34,8 +34,7 @@ namespace opensaml { * SAML 2.0 Browser SSO Profile Assertion Validator * *

In addition to standard core requirements for validity, SSO assertions - * MUST have NotBefore/NotOnOrAfter attributes and each subject statement - * MUST be confirmable via bearer method. + * MUST be bearer-confirmable. */ class SAML_API BrowserSSOProfileValidator : public AssertionValidator { diff --git a/saml/saml2/profile/Assertion20Validator.cpp b/saml/saml2/profile/impl/Assertion20Validator.cpp similarity index 100% rename from saml/saml2/profile/Assertion20Validator.cpp rename to saml/saml2/profile/impl/Assertion20Validator.cpp diff --git a/saml/saml2/profile/impl/BearerConfirmationRule.cpp b/saml/saml2/profile/impl/BearerConfirmationRule.cpp new file mode 100644 index 0000000..b197226 --- /dev/null +++ b/saml/saml2/profile/impl/BearerConfirmationRule.cpp @@ -0,0 +1,144 @@ +/* + * Copyright 2009 Internet2 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * BearerConfirmationRule.cpp + * + * SAML 2.0 Bearer SubjectConfirmation SecurityPolicyRule + */ + +#include "internal.h" +#include "exceptions.h" +#include "binding/SecurityPolicyRule.h" +#include "saml2/core/Assertions.h" +#include "saml2/profile/SAML2AssertionPolicy.h" + +#include +#include + +using namespace opensaml::saml2; +using namespace xmltooling; +using namespace std; + +namespace opensaml { + namespace saml2 { + + class SAML_DLLLOCAL BearerConfirmationRule : public opensaml::SecurityPolicyRule + { + public: + BearerConfirmationRule(const DOMElement* e); + + virtual ~BearerConfirmationRule() { + } + const char* getType() const { + return BEARER_POLICY_RULE; + } + bool evaluate(const XMLObject& message, const GenericRequest* request, opensaml::SecurityPolicy& policy) const; + + private: + bool m_validity, m_recipient, m_correlation, m_fatal; + }; + + opensaml::SecurityPolicyRule* SAML_DLLLOCAL BearerConfirmationRuleFactory(const DOMElement* const & e) + { + return new BearerConfirmationRule(e); + } + + static const XMLCh checkValidity[] = UNICODE_LITERAL_13(c,h,e,c,k,V,a,l,i,d,i,t,y); + static const XMLCh checkRecipient[] = UNICODE_LITERAL_14(c,h,e,c,k,R,e,c,i,p,i,e,n,t); + static const XMLCh checkCorrelation[] = UNICODE_LITERAL_16(c,h,e,c,k,C,o,r,r,e,l,a,t,i,o,n); + static const XMLCh missingFatal[] = UNICODE_LITERAL_12(m,i,s,s,i,n,g,F,a,t,a,l); + }; +}; + +BearerConfirmationRule::BearerConfirmationRule(const DOMElement* e) : m_validity(true), m_recipient(true), m_correlation(true), m_fatal(true) +{ + const XMLCh* flag = e ? e->getAttributeNS(NULL, checkValidity) : NULL; + m_validity = (!flag || (*flag != chLatin_f && *flag != chDigit_0)); + flag = e ? e->getAttributeNS(NULL, checkRecipient) : NULL; + m_recipient = (!flag || (*flag != chLatin_f && *flag != chDigit_0)); + flag = e ? e->getAttributeNS(NULL, checkCorrelation) : NULL; + m_correlation = (!flag || (*flag != chLatin_f && *flag != chDigit_0)); + flag = e ? e->getAttributeNS(NULL, missingFatal) : NULL; + m_fatal = (!flag || (*flag != chLatin_f && *flag != chDigit_0)); +} + +bool BearerConfirmationRule::evaluate(const XMLObject& message, const GenericRequest* request, opensaml::SecurityPolicy& policy) const +{ + const Assertion* a=dynamic_cast(&message); + if (!a) + return false; + + logging::Category& log = logging::Category::getInstance(SAML_LOGCAT".SecurityPolicyRule.BearerConfirmation"); + + const char* msg=NULL; + const Subject* subject = a->getSubject(); + if (subject) { + const vector& confs = subject->getSubjectConfirmations(); + for (vector::const_iterator sc = confs.begin(); sc!=confs.end(); ++sc) { + if (XMLString::equals((*sc)->getMethod(), SubjectConfirmation::BEARER)) { + + const SubjectConfirmationDataType* data = dynamic_cast((*sc)->getSubjectConfirmationData()); + + if (m_recipient) { + const HTTPRequest* httpRequest = dynamic_cast(request); + if (httpRequest && httpRequest->getRequestURL()) { + string dest = httpRequest->getRequestURL(); + auto_ptr_XMLCh destination(dest.substr(0,dest.find('?')).c_str()); + if (!XMLString::equals(destination.get(), data ? data->getRecipient() : NULL)) { + msg = "bearer confirmation failed with recipient mismatch"; + continue; + } + } + } + + if (m_correlation && policy.getCorrelationID()) { + if (!XMLString::equals(policy.getCorrelationID(), data ? data->getInResponseTo() : NULL)) { + msg = "bearer confirmation failed with request correlation mismatch"; + continue; + } + } + + if (m_validity) { + if (!data || !data->getNotOnOrAfter()) { + msg = "bearer SubjectConfirmationData missing NotOnOrAfter attribute"; + continue; + } + else if (data->getNotOnOrAfterEpoch() <= policy.getTime() - XMLToolingConfig::getConfig().clock_skew_secs) { + msg = "bearer confirmation has expired"; + continue; + } + + if (data && data->getNotBefore() && policy.getTime() + XMLToolingConfig::getConfig().clock_skew_secs < data->getNotBeforeEpoch()) { + msg = "bearer confirmation not yet valid"; + continue; + } + } + + SAML2AssertionPolicy* saml2policy = dynamic_cast(&policy); + if (saml2policy) + saml2policy->setSubjectConfirmation(*sc); + log.debug("assertion satisfied bearer confirmation requirements"); + return true; + } + } + } + + log.error(msg); + if (m_fatal) + throw SecurityPolicyException("Unable to locate satisfiable bearer SubjectConfirmation in assertion."); + return false; +} diff --git a/saml/saml2/profile/BrowserSSOProfile20Validator.cpp b/saml/saml2/profile/impl/BrowserSSOProfile20Validator.cpp similarity index 100% rename from saml/saml2/profile/BrowserSSOProfile20Validator.cpp rename to saml/saml2/profile/impl/BrowserSSOProfile20Validator.cpp diff --git a/saml/saml2/profile/SAML2AssertionPolicy.cpp b/saml/saml2/profile/impl/SAML2AssertionPolicy.cpp similarity index 100% rename from saml/saml2/profile/SAML2AssertionPolicy.cpp rename to saml/saml2/profile/impl/SAML2AssertionPolicy.cpp diff --git a/samltest/data/saml1/profile/SAML1Assertion.xml b/samltest/data/saml1/profile/SAML1Assertion.xml index 01d7d96..81fc080 100644 --- a/samltest/data/saml1/profile/SAML1Assertion.xml +++ b/samltest/data/saml1/profile/SAML1Assertion.xml @@ -7,6 +7,11 @@ IssueInstant="1970-01-02T01:01:02.100Z" Issuer="https://idp.example.org/" MajorV - John Doe + + John Doe + + urn:oasis:names:tc:SAML:1.0:cm:bearer + + diff --git a/samltest/data/saml2/profile/SAML2Assertion.xml b/samltest/data/saml2/profile/SAML2Assertion.xml index 758e79e..e423ec1 100644 --- a/samltest/data/saml2/profile/SAML2Assertion.xml +++ b/samltest/data/saml2/profile/SAML2Assertion.xml @@ -1,6 +1,11 @@ https://idp.example.org/ - John Doe + + John Doe + + + + https://sp.example.org diff --git a/samltest/saml1/profile/SAML1PolicyTest.h b/samltest/saml1/profile/SAML1PolicyTest.h index 49554e5..3ea4593 100644 --- a/samltest/saml1/profile/SAML1PolicyTest.h +++ b/samltest/saml1/profile/SAML1PolicyTest.h @@ -24,18 +24,18 @@ using namespace opensaml; class SAML1PolicyTest : public CxxTest::TestSuite { SecurityPolicy* m_policy; - SecurityPolicyRule* m_rule; + vector m_rules; public: void setUp() { m_policy = NULL; - m_rule = NULL; - m_rule = SAMLConfig::getConfig().SecurityPolicyRuleManager.newPlugin(CONDITIONS_POLICY_RULE, NULL); + m_rules.push_back(SAMLConfig::getConfig().SecurityPolicyRuleManager.newPlugin(CONDITIONS_POLICY_RULE, NULL)); + m_rules.push_back(SAMLConfig::getConfig().SecurityPolicyRuleManager.newPlugin(SAML1BROWSERSSO_POLICY_RULE, NULL)); m_policy = new SecurityPolicy(); - m_policy->getRules().push_back(m_rule); + m_policy->getRules().assign(m_rules.begin(), m_rules.end()); } void tearDown() { - delete m_rule; + for_each(m_rules.begin(), m_rules.end(), xmltooling::cleanup()); delete m_policy; } diff --git a/samltest/saml2/profile/SAML2PolicyTest.h b/samltest/saml2/profile/SAML2PolicyTest.h index 3f3b958..c5b80e0 100644 --- a/samltest/saml2/profile/SAML2PolicyTest.h +++ b/samltest/saml2/profile/SAML2PolicyTest.h @@ -24,18 +24,18 @@ using namespace opensaml; class SAML2PolicyTest : public CxxTest::TestSuite { SecurityPolicy* m_policy; - SecurityPolicyRule* m_rule; + vector m_rules; public: void setUp() { m_policy = NULL; - m_rule = NULL; - m_rule = SAMLConfig::getConfig().SecurityPolicyRuleManager.newPlugin(CONDITIONS_POLICY_RULE, NULL); + m_rules.push_back(SAMLConfig::getConfig().SecurityPolicyRuleManager.newPlugin(CONDITIONS_POLICY_RULE, NULL)); + m_rules.push_back(SAMLConfig::getConfig().SecurityPolicyRuleManager.newPlugin(BEARER_POLICY_RULE, NULL)); m_policy = new SecurityPolicy(); - m_policy->getRules().push_back(m_rule); + m_policy->getRules().assign(m_rules.begin(), m_rules.end()); } void tearDown() { - delete m_rule; + for_each(m_rules.begin(), m_rules.end(), xmltooling::cleanup()); delete m_policy; } @@ -51,10 +51,18 @@ public: ); janitor.release(); + auto_ptr_XMLCh requestID("_12345"); + m_policy->setCorrelationID(requestID.get()); + TSM_ASSERT_THROWS("Policy should have tripped on AudienceRestriction", m_policy->evaluate(*assertion.get()), SecurityPolicyException); auto_ptr_XMLCh recipient("https://sp.example.org"); m_policy->getAudiences().push_back(recipient.get()); + TSM_ASSERT_THROWS("Policy should have tripped on InResponseTo correlation", m_policy->evaluate(*assertion.get()), SecurityPolicyException); + + dynamic_cast( + assertion->getSubject()->getSubjectConfirmations().front()->getSubjectConfirmationData() + )->setInResponseTo(requestID.get()); m_policy->evaluate(*assertion.get()); } catch (exception& ex) {