-
-
Notifications
You must be signed in to change notification settings - Fork 569
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.
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
Add decrypt support. #241
Add decrypt support. #241
Changes from 6 commits
eaa3a58
629b421
bfd0d2d
50bd166
3867506
b164dd7
5b7b7c6
5a2a943
8d874ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ class Response < SamlMessage | |
ASSERTION = "urn:oasis:names:tc:SAML:2.0:assertion" | ||
PROTOCOL = "urn:oasis:names:tc:SAML:2.0:protocol" | ||
DSIG = "http://www.w3.org/2000/09/xmldsig#" | ||
XENC = "http://www.w3.org/2001/04/xmlenc#" | ||
|
||
# TODO: Settings should probably be initialized too... WDYT? | ||
|
||
|
@@ -24,6 +25,7 @@ class Response < SamlMessage | |
attr_accessor :errors | ||
|
||
attr_reader :document | ||
attr_reader :decrypted_document | ||
attr_reader :response | ||
attr_reader :options | ||
|
||
|
@@ -39,7 +41,7 @@ def initialize(response, options = {}) | |
@errors = [] | ||
|
||
raise ArgumentError.new("Response cannot be nil") if response.nil? | ||
@options = options | ||
@options = options | ||
|
||
@soft = true | ||
if !options.empty? && !options[:settings].nil? | ||
|
@@ -51,6 +53,21 @@ def initialize(response, options = {}) | |
|
||
@response = decode_raw_saml(response) | ||
@document = XMLSecurity::SignedDocument.new(@response, @errors) | ||
|
||
return unless assertion_encrypted? | ||
|
||
if @settings.nil? || [email protected]_sp_key | ||
validation_error('An EncryptedAssertion found and no SP private key found on the settings to decrypt it. Be sure you provided the :settings parameter at the initialize method') | ||
end | ||
|
||
# Marshal at Ruby 1.8.7 throw an Exception | ||
if RUBY_VERSION < "1.9" | ||
document_copy = XMLSecurity::SignedDocument.new(@response, @errors) | ||
else | ||
document_copy = Marshal.load(Marshal.dump(@document)) | ||
end | ||
|
||
@decrypted_document = decrypt_assertion_from_document(document_copy) | ||
end | ||
|
||
# Append the cause to the errors array, and based on the value of soft, return false or raise | ||
|
@@ -76,7 +93,12 @@ def is_valid? | |
# | ||
def name_id | ||
@name_id ||= begin | ||
node = xpath_first_from_signed_assertion('/a:Subject/a:NameID') | ||
enc_node = xpath_first_from_signed_assertion('/a:Subject/a:EncryptedID') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would complete the name to make it more readable. encrypted_node |
||
if enc_node | ||
node = decrypt_nameid(enc_node) | ||
else | ||
node = xpath_first_from_signed_assertion('/a:Subject/a:NameID') | ||
end | ||
node.nil? ? nil : node.text | ||
end | ||
end | ||
|
@@ -205,9 +227,10 @@ def issuers | |
issuers = [] | ||
nodes = REXML::XPath.match( | ||
document, | ||
"/p:Response/a:Issuer | /p:Response/a:Assertion/a:Issuer", | ||
"/p:Response/a:Issuer", | ||
{ "p" => PROTOCOL, "a" => ASSERTION } | ||
) | ||
nodes += xpath_from_signed_assertion("/a:Issuer") | ||
nodes.each do |node| | ||
issuers << node.text if node.text | ||
end | ||
|
@@ -371,11 +394,7 @@ def validate_num_assertion | |
# @raise [ValidationError] if soft == false and validation fails | ||
# | ||
def validate_no_encrypted_attributes | ||
nodes = REXML::XPath.match( | ||
document, | ||
"/p:Response/a:Assertion/a:AttributeStatement/a:EncryptedAttribute", | ||
{ "p" => PROTOCOL, "a" => ASSERTION } | ||
) | ||
nodes = xpath_from_signed_assertion("/a:AttributeStatement/a:EncryptedAttribute") | ||
if nodes && nodes.length > 0 | ||
return append_error("There is an EncryptedAttribute in the Response and this SP not support them") | ||
end | ||
|
@@ -391,7 +410,7 @@ def validate_no_encrypted_attributes | |
# | ||
def validate_signed_elements | ||
signature_nodes = REXML::XPath.match( | ||
document, | ||
decrypted_document.nil? ? document : decrypted_document, | ||
"//ds:Signature", | ||
{"ds"=>DSIG} | ||
) | ||
|
@@ -452,13 +471,13 @@ def validate_conditions | |
|
||
now = Time.now.utc | ||
|
||
if not_before && (now + (options[:allowed_clock_drift] || 0)) < not_before | ||
error_msg = "Current time is earlier than NotBefore condition #{(now + (options[:allowed_clock_drift] || 0))} < #{not_before})" | ||
if not_before && (now + allowed_clock_drift) < not_before | ||
error_msg = "Current time is earlier than NotBefore condition #{(now + allowed_clock_drift)} < #{not_before})" | ||
return append_error(error_msg) | ||
end | ||
|
||
if not_on_or_after && now >= not_on_or_after | ||
error_msg = "Current time is on or after NotOnOrAfter condition (#{now} >= #{not_on_or_after})" | ||
if not_on_or_after && now >= (not_on_or_after + allowed_clock_drift) | ||
error_msg = "Current time is on or after NotOnOrAfter condition (#{now} >= #{not_on_or_after + allowed_clock_drift})" | ||
return append_error(error_msg) | ||
end | ||
|
||
|
@@ -551,7 +570,17 @@ def validate_subject_confirmation | |
def validate_signature | ||
fingerprint = settings.get_fingerprint | ||
|
||
unless fingerprint && document.validate_document(fingerprint, soft, :fingerprint_alg => settings.idp_cert_fingerprint_algorithm) | ||
# If the response contains the signature, and the assertion was encrypted, validate the original SAML Response | ||
# otherwise, review if the decrypted assertion contains a signature | ||
response_signed = REXML::XPath.first( | ||
document, | ||
"/p:Response[@ID=$id]", | ||
{ "p" => PROTOCOL, "ds" => DSIG }, | ||
{ 'id' => document.signed_element_id } | ||
) | ||
doc = (response_signed || decrypted_document.nil?) ? document : decrypted_document | ||
|
||
unless fingerprint && doc.validate_document(fingerprint, :fingerprint_alg => settings.idp_cert_fingerprint_algorithm) | ||
error_msg = "Invalid Signature on SAML Response" | ||
return append_error(error_msg) | ||
end | ||
|
@@ -565,17 +594,18 @@ def validate_signature | |
# @return [REXML::Element | nil] If any matches, return the Element | ||
# | ||
def xpath_first_from_signed_assertion(subelt=nil) | ||
doc = decrypted_document.nil? ? document : decrypted_document | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line is repeated, maybe it worth a method? |
||
node = REXML::XPath.first( | ||
document, | ||
doc, | ||
"/p:Response/a:Assertion[@ID=$id]#{subelt}", | ||
{ "p" => PROTOCOL, "a" => ASSERTION }, | ||
{ 'id' => document.signed_element_id } | ||
{ 'id' => doc.signed_element_id } | ||
) | ||
node ||= REXML::XPath.first( | ||
document, | ||
doc, | ||
"/p:Response[@ID=$id]/a:Assertion#{subelt}", | ||
{ "p" => PROTOCOL, "a" => ASSERTION }, | ||
{ 'id' => document.signed_element_id } | ||
{ 'id' => doc.signed_element_id } | ||
) | ||
node | ||
end | ||
|
@@ -586,20 +616,93 @@ def xpath_first_from_signed_assertion(subelt=nil) | |
# @return [Array of REXML::Element] Return all matches | ||
# | ||
def xpath_from_signed_assertion(subelt=nil) | ||
doc = decrypted_document.nil? ? document : decrypted_document | ||
node = REXML::XPath.match( | ||
document, | ||
doc, | ||
"/p:Response/a:Assertion[@ID=$id]#{subelt}", | ||
{ "p" => PROTOCOL, "a" => ASSERTION }, | ||
{ 'id' => document.signed_element_id } | ||
{ 'id' => doc.signed_element_id } | ||
) | ||
node.concat( REXML::XPath.match( | ||
document, | ||
doc, | ||
"/p:Response[@ID=$id]/a:Assertion#{subelt}", | ||
{ "p" => PROTOCOL, "a" => ASSERTION }, | ||
{ 'id' => document.signed_element_id } | ||
{ 'id' => doc.signed_element_id } | ||
)) | ||
end | ||
|
||
# Obtains a SAML Response with the EncryptedAssertion element decrypted | ||
# @param document_copy [XMLSecurity::SignedDocument] A copy of the original SAML Response with the encrypted assertion | ||
# @return [XMLSecurity::SignedDocument] The SAML Response with the assertion decrypted | ||
# | ||
def decrypt_assertion_from_document(document_copy) | ||
response_node = REXML::XPath.first( | ||
document_copy, | ||
"/p:Response/", | ||
{ "p" => PROTOCOL } | ||
) | ||
encrypted_assertion_node = REXML::XPath.first( | ||
document_copy, | ||
"(/p:Response/EncryptedAssertion/)|(/p:Response/a:EncryptedAssertion/)", | ||
{ "p" => PROTOCOL, "a" => ASSERTION } | ||
) | ||
response_node.add(decrypt_assertion(encrypted_assertion_node)) | ||
encrypted_assertion_node.remove | ||
XMLSecurity::SignedDocument.new(response_node.to_s) | ||
end | ||
|
||
# Checks if the SAML Response contains or not an EncryptedAssertion element | ||
# @return [Boolean] True if the SAML Response contains an EncryptedAssertion element | ||
# | ||
def assertion_encrypted? | ||
encrypted_node = REXML::XPath.first( | ||
document, | ||
"(/p:Response/EncryptedAssertion/)|(/p:Response/a:EncryptedAssertion/)", | ||
{ "p" => PROTOCOL, "a" => ASSERTION } | ||
) | ||
!encrypted_node.nil? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would make this simpler:
|
||
end | ||
|
||
# Decrypts an EncryptedAssertion element | ||
# @param encrypted_assertion_node [REXML::Element] The EncryptedAssertion element | ||
# @return [REXML::Document] The decrypted EncryptedAssertion element | ||
# | ||
def decrypt_assertion(encrypted_assertion_node) | ||
if settings.nil? || !settings.get_sp_key | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this can be expressed with an eager return:
|
||
validation_error('An EncryptedAssertion found and no SP private key found on the settings to decrypt it') | ||
else | ||
assertion_plaintext = OneLogin::RubySaml::Utils.decrypt_data(encrypted_assertion_node, settings.get_sp_key) | ||
# If we get some problematic noise in the plaintext after decrypting. | ||
# This quick regexp parse will grab only the assertion and discard the noise. | ||
assertion_plaintext = assertion_plaintext.match(/(.*<\/(saml:|)Assertion>)/m)[0] | ||
# To avoid namespace errors if saml namespace is not defined at assertion_plaintext | ||
# create a parent node first with the saml namespace defined | ||
assertion_plaintext = '<node xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion">'+ assertion_plaintext + '</node>' | ||
doc = REXML::Document.new(assertion_plaintext) | ||
doc.root[0] | ||
end | ||
end | ||
|
||
# Decrypts an EncryptedID element | ||
# @param encryptedid_node [REXML::Element] The EncryptedID element | ||
# @return [REXML::Document] The decrypted EncrypedtID element | ||
# | ||
def decrypt_nameid(encryptedid_node) | ||
if settings.nil? || !settings.get_sp_key | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. eager return here too There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will refactor both methods ;) |
||
validation_error('An EncryptedID found and no SP private key found on the settings to decrypt it') | ||
else | ||
nameid_plaintext = OneLogin::RubySaml::Utils.decrypt_data(encryptedid_node, settings.get_sp_key) | ||
# If we get some problematic noise in the plaintext after decrypting. | ||
# This quick regexp parse will grab only the NameID and discard the noise. | ||
nameid_plaintext = nameid_plaintext.match(/(.*<\/(saml:|)NameID>)/m)[0] | ||
# To avoid namespace errors if saml namespace is not defined at assertion_plaintext | ||
# create a parent node first with the saml namespace defined | ||
nameid_plaintext = '<node xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion">'+ nameid_plaintext + '</node>' | ||
doc = REXML::Document.new(nameid_plaintext) | ||
doc.root[0] | ||
end | ||
end | ||
|
||
# Parse the attribute of a given node in Time format | ||
# @param node [REXML:Element] The node | ||
# @param attribute [String] The attribute name | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the code after this line is for encrypt the assertion, you should create a method for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
**decrypt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a method for that: decrypt_assertion_from_document but the problem is that I need to clone the document in order to later decrypt it (if I no clone, then when decrypting I change the original object). And due Marshal bugs on Ruby 1.8.7 I need to offer an alternative re-parsing the response. And I want to pass to the method the cloned object. Does it make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not clone the object inside
decrypt_assertion_from_document
and passdocument
orresponse
(whatever is best) to that same method?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luisvm, I think that your proposal turns the code more complex..and this is maybe what we are trying to avoid.. You need to pass document on 1.8.7 and response in other case, so you need to maintain the if else, and later, in the decrypt method, add another if/else statement...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pitbulk well, initialize is already pretty complex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luisvm could be that a solution?
https://gist.github.com/pitbulk/c143ddc83aeaefca3834