-
Notifications
You must be signed in to change notification settings - Fork 181
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
[feat] External Signature param for Redirect Binding #203
Changes from all commits
3e58ea2
60fc586
d7ea637
ae83a16
41aad96
43b588f
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 |
---|---|---|
|
@@ -3,7 +3,7 @@ | |
require 'logger' | ||
module SamlIdp | ||
class Request | ||
def self.from_deflated_request(raw) | ||
def self.from_deflated_request(raw, external_attributes = {}) | ||
if raw | ||
decoded = Base64.decode64(raw) | ||
zstream = Zlib::Inflate.new(-Zlib::MAX_WBITS) | ||
|
@@ -18,18 +18,22 @@ def self.from_deflated_request(raw) | |
else | ||
inflated = "" | ||
end | ||
new(inflated) | ||
new(inflated, external_attributes) | ||
end | ||
|
||
attr_accessor :raw_xml | ||
attr_accessor :raw_xml, :saml_request, :signature, :sig_algorithm, :relay_state | ||
|
||
delegate :config, to: :SamlIdp | ||
private :config | ||
delegate :xpath, to: :document | ||
private :xpath | ||
|
||
def initialize(raw_xml = "") | ||
def initialize(raw_xml = "", external_attributes = {}) | ||
self.raw_xml = raw_xml | ||
self.saml_request = external_attributes[:saml_request] | ||
self.relay_state = external_attributes[:relay_state] | ||
self.sig_algorithm = external_attributes[:sig_algorithm] | ||
self.signature = external_attributes[:signature] | ||
end | ||
|
||
def logout_request? | ||
|
@@ -85,7 +89,7 @@ def log(msg) | |
end | ||
end | ||
|
||
def valid? | ||
def valid?(external_attributes = {}) | ||
unless service_provider? | ||
log "Unable to find service provider for issuer #{issuer}" | ||
return false | ||
|
@@ -96,8 +100,15 @@ def valid? | |
return false | ||
end | ||
|
||
unless valid_signature? | ||
log "Signature is invalid in #{raw_xml}" | ||
# XML embedded signature | ||
if signature.nil? && !valid_signature? | ||
log "Requested document signature is invalid in #{raw_xml}" | ||
return false | ||
end | ||
|
||
# URI query signature | ||
if signature.present? && !valid_external_signature? | ||
log "Requested URI signature is invalid in #{raw_xml}" | ||
return false | ||
end | ||
|
||
|
@@ -120,12 +131,29 @@ def valid_signature? | |
# Validate signature when metadata specify AuthnRequest should be signed | ||
metadata = service_provider.current_metadata | ||
if logout_request? || authn_request? && metadata.respond_to?(:sign_authn_request?) && metadata.sign_authn_request? | ||
document.valid_signature?(service_provider.fingerprint) | ||
document.valid_signature?(service_provider.cert, service_provider.fingerprint) | ||
else | ||
true | ||
end | ||
end | ||
|
||
def valid_external_signature? | ||
cert = OpenSSL::X509::Certificate.new(service_provider.cert) | ||
|
||
sha_version = sig_algorithm =~ /sha(.*?)$/i && $1.to_i | ||
raw_signature = Base64.decode64(signature) | ||
|
||
signature_algorithm = case sha_version | ||
when 256 then OpenSSL::Digest::SHA256 | ||
when 384 then OpenSSL::Digest::SHA384 | ||
when 512 then OpenSSL::Digest::SHA512 | ||
else | ||
OpenSSL::Digest::SHA1 | ||
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. should this default value be configurable ? 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. The request will have an extra parameter that should be used for validation. 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. SHA-1 is not anymore enabled on modern distributions (like RHEL 9) |
||
end | ||
|
||
cert.public_key.verify(signature_algorithm.new, raw_signature, query_request_string) | ||
end | ||
|
||
def service_provider? | ||
service_provider && service_provider.valid? | ||
end | ||
|
@@ -148,6 +176,13 @@ def session_index | |
@_session_index ||= xpath("//samlp:SessionIndex", samlp: samlp).first.try(:content) | ||
end | ||
|
||
def query_request_string | ||
url_string = "SAMLRequest=#{CGI.escape(saml_request)}" | ||
url_string << "&RelayState=#{CGI.escape(relay_state)}" if relay_state | ||
url_string << "&SigAlg=#{CGI.escape(sig_algorithm)}" | ||
end | ||
private :query_request_string | ||
|
||
def response_host | ||
uri = URI(response_url) | ||
if uri | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ module SamlIdp | |
end | ||
|
||
it "signs valid xml" do | ||
expect(Saml::XML::Document.parse(subject.signed).valid_signature?(Default::FINGERPRINT)).to be_truthy | ||
expect(Saml::XML::Document.parse(subject.signed).valid_signature?("", Default::FINGERPRINT)).to be_truthy | ||
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 does not need to be better validated? 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. The first attribute of this method is optional if the XML document already contains the certificate with the document. I understand that the code may look a bit strange, and using hash keys might be better, but that would require a lot of changes across different layers, which I want to avoid with these new changes. |
||
end | ||
|
||
it "includes logout element" do | ||
|
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.
There are a few notes in the README that are broken by these signature changes - can you take a quick pass at updating those?
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.
It might(?) just be this one
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 have checked README, there was no specific section on how
controller.rb
methods could be help. But I have added a section that explains the starting point of this gem.