Skip to content

v2.0: Support DSA and ECDSA signing keys#705

Merged
pitbulk merged 6 commits intoSAML-Toolkits:v2.xfrom
johnnyshields:ec-dsa-redux
Sep 30, 2024
Merged

v2.0: Support DSA and ECDSA signing keys#705
pitbulk merged 6 commits intoSAML-Toolkits:v2.xfrom
johnnyshields:ec-dsa-redux

Conversation

@johnnyshields
Copy link
Copy Markdown
Collaborator

@johnnyshields johnnyshields commented Jul 10, 2024

Fixes #661

Replaces #683

Depends on #711, please merge it first.

Currently RubySaml supports only RSA keys. The SAML standard can also support ECDSA and DSA keys. This PR adds support for both:

  1. Validating IdP EC/DSA sigs AND
  2. Using SP EC/DSA signing keys.

JRuby supports DSA but cannot support ECDSA due to this issue.

You may not use EC/DSA keys for encryption. An ArgumentError is raised if you attempt to do so.

It includes the following changes, which are all done in a backward compatible manner:

  • When generating SP metadata/requests, settings.security[:signature_method] now ignores the "rsa" component of its user-set value and automatically uses whatever type of SP signing public key you actually set (e.g. a DSA key) plus the "sha" component of the value.
    • (Previously, only RSA was supported, so this doesn't break anything.)
  • settings.security[:signature_method] supports shortcut values :sha1, :sha256, etc.
    • Shortcuts :rsa_sha256, :dsa_sha256 etc. also work, but as per above the "rsa"/"dsa" are ignored in favor of the SP public key type.
  • Similar to above settings.security[:digest_method] supports shortcut values sha1, sha256, etc.
  • New module XMLSecurity::Crypto is extracted from XMLSecurity::Document
  • Cleaned-up code, including related to canonicalization

DONE:

  • Ensure existing tests pass
  • request_test.rb -- cover all 3 key types
  • logoutrequest_test.rb -- cover all 3 key types
  • slo_logoutresponse_test.rb -- cover all 3 key types
  • metadata_test.rb -- cover all 3 key types
  • Fix JRuby failures
  • basic tests (settings, etc.) -- cover all 3 key types
  • test EC/DSA keys when used by IdP
  • disable encryption with EC/DSA keys

@johnnyshields johnnyshields changed the base branch from master to v2.x July 10, 2024 16:04
@johnnyshields johnnyshields changed the title Ec dsa redux [WIP - NOT READY] v2.0: Support DSA and ECDSA signing keys Jul 10, 2024
@johnnyshields
Copy link
Copy Markdown
Collaborator Author

johnnyshields commented Jul 10, 2024

@pitbulk this is ready for your initial review. I'll give it another pass tomorrow but the core changes and tests are basically working 🎉

@johnnyshields johnnyshields changed the title [WIP - NOT READY] v2.0: Support DSA and ECDSA signing keys [PLEASE REVIEW BUT DO NOT MERGE] v2.0: Support DSA and ECDSA signing keys Jul 11, 2024
…y PEM values, including the `RubySaml::Util#format_cert` and `#format_private_key` methods. Introduces new `RubySaml::PemFormatter` module.
@johnnyshields johnnyshields force-pushed the ec-dsa-redux branch 12 times, most recently from 4680d8d to 9430fe9 Compare July 13, 2024 14:52
@johnnyshields johnnyshields changed the title [PLEASE REVIEW BUT DO NOT MERGE] v2.0: Support DSA and ECDSA signing keys [READY] v2.0: Support DSA and ECDSA signing keys Jul 13, 2024
@johnnyshields
Copy link
Copy Markdown
Collaborator Author

@pitbulk this is ready for your review and merge. (Please review #711 first)

@johnnyshields
Copy link
Copy Markdown
Collaborator Author

@pitbulk FYI I am now running this branch in prod without issue.

@pitbulk pitbulk merged commit 455d17d into SAML-Toolkits:v2.x Sep 30, 2024
@pitbulk
Copy link
Copy Markdown
Collaborator

pitbulk commented Sep 30, 2024

@johnnyshields, merged this one but rubocop needs to be fixed

@pitbulk pitbulk changed the title [READY] v2.0: Support DSA and ECDSA signing keys v2.0: Support DSA and ECDSA signing keys Mar 19, 2025
bcgraham pushed a commit to vericred/ruby-saml that referenced this pull request Nov 5, 2025
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.

2 participants