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

Include proper KeyName in encrypted assertion #781

Merged
merged 1 commit into from
Nov 2, 2021

Conversation

challet
Copy link
Contributor

@challet challet commented Mar 15, 2021

Problem

The IdP encrypted assertions returned by pysaml2 are always associated with the following KeyInfo :

<ns1:KeyInfo>
    <ns1:KeyName>my-rsa-key</ns1:KeyName>
</ns1:KeyInfo>

That can make some SPs to not know which one of their keys has been used (even if they specify just one in their metadata, they might perform a check at decryption time).

This PR aims to do the following :

  • If no KeyInfo exists in the SP metadata, no KeyInfo is associated with the encrypted assertion
  • If such an element exist in the SP metadata, it is re-used to inform what key has been used to encrypt the assertion

Solution

  • extracting certificates from the metadata now returns a list of tuples (key_name, cert)
  • this tuples are used at encryption time and the name is passed to the pre_encryption_part function
  • pre_encryption_part has a None default value for its key_name parameter and skip the KeyInfo element if no other value is passed

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?

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
@c00kiemon5ter
Copy link
Member

hello and thanks for the PR,

I have rebased the code and made small changes to make the diff smaller and the code cleaner.
I will make this part of the next release of pysaml2.

thank you!

@c00kiemon5ter c00kiemon5ter changed the title Ouput the according KeyName along encrypted assertion Include proper KeyName in encrypted assertion Nov 2, 2021
@c00kiemon5ter c00kiemon5ter merged commit 5caf6da into IdentityPython:master Nov 2, 2021
@challet
Copy link
Contributor Author

challet commented Nov 3, 2021

Great, thank you

@tommilligan
Copy link

Thanks to both of you for your work maintaining this library 🙂

I found this PR by upgrading pysaml to 7.1.0 and experiencing a breaking change, introduced here: https://github.com/IdentityPython/pysaml2/pull/781/files#diff-09a88bc829016486f76a00a634c17dbb59c113762723c7edb0c4c0e5103e6511R502

This changes the return type of the public Metadata.certs method from List[str] to List[Tuple[str, str]].

I wanted to ask if this should be noted in the changelog as a breaking change, or if it is assumed that minor versions can contain breaking changes? I'd like to know for future releases (I'd assumed the project uses semver by default).

@c00kiemon5ter
Copy link
Member

c00kiemon5ter commented Dec 3, 2021

Sorry for this @tommilligan; you are right that we should have noted that as a breaking change with a bump in the major version. I will update the changelog to mention that.

(already in the GH-releases https://github.com/IdentityPython/pysaml2/releases/tag/v7.1.0)

@tommilligan
Copy link

No worries, it happens! Thanks for the clarification, and updating the changelogs 🙂

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