Skip to content

Updated #186 - Added options to switch IdP configurations#209

Closed
pelted wants to merge 2 commits intosaml-idp:masterfrom
kolide:multiple_certs_2-updated
Closed

Updated #186 - Added options to switch IdP configurations#209
pelted wants to merge 2 commits intosaml-idp:masterfrom
kolide:multiple_certs_2-updated

Conversation

@pelted
Copy link
Copy Markdown
Contributor

@pelted pelted commented Jul 31, 2024

References #186 by @hamaron

This PR rebases the current master onto the existing changes which resolves the conflicts mentioned in the PR review while bringing it up to date.

This is a valuable addition that I hate to see get forgotten.

@Zogoo
Copy link
Copy Markdown
Collaborator

Zogoo commented Aug 13, 2024

@pelted thanks for your valuable contribution, could you please fix the failing tests then let us quickly review and merge it

@Zogoo
Copy link
Copy Markdown
Collaborator

Zogoo commented Nov 1, 2024

@pelted Since proc can do the trick I think we don’t need to do another extra config like this. Because I think too many ways may confuse other people. For this reason, let me close this PR.
But if you could see other use cases that must supported by IDP please feel free to re-open it.

@Zogoo Zogoo closed this Nov 1, 2024
@hamaron
Copy link
Copy Markdown
Contributor

hamaron commented Nov 1, 2024

I just noticed all things happened around the changes I originally made.

Thank you @pelted for implementing these changes and @Zogoo for managing them.

I'd like to clarify the use case for runtime IDP configuration. The new configuration approach:

config.x509_certificate = -> { File.read("cert.pem") }
config.secret_key = -> { SecretKeyFinder.key_for(id: 1) }
config.password = -> { Rails.application.credentials.dig(:saml_idp, :password) }

This works well when settings are determined at Rails startup because it's an initializer.
However, our use case requires dynamic configuration in the IDP controller based on the requesting user. This was the motivation behind the interface proposed in #186:

def create
  @saml_response = encode_response(current_user, {
    x509_certificate: current_user.retrieve_x509_cert,
    secret_key: current_user.retrieve_secret_key
  })
  render template: "saml_idp/idp/saml_post", layout: false
end

While we could potentially use a proc with a principal parameter (similar to config.name_id.formats):

config.x509_certificate = -> (principal) { principal.retrieve_x509_certificate_from_database }

This would mix business logic into configuration files, which I'm not a big fan of since it's a violation of
separation of concern.
For cases requiring dynamic configuration, having the ability to set these values in the controller remains valuable.

I'm not quite sure if I need to mention this, but configuring the SamlIdp.config in the controller
would not work since it is not thread safe because SamlIdp.config is a singleton.

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