Skip to content

Allow SPs to have multiple certs (LG-2049)#4851

Merged
zachmargolis merged 14 commits intomainfrom
margolis-multiple-certs
Mar 30, 2021
Merged

Allow SPs to have multiple certs (LG-2049)#4851
zachmargolis merged 14 commits intomainfrom
margolis-multiple-certs

Conversation

@zachmargolis
Copy link
Contributor

This PR should let us manage multiple certs per SP.

At a high level:

  • SAML requests can be signed by an SP's private cert, and so we detect the cert used and then encrypt the response to the matching cert
  • OIDC assertions are signed by a client cert, but the responses are all signed by the IDP's public key so no need to change the response

Next steps:

  • Update the identity-idp-config to use the certs (plural)
  • Update the dashboard to send the certs (plural) key
  • Validate with a real partner that they can use multiple certs with their SP in INT
  • Cleanup: Remove usage of singular cert
    • Remove in dashboard
    • Remove in identity-idp-config


class ServiceProvider < ApplicationRecord
self.ignored_columns = %w[deal_id agency aal]
self.ignored_columns = %w[deal_id agency aal fingerprint]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was a very old column and adding it here makes sure we don't reference it at all

Copy link
Contributor

Choose a reason for hiding this comment

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

this was a very old column and adding it here makes sure we don't reference it at all

Appears we may still be referencing it during local development make setup:

end.update!(config.except('agency',

fingerprint: '08:79:F5:B1:B8:CC:EC:8F:5C:2A:58:03:30:14:C9:E6:F1:67:78:F1:97:E8:3A:88:EB:8E:70:92:25:D2:2F:32'

This is preventing make setup from succeeding:

ActiveModel::UnknownAttributeError: unknown attribute 'fingerprint' for ServiceProvider.
/Users/Documents/Code/identity-idp/app/services/service_provider_seeder.rb:20:in `block in run'
/Users/Documents/Code/identity-idp/app/services/service_provider_seeder.rb:13:in `each'
/Users/Documents/Code/identity-idp/app/services/service_provider_seeder.rb:13:in `run'
/Users/Documents/Code/identity-idp/db/seeds.rb:2:in `<main>'

We do want to remove that configuration value from the second file?

Copy link
Contributor

Choose a reason for hiding this comment

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

For posterity: #4877

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, should fingerprint be removed from COLUMNS in NullServiceProvider? Currently it behaves differently between ServiceProvider and NullServiceProvider:

[3] pry(main)> ServiceProvider.new.fingerprint
NoMethodError: undefined method `fingerprint' for #<ServiceProvider:0x00007fad4c15e810>
from /Users/andrewmduthie/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/activemodel-6.1.3.1/lib/active_model/attribute_methods.rb:469:in `method_missing'
[7] pry(main)> NullServiceProvider.new(issuer:nil).fingerprint
=> nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for these catches... will make a PR to remedy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@stevegsa stevegsa left a comment

Choose a reason for hiding this comment

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

Excellent job!

Copy link
Contributor

@solipet solipet left a comment

Choose a reason for hiding this comment

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

Looks great - just one question

Copy link
Contributor

@orenyk orenyk left a comment

Choose a reason for hiding this comment

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

Looks good! A few questions below.

aduth added a commit that referenced this pull request Apr 8, 2021
zachmargolis pushed a commit that referenced this pull request Apr 8, 2021
* Revert "Ensure non-nil fingerprint for SAML logout (#4890)"

This reverts commit 00deec6.

* Revert "Stop referencing ServiceProvider#fingerprint (#4884)"

This reverts commit 7fe7be7.

* Revert "Remove fingerprint from seeded local development service provider (#4877)"

This reverts commit a8c9481.

* Revert "Allow SPs to have multiple certs (LG-2049) (#4851)"

This reverts commit 0efa17f.

* Restore migration from multi-cert

* Re-add certs column to NullServiceProvider
aduth added a commit that referenced this pull request Apr 8, 2021
* Revert "Ensure non-nil fingerprint for SAML logout (#4890)"

This reverts commit 00deec6.

* Revert "Stop referencing ServiceProvider#fingerprint (#4884)"

This reverts commit 7fe7be7.

* Revert "Remove fingerprint from seeded local development service provider (#4877)"

This reverts commit a8c9481.

* Revert "Allow SPs to have multiple certs (LG-2049) (#4851)"

This reverts commit 0efa17f.

* Restore migration from multi-cert

* Re-add certs column to NullServiceProvider
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.

5 participants