Skip to content

Refactor the OneLogin::RubySaml::Metadata class #602

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
pitbulk merged 6 commits into from
Aug 16, 2021
Merged

Refactor the OneLogin::RubySaml::Metadata class #602

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
25160fe
Fixes #433. Refactor the OneLogin::RubySaml::Metadata class so it is …
johnnyshields Aug 7, 2021
b0a8834
Add docs
johnnyshields Aug 7, 2021
a33e9d7
Fix metadata signature
johnnyshields Aug 9, 2021
b82fc60
SPSSODescriptor element is not selected properly
johnnyshields Aug 9, 2021
deaf112
Response to code review
johnnyshields Aug 12, 2021
cc295ea
Ensure sig is added as first child of root in XML and add tests
johnnyshields Aug 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -821,3 +821,27 @@ end
```

The `attribute_value` option additionally accepts an array of possible values.

## Custom Metadata Fields

Some IdPs may require to add SPs to add additional fields (Organization, ContactPerson, etc.)
into the SP metadata. This can be acheived by extending the `OneLogin::RubySaml::Metadata`
class and overriding the `#add_extras` method as per the following example:

```ruby
class MyMetadata < OneLogin::RubySaml::Metadata
def add_extras(root, _settings)
org = root.add_element("md:Organization")
org.add_element("md:OrganizationName", 'xml:lang' => "en-US").text = 'ACME Inc.'
org.add_element("md:OrganizationDisplayName", 'xml:lang' => "en-US").text = 'ACME'
org.add_element("md:OrganizationURL", 'xml:lang' => "en-US").text = 'https://www.acme.com'

cp = root.add_element("md:ContactPerson", 'contactType' => 'technical')
cp.add_element("md:GivenName").text = 'ACME SAML Team'
cp.add_element("md:EmailAddress").text = 'saml@acme.com'
end
end

# Output XML with custom metadata
MyMetadata.new.generate(settings)
```
79 changes: 57 additions & 22 deletions lib/onelogin/ruby-saml/metadata.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,50 @@ class Metadata
#
def generate(settings, pretty_print=false, valid_until=nil, cache_duration=nil)
meta_doc = XMLSecurity::Document.new
add_xml_declaration(meta_doc)
root = add_root_element(meta_doc, settings, valid_until, cache_duration)
sp_sso = add_sp_sso_element(root, settings)
add_sp_certificates(sp_sso, settings)
add_sp_service_elements(sp_sso, settings)
add_extras(root, settings)
embed_signature(meta_doc, settings)
output_xml(meta_doc, pretty_print)
end

protected

def add_xml_declaration(meta_doc)
meta_doc << REXML::XMLDecl.new('1.0', 'UTF-8')
end

def add_root_element(meta_doc, settings, valid_until, cache_duration)
namespaces = {
"xmlns:md" => "urn:oasis:names:tc:SAML:2.0:metadata"
}

if settings.attribute_consuming_service.configured?
namespaces["xmlns:saml"] = "urn:oasis:names:tc:SAML:2.0:assertion"
end
root = meta_doc.add_element "md:EntityDescriptor", namespaces
sp_sso = root.add_element "md:SPSSODescriptor", {

root = meta_doc.add_element("md:EntityDescriptor", namespaces)
root.attributes["ID"] = OneLogin::RubySaml::Utils.uuid
root.attributes["entityID"] = settings.sp_entity_id if settings.sp_entity_id
root.attributes["validUntil"] = valid_until.strftime('%Y-%m-%dT%H:%M:%S%z') if valid_until
root.attributes["cacheDuration"] = "PT" + cache_duration.to_s + "S" if cache_duration
root
end

def add_sp_sso_element(root, settings)
root.add_element "md:SPSSODescriptor", {
"protocolSupportEnumeration" => "urn:oasis:names:tc:SAML:2.0:protocol",
"AuthnRequestsSigned" => settings.security[:authn_requests_signed],
"WantAssertionsSigned" => settings.security[:want_assertions_signed],
}
end

# Add KeyDescriptor if messages will be signed / encrypted
# with SP certificate, and new SP certificate if any
# Add KeyDescriptor if messages will be signed / encrypted
# with SP certificate, and new SP certificate if any
def add_sp_certificates(sp_sso, settings)
cert = settings.get_sp_cert
cert_new = settings.get_sp_cert_new

Expand All @@ -58,27 +87,23 @@ def generate(settings, pretty_print=false, valid_until=nil, cache_duration=nil)
end
end

root.attributes["ID"] = OneLogin::RubySaml::Utils.uuid
if settings.sp_entity_id
root.attributes["entityID"] = settings.sp_entity_id
end
if valid_until
root.attributes["validUntil"] = valid_until.strftime('%Y-%m-%dT%H:%M:%S%z')
end
if cache_duration
root.attributes["cacheDuration"] = "PT" + cache_duration.to_s + "S"
end
sp_sso
end

def add_sp_service_elements(sp_sso, settings)
if settings.single_logout_service_url
sp_sso.add_element "md:SingleLogoutService", {
"Binding" => settings.single_logout_service_binding,
"Location" => settings.single_logout_service_url,
"ResponseLocation" => settings.single_logout_service_url
}
end

if settings.name_identifier_format
nameid = sp_sso.add_element "md:NameIDFormat"
nameid.text = settings.name_identifier_format
end

if settings.assertion_consumer_service_url
sp_sso.add_element "md:AssertionConsumerService", {
"Binding" => settings.assertion_consumer_service_binding,
Expand Down Expand Up @@ -117,23 +142,33 @@ def generate(settings, pretty_print=false, valid_until=nil, cache_duration=nil)
# <md:RoleDescriptor xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:query="urn:oasis:names:tc:SAML:metadata:ext:query" xsi:type="query:AttributeQueryDescriptorType" protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol"/>
# <md:XACMLAuthzDecisionQueryDescriptor WantAssertionsSigned="false" protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol"/>

meta_doc << REXML::XMLDecl.new("1.0", "UTF-8")
sp_sso
end

# embed signature
if settings.security[:metadata_signed] && settings.private_key && settings.certificate
private_key = settings.get_sp_key
meta_doc.sign_document(private_key, cert, settings.security[:signature_method], settings.security[:digest_method])
end
# can be overridden in subclass
def add_extras(root, _settings)
root
end

def embed_signature(meta_doc, settings)
return unless settings.security[:metadata_signed] && settings.private_key && settings.certificate

private_key = settings.get_sp_key
cert = settings.get_sp_cert
meta_doc.sign_document(private_key, cert, settings.security[:signature_method], settings.security[:digest_method])
Comment thread
johnnyshields marked this conversation as resolved.
end

def output_xml(meta_doc, pretty_print)
ret = ''

ret = ""
# pretty print the XML so IdP administrators can easily see what the SP supports
if pretty_print
meta_doc.write(ret, 1)
else
ret = meta_doc.to_s
end

return ret
ret
end
end
end
Expand Down
6 changes: 3 additions & 3 deletions lib/xml_security.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,10 +161,10 @@ def sign_document(private_key, certificate, signature_method = RSA_SHA1, digest_
# add the signature
issuer_element = self.elements["//saml:Issuer"]
if issuer_element
self.root.insert_after issuer_element, signature_element
self.root.insert_after(issuer_element, signature_element)
else
if sp_sso_descriptor = self.elements["/md:EntityDescriptor"]
self.root.insert_before sp_sso_descriptor, signature_element
if sp_sso_descriptor = self.elements["/md:EntityDescriptor/md:SPSSODescriptor"]
self.root.insert_before(sp_sso_descriptor, signature_element)
Copy link
Copy Markdown
Collaborator Author

@johnnyshields johnnyshields Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pitbulk the code as I have it here is correct with what you are saying. self.root is EntityDescriptor, so self.root.insert_before(sp_sso_descriptor, ...) means "Insert with EntityDescriptor as the parent, and before md:SPSSODescriptor as a sibling). I've tested it and it works correctly.

Here's the REXML documenation: https://ruby-doc.org/stdlib-2.5.1/libdoc/rexml/rdoc/REXML/Parent.html#method-i-insert_before

(It's a mystery that the previous code worked at all; it probably should have thrown an error.)

Copy link
Copy Markdown
Collaborator

@pitbulk pitbulk Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, in case of md:EntitiesDescriptor Signature needs to be added as first child which is covered later... I think you are right.... maybe we could add a unittest to verify where the signature is added on the different scenarios to assure it is not break in the future. Rather than that the PR seems ok, I just had a minus comment about a refactor. Good job.

Copy link
Copy Markdown
Collaborator Author

@johnnyshields johnnyshields Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case we should force it to always insert explicitly as the first child of md:EntitiesDescriptor. The way the code is now, it "happens to work" because md:SPSSODescriptor happens to be the first child. But if one's subclass were to do some crazypants custom modification of the xml in the add_extras method, this might no longer be the case.

Copy link
Copy Markdown
Collaborator Author

@johnnyshields johnnyshields Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an expert in REXML so I'll have to check the right way to do this. If you know offhand please let me know :)

Copy link
Copy Markdown
Collaborator

@pitbulk pitbulk Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an REXML expert neither ;) , that why makes sense the extra unittests

Copy link
Copy Markdown
Collaborator Author

@johnnyshields johnnyshields Aug 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit test added and ready for final review. Note that unit test includes validate_xml! which catches most of errors.

In a follow-up PR we should consider to have the Metadata class itself do the XSD validation, since we're now allowing users to add custom elements and the validation is quite strict.

else
self.root.add_element(signature_element)
end
Expand Down