Skip to content

Method to Match certs including SHA256 errors#126

Merged
Sgtpluck merged 5 commits intomainfrom
dmm/fix-digest-stuff
Nov 25, 2024
Merged

Method to Match certs including SHA256 errors#126
Sgtpluck merged 5 commits intomainfrom
dmm/fix-digest-stuff

Conversation

@Sgtpluck
Copy link
Copy Markdown

Recently, I merged code to add an add an explicit error for when requests are using anything but SHA256, our desired encryption algorithm, for the signature method algorithm.

However, when I went to add this to the Idp, I tried to write some tests to replicate the original behavior and was unable to. This is when I determined (with the help of @jmhooper) that we actually do not seem to have validations for the signature method algorithm, but we ARE doing some determinations around whether the digest method algorithm is SHA256. We use that algorithm to ensure that the cert that is passed in the request matches a Service Provider's registered certs, but that code is complex and could be streamlined by just directly comparing the certificates (if the certificate is embedded in the request.)

Updating this means there is a possibility of a currently valid request being marked as invalid, or vice versa, which means there are opportunities for us to find a different matching certificate.

This change:

  • creates a new entry point (validate_with_sha256) to the XmlSecurity class that we can call so that we can set up event tracking in the IdP in order to determine if any integrations will break.
  • introduces a new, easier to understand (imo) way to compare the certs that we can use to replace the existing fingerprint code (we don't need to mess around with fingerprints, since we have the certificates)

@Sgtpluck Sgtpluck requested a review from jmhooper November 22, 2024 22:17
Comment on lines +86 to +93
@request_cert ||= if cert_element.text.blank?
raise ValidationError.new(
'Certificate element present in response (ds:X509Certificate) but evaluating to nil',
:no_certificate_in_request
)
end

cert_element.text
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As-is, the memoization seems to be memoizing the raise which is not actually a value? so I think this is memoizing nil

Unless cert_element.text is very expensive, probably easier not to memoize at all

Suggested change
@request_cert ||= if cert_element.text.blank?
raise ValidationError.new(
'Certificate element present in response (ds:X509Certificate) but evaluating to nil',
:no_certificate_in_request
)
end
cert_element.text
if cert_element.text.blank?
raise ValidationError.new(
'Certificate element present in response (ds:X509Certificate) but evaluating to nil',
:no_certificate_in_request
)
end
cert_element.text

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated in 1eb5cca

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants