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

Allow to choose algorithms when creating new metadata #645

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

psmiraglia
Copy link

The make_metadata.py uses default (and poor) algorithms for signature and digest computation when creating new metadata. It would be nice to allow the selection of these algorithms. The PR introduces the -S and -D command line arguments that can be used as follows

$ ../../tools/make_metadata.py \
    -s -x /usr/bin/xmlsec1 \
    -k pki/mykey.pem -c pki/mycert.pem \
    -S http://www.w3.org/2001/04/xmldsig-more#rsa-sha384 \
    -D http://www.w3.org/2001/04/xmlenc#sha512 \
    sp_conf

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?

@codecov
Copy link

codecov bot commented Sep 20, 2019

Codecov Report

Merging #645 into master will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #645      +/-   ##
==========================================
+ Coverage   65.44%   65.48%   +0.04%     
==========================================
  Files         103      103              
  Lines       25703    25703              
==========================================
+ Hits        16821    16832      +11     
+ Misses       8882     8871      -11
Impacted Files Coverage Δ
src/saml2/__init__.py 87.78% <0%> (-0.19%) ⬇️
src/saml2/time_util.py 87.42% <0%> (+0.59%) ⬆️
src/saml2/validate.py 79.21% <0%> (+4.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b224a02...123427f. Read the comment docs.

@peppelinux
Copy link
Member

@psmiraglia Hey Paolo, nice to see you here!
The discussion on this topic is here: #628

I also saw that you have also bringed some commits from another branch of mine (those with allow_create documentation... already merged by c00kieMon5ter here: https://github.com/IdentityPython/pysaml2/pull/632/files).

If you can we could work on this in that PR, close this one.
We should move to a configuration asset that permits users to disable weak alghorithms not only in the metadata but also during signature/encryption verification.

Nice shoot

@psmiraglia psmiraglia force-pushed the feat/allow-to-choose-metadata-algs branch from eace864 to 123427f Compare September 20, 2019 10:31
@psmiraglia
Copy link
Author

@psmiraglia Hey Paolo, nice to see you here!
The discussion on this topic is here: #628

I don't think so. My PR just covers the script to generate the metadata. Algorithms I'm referring to are just used to compute the signature and the digest of the whole metadata (to check its integrity and authenticity). The mechanism to advertise supported algorithms (for SAML signature and assertions encryption) remains untouched.

I also saw that you have also bringed some commits from another branch of mine (those with allow_create documentation... already merged by c00kieMon5ter here: https://github.com/IdentityPython/pysaml2/pull/632/files).

Don't know why it happened... Anyway, I rebased it!

If you can we could work on this in that PR, close this one.

As I said, topics are similar but different. So, I would keep the PR opened.

We should move to a configuration asset that permits users to disable weak alghorithms not only in the metadata but also during signature/encryption verification.

This makes sense

Nice shoot

😄

@peppelinux peppelinux force-pushed the feat/allow-to-choose-metadata-algs branch from b6a5322 to 027d8d2 Compare December 15, 2020 16:15
@vladimir-mencl-eresearch
Copy link
Contributor

Hi,
Given that metadata generation can also be invoked within an application (e.g., in SATOSA at /Saml2/proxy_saml2_backend.xml), it might be better to specify the algorithms in the config, instead of extra flags passed to make_metadata.py.

If all input information needed for generating the metadata is captured in the config, then all the different ways of triggering the metadata generation would produce the same output.

How does this sound?

Cheers,
Vlad

@peppelinux
Copy link
Member

Good to me!

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.

4 participants