Skip to content

Add SHA1 support to Fingerprint.certificate_digest#247

Open
pelted wants to merge 1 commit into
saml-idp:masterfrom
kolide:chris/add-sha1-fingerprint-support
Open

Add SHA1 support to Fingerprint.certificate_digest#247
pelted wants to merge 1 commit into
saml-idp:masterfrom
kolide:chris/add-sha1-fingerprint-support

Conversation

@pelted
Copy link
Copy Markdown
Contributor

@pelted pelted commented Apr 2, 2026

Summary

Add :sha1 support to SamlIdp::Fingerprint.certificate_digest so the helper can generate fingerprints that match the verification logic already used in SignedDocument#validate.

Problem

Before this change, Fingerprint.certificate_digest supported only :sha256 and :sha512.

That did not match the behavior in SignedDocument#validate, which compares:

  • a fingerprint derived from the XML document's <ds:DigestMethod>
  • a SHA1 fingerprint fallback computed unconditionally

In practice, this meant SHA1 fingerprints were already accepted during validation, but the helper could not generate them.

This was especially inconsistent because Fingerprint.certificate_digest(cert) falls back to SamlIdp.config.algorithm, and the default configured algorithm is already :sha1.

As a result, the helper's default path could raise ArgumentError even though SHA1 fingerprints are valid inputs for request validation.

Fix

Add :sha1 to Fingerprint.digest_sha_class, mapped to Digest::SHA1.

This aligns the fingerprint helper with the existing validation behavior in xml_security.rb, which already uses SHA1 as a supported fingerprint comparison path.

Testing

Expand Fingerprint specs to cover:

  • SHA1 fingerprints
  • SHA256 fingerprints
  • SHA512 fingerprints
  • unsupported algorithms raising ArgumentError

Notes

Using SHA1 here is for certificate identity matching, not for signature security. Signature verification still happens separately in validate_doc using the algorithm declared by the XML signature.

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.

1 participant