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

Fixes and improvements for satosa-saml-metadata #429

Merged
merged 2 commits into from Jun 11, 2023

Conversation

vladimir-mencl-eresearch
Copy link
Contributor

Hi @c00kiemon5ter ,

I am working on a new feature for my deployment where I'd rely on the metadata created by satosa-saml-metadata - I'd then register this metadata into our federation.

When trying to use it, I found the metadata would always be signed - but for this purpose, I'd rather avoid having the signature embedded (on the EntityDescriptor for a single SP) - so I've added a --no-sign option. And with this option, I made the signing cert and key optional.

I also found the metadata was missing the encryption keys (KeyDescriptor use="encryption") - and I found it was because SATOSA SAML Backend makes changes to the loaded config, but this change was missed by the satosa-saml-metadata tool because of how it was referring to the config - found this was an easy fix to make.

Do these two changes look to you OK to merge?

Thanks a lot in advance for getting back to me.

Cheers,
Vlad

All Submissions:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what problem you are trying to solve with this PR?
  • Have you added information on what your changes do and why you chose this as your solution?
  • Have you written new tests for your changes?
  • Does your submission pass tests?
  • This project follows PEP8 style guide. Have you run your code against the 'flake8' linter?

@vladimir-mencl-eresearch vladimir-mencl-eresearch changed the title Saml metadata opts Fixes and improvements for satosa-saml-metadata Jan 27, 2023
@vladimir-mencl-eresearch
Copy link
Contributor Author

Hi @c00kiemon5ter ,

Just wondering whether there's anything blocking this PR from being reviewed and merged?

I think these are fairly trivial and backwards compatible changes.

Please let me know if there are any issues to address / points to clarify.

Thanks a lot in advance for getting back to me.

Cheers,
Vlad

…adata

... because SAMLBackend modifies the config (adding encryption_keypairs to config)
and this modified config is stored under sp.config.

Otherwise, metadata created via the metadata-creation scripts (satosa-saml-metadata)
would be missing encryption keys (KeyDescriptor use="encryption").
Allow skipping signing with --no-sign - and in that case, do not require key+cert.

Default to signing enabled (keep existing behaviour).

Mark key and cert args as optional in Click and instead check them explicitly
when signing is enabled.

Add new method create_entity_descriptor_metadata as counterpart
to create_signed_entity_descriptor to also apply `valid` option
to EntityDescriptor but avoid signing.
@vladimir-mencl-eresearch
Copy link
Contributor Author

Hi @c00kiemon5ter ,

I've just rebased this on master to bring this up to date.

These changes are I think trivial - is there anything you'd like to see changed before merging them?

Cheers,
Vlad

@vladimir-mencl-eresearch
Copy link
Contributor Author

@c00kiemon5ter , can you please let me know if there's anything blocking this PR - or whether you'd be able to review and merge?

It's a fairly trivial fix + improvement...

Thanks a lot in advance for getting back to me.

Cheers,
Vlad

@c00kiemon5ter c00kiemon5ter merged commit 69b532a into IdentityPython:master Jun 11, 2023
@c00kiemon5ter
Copy link
Member

Thank you Vlad, and sorry for the poor communication.

@vladimir-mencl-eresearch
Copy link
Contributor Author

Np - and thanks for accepting this in!

@vladimir-mencl-eresearch vladimir-mencl-eresearch deleted the saml-metadata-opts branch June 13, 2023 05:01
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