settings parser throws "idp cert not found" exception

[do9xe]

Hi everyone,

I just found a potential issue in the configuration check that causes a OneLogin_Saml2_Error Exception with the message idp_cert_or_fingerprint_not_found_and_required.
As with many other programs I wan't to keep my certificate files in a special folder on my server. Therefor I left the x509 cert and key fields in the SP and IDP config part empty and provided a custom_base_path. The SP cert and key are loaded correctly from the file, just the configuration check for the IDP config is failing.

How to reproduce:

1. set a custom_base_path in the config
2. place all files (sp.crt, sp.key, idp.crt) in a certs folder in the base path
3. leave all x509 cert and key fileds in the SP and IDP config empty
4. set signMetadata to True
5. try to get the Metadata from the SP.

Suggested Fix
I identified this line of code as the root cause of the issue, as it only checks for an x509 cert in the IDP config, but not in the file.

python3-saml/src/onelogin/saml2/settings.py

Line 392 in a1211a8

exists_x509 = bool(idp.get('x509cert'))

My Suggestion would be to use the native function in order to check both the configuration and a potential cert file.
exists_x509 = bool(self.get_idp_cert())

If this is considered as an issue that needs to be fixed I'll be happy to open a pull request.

[pitbulk]

The check_idp_settings method checks that the settings dict provided are ok. As no 'x509cert' is inside the dict, is expected that this raises an error.

Can you describe how are you getting the idp_cert_or_fingerprint_not_found_and_required error? how are you intitializing the toolkit?

[do9xe]

I use Flask-Multipass as a wrapper. For SAML it just maps the config through.
In my config wantAssertionsSigned and wantMessagesSigned is set to true, which enforces the error to be added if there is no cert in the config. The idp_cert_or_fingerprint_not_found_and_required error is directly caused by the configuration being invalid as the cert is in a file and not in the dict.

I understand that the check_idp_settings is dedicated to the actual config-dict, but it is not taking into account that the idp cert could also be in a file.

Edit: The SP certs are checked with check_sp_certs() which basically calls get_sp_cert() - therefor I think the IDP cert should be checked the same way.

Here is the full stacktrace, just in case:

[2024-02-08 14:09:43,332] ERROR in app: Exception on /login/saml [GET]
Traceback (most recent call last):
  File "<project-path>\.venv\lib\site-packages\flask\app.py", line 1463, in wsgi_app
    response = self.full_dispatch_request()
  File "<project-path>\.venv\lib\site-packages\flask\app.py", line 872, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "<project-path>\.venv\lib\site-packages\flask\app.py", line 870, in full_dispatch_request
    rv = self.dispatch_request()
  File "<project-path>\.venv\lib\site-packages\flask\app.py", line 855, in dispatch_request
    return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)  # type: ignore[no-any-return]
  File "<project-path>\.venv\lib\site-packages\flask_multipass\core.py", line 165, in process_login
    return self._login_external(provider)
  File "<project-path>\.venv\lib\site-packages\flask_multipass\core.py", line 503, in _login_external
    return provider.initiate_external_login()
  File "<project-path>\.venv\lib\site-packages\flask_multipass\providers\saml.py", line 89, in initiate_external_login
    auth = self._init_saml_auth()
  File "<project-path>\.venv\lib\site-packages\flask_multipass\providers\saml.py", line 83, in _init_saml_auth
    return OneLogin_Saml2_Auth(req, config)
  File "<project-path>\.venv\lib\site-packages\onelogin\saml2\auth.py", line 57, in __init__
    self._settings = OneLogin_Saml2_Settings(old_settings, custom_base_path)
  File "<project-path>\.venv\lib\site-packages\onelogin\saml2\settings.py", line 128, in __init__
    raise OneLogin_Saml2_Error(
onelogin.saml2.errors.OneLogin_Saml2_Error: Invalid dict settings: idp_cert_or_fingerprint_not_found_and_required

[pitbulk]

oh ok, now I see your proposal, so having something like:

def check_idp_settings(self, settings):
        """
        Checks the IdP settings info.
        :param settings: Dict with settings data
        :type settings: dict
        :returns: Errors found on the IdP settings data
        :rtype: list
        """
        assert isinstance(settings, dict)

        errors = []
        if not isinstance(settings, dict) or len(settings) == 0:
            errors.append('invalid_syntax')
        else:
            if not settings.get('idp'):
                errors.append('idp_not_found')
            else:
                allow_single_domain_urls = self._get_allow_single_label_domain(settings)
                
                # get_idp_cert uses self._idp so I add it
                old_idp = self._idp
                self._idp = settings['idp']
                
                idp = settings['idp']
                if not idp.get('entityId'):
                    errors.append('idp_entityId_not_found')

                if not idp.get('singleSignOnService', {}).get('url'):
                    errors.append('idp_sso_not_found')
                elif not validate_url(idp['singleSignOnService']['url'], allow_single_domain_urls):
                    errors.append('idp_sso_url_invalid')

                slo_url = idp.get('singleLogoutService', {}).get('url')
                if slo_url and not validate_url(slo_url, allow_single_domain_urls):
                    errors.append('idp_slo_url_invalid')

                if 'security' in settings:
                    security = settings['security']

                    exists_x509 = bool(self.get_idp_cert())
                    exists_fingerprint = bool(idp.get('certFingerprint'))

                    exists_multix509sign = 'x509certMulti' in idp and \
                        'signing' in idp['x509certMulti'] and \
                        idp['x509certMulti']['signing']
                    exists_multix509enc = 'x509certMulti' in idp and \
                        'encryption' in idp['x509certMulti'] and \
                        idp['x509certMulti']['encryption']

                    want_assert_sign = bool(security.get('wantAssertionsSigned'))
                    want_mes_signed = bool(security.get('wantMessagesSigned'))
                    nameid_enc = bool(security.get('nameIdEncrypted'))

                    if (want_assert_sign or want_mes_signed) and \
                            not (exists_x509 or exists_fingerprint or exists_multix509sign):
                        errors.append('idp_cert_or_fingerprint_not_found_and_required')
                    if nameid_enc and not (exists_x509 or exists_multix509enc):
                        errors.append('idp_cert_not_found_and_required')
        
        # Restores the value that had the self._idp
        if 'old_idp' in locals():
            self._idp = old_idp
        return errors

Can you create the PR and maybe add some tests?

[do9xe]

Sure, I`ll see what I can do.

[do9xe]

In order to add a test with and without a certificate present in the certs folder and also because I'd like to use it in production:
Would it be okay, if I add the possibility to provide a custom filename for the certificates and keys? Default would stay the same, just adding and option to enable the use of MyOrgRootCA.crt as a filename.
Also, I could add the idp.crt into the certs folder of the tests and then test what happens if the cert is read from the file or if the cert is not provided by setting the name wrong.crt.
Just an Idea to add a test for idp-cert files and more functionality.

[pitbulk]

The tests folder of the repo already has a certs folder there, you can add the idp.crt file there.

Be able to customize the name of the cert file I believe will add extra complexity to the code of the project.

[atezet]

I'm having a similar issue, but I do set idp.metadata_path and I was wondering if it wouldn't be possible to just use the certificates available in there if no explicit fingerprint/certificate is set.

Edit: only now I realise that metadata_path is not part of this library but of the wrapper application I am using. Take it as a suggestion that I think it would be good to be able to just grab an IdP's metadata and work all settings out from there.