metadata_persister does not appear to be called about saml_idp HOT 3 OPEN

Fourthed. From reading #167, this issue seems to stem from conflicting understanding where:

• the user of the gem assumes that metadata persister / metadata getter is some kind of a caching mechanism (just as how DNS works) and should be called at runtime
• the maintainer(s) of the gem assumes that metadata persistence should be done beforehand, not at runtime

I see the following problems and propose following fixes:

1. If metadata persistence should be done beforehand and not at runtime

• It is not documented that metadata should be persisted beforehand
  • ... or not even about how the metadata_persister / metadata_getter should be used
  • The maintainers' intention on how to use these functions should be documented somewhere, especially when there's a need for user action
• A straightforward way to actually persist the metadata does not exist
  • Just as this issue says, metadata_persister is not called from anywhere, and if someone has to persist the metadata beforehand, the person has to persist metadata by invoking this code manally.
  • This can be fixed by turning into the metadata persisting code into a rake task. This rake task should provide a default way to persist metadata, and would be run by passing the URL of the metadata and the ID of the SAML SP.
    • In this case, metadata_persister's code should not be written in the initializer. If somebody has a strong need to customize the metadata_persister's code, he/she can copy the rake task and create a custom rake task.
    • In the same way, metadata_getter should not be a lambda in the initializer and should just provide the basic getter code by default. If somebody has to customize it, he/she can monkey patch it (that's why we use ruby, right?).
    • As far as I know the default persister/getter is sufficient for most of the use cases and the only part that needs external parameters is the path to store persisted files.

2. If metadata persistence should work as a cache mechanism

I believe that this is the right way moving forward, and also what most of the users are expecting (from reading the comments). Metadata persistence should be used as a caching mechanism (so as not to overflow the SAML SP with metadata requests).
To support my opinion, OpenID Connect Discovery is used to provide OpenID Connect RPs with the information of how to use the IdP, including the JWKs URL, which provides the public key of the IdP. In #167 it is said that SP metadata persister is not suggested to be used when SP is over the Internet, but with TLS being used in most of the internet recently, the integrity of the SP metadata is preserved using TLS. OAuth and OpenID Connect operates under this security assumption, and it is safe to assume that SAML also can adopt this security assumption. If the IdP is operating under a very strict/paranoid security assumption that even TLS is not enough and only trust the metadata exchanged beforehand, they can fall back to the approach written in (1).

In this case, the metadata_getter and metadata_persister has to be rewritten so that:

• there is a preset cache expiration period in the config
• when metadata getter is called
  • if there is no current metadata or the cache is expired
    • call the metadata URL, update the cached(persisted) metadata with the retrieved metadata and return the retrieved metadata
  • else
    • return the cached(persisted) metadata

I am able to provide a patch if/when the design decision on how to handle persistence is fixed among the maintainers. I would like to hear from the maintainers about how this should be handled.

from saml_idp.

Seconded

from saml_idp.

Thirded. I'd expect that somewhere in service_provider.rb (current_metadata?) that it would call refresh_metadata though there's a number of gotchas and loops w/r/t checking signatures (valid_signature? depends on should_validate_signature? which looks to current_metadata.respond_to?(:sign_assertions?) which starts the cycle all over again trying to get current_metadata)

from saml_idp.