Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Most of SAML integration fixed #605

Merged
merged 3 commits into from Dec 15, 2019

Conversation

Neven1986
Copy link
Contributor

- Variable references inside SAML class were fixed
- Function signatures inside SAML class were fixed
- Redirect URL for /saml/login path was modified (saml_authorized -> index.saml_authorized)

Current status is that SAML metadata can be generated under /saml/metadata and communication to SAML iDP is working

Problems remaining:
    - SAML Response doesn't contain any attributes (There is no AttributeStatement on the Response). Probably it is problem on iDP side. I'll need to take other SAML iDP.
    - Background thread in retrieve_idp_data() cannot be spawned, this part is currently commented out, old code needs to be revisited. Will to try to resolve it soon.

    - Function signatures inside SAML class were fixed
    - Redirect URL for /saml/login path was modified (saml_authorized -> index.saml_authorized)

    Current status is that SAML metadata can be generated under /saml/metadata and communication to SAML iDP is working

    Problems remaining:
        - SAML Response doesn't contain any attributes (There is no AttributeStatement on the Response). It can be that problem is on iDP side
        - Background thread in retrieve_idp_data() cannot be spawned, this part is currently commented out, old code needs to be revisited
@lgtm-com
Copy link

lgtm-com bot commented Dec 14, 2019

This pull request introduces 1 alert when merging cd3535d into ad6b04b - view on LGTM.com

new alerts:

  • 1 for Unused import

From my perspective, if agreed, this change can be merged, because the basic SAM auth. functionality is now present
and was tested with "samlidp.io" iDP.

However, there are further improvements which I would like to integrate, but as a separate features in separate pull requests
@Neven1986
Copy link
Contributor Author

SAML seems to be reintegrated now. I conducted few tests with samlidp.io as iDP.
Please review this PR and if you find it OK merge it.

There are few things which should be implemented, but as new features and as separate pull requests. I will try to take care of it as well in coming week or two.

@ngoduykhanh
Copy link
Contributor

Hi @Neven1986 , thanks for submitting the PR. The code looks good to me. I wanted to test with samlidp.io as well. Do you know which SAML_SP_ENTITY_ID value and certificate I should use for it?

@Neven1986
Copy link
Contributor Author

Hi @ngoduykhanh,

SAML SP ENTITY ID is pretty arbitrary thing and can be almost any URI derived from your domain or your domain itself. In my case that was http://pda.<my.domain>:9191/ - simple as that...

You can use any self-signed x509 certificate... I used my own generated one and I used in second test certificates generated by PDA from certutils.py

Just as side note... With samlidp.io there is one peculiar thing. In order to get attributes in response you need to explicitly request them by defining AttributeConsumingService attributes in your metadata... Which is currently empty 'sp settings' substructure in saml.py. For testing purposes I needed to manually define this in the code since it's not configurable via default_config.py. This is one of improvements which I talked about, that I would like to implement.

I did additional test with my company's iDP and it passed went through as well. The only difference is that this iDP will return all attributes without requesting them specifically in SP metadata. A lot of things are dependent on iDP config itself.

If you need more heads-up on this I can send you my example metadata.

@ngoduykhanh
Copy link
Contributor

Hi @Neven1986 , thanks for the explanation and tests.
I wanted to keep the default_config file minimal. If you look at the configs/development.py file, you would see more config related to SAML.

I am going to merge your PR. If you have any adjustments in the feature, feel free to create another one.

@ngoduykhanh ngoduykhanh merged commit 451bad2 into PowerDNS-Admin:saml_fixes Dec 15, 2019
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.

None yet

2 participants