Skip to content

Conversation

ioparaskev
Copy link
Contributor

  • Uses cryptography.x509 load_pem_x509_certificate or
    load_der_x509_certificate depending on the cert type. This ensures 1)
    the certificate is a valid certificate 2) trailing newlines and
    whitespaces will be ignored
  • Adds tests for the above scenarios
  • Ignores cer/crt as certificate type since these are file extensions
    and do not guarrantee the certificate encoding. Uses "pem" as default
    type (mostly for backward compatibility). Only other valid option is
    "der" (everything else falls back to "pem")
  • Uses sigver cert loading function for the metadata cert loading
    (removes read_cert function)
  • Fixes x509 certificate with trailing whitespace doesn't load #659

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?

- Uses x509.load_pem_x509_certificate to read a certificate file. This
  ensures 1) the certificate is a valid certificate 2) trailing newlines
and whitespaces will be ignored
- Adds tests for the above scenarios
Fixes IdentityPython#659
- Uses cryptography.x509 load_pem_x509_certificate or
  load_der_x509_certificate depending on the cert type. This ensures 1)
the certificate is a valid certificate 2) trailing newlines and
whitespaces will be ignored
- Ignores cer/crt as certificate type since these are file extensions
  and do not guarrantee the certificate encoding. Uses "pem" as default
type (mostly for backward compatibility). Only other valid option is
"der" (everything else falls back to "pem")
- Uses sigver cert loading function for the metadata cert loading
  (removes read_cert function)
@codecov
Copy link

codecov bot commented Jan 14, 2020

Codecov Report

Merging #660 into master will increase coverage by <.01%.
The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #660      +/-   ##
==========================================
+ Coverage   65.06%   65.06%   +<.01%     
==========================================
  Files         102      102              
  Lines       25667    25659       -8     
==========================================
- Hits        16700    16696       -4     
+ Misses       8967     8963       -4
Impacted Files Coverage Δ
src/saml2/sigver.py 70.45% <100%> (+0.22%) ⬆️
src/saml2/metadata.py 45.06% <71.42%> (-0.11%) ⬇️

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 f27c7e7...8e05655. Read the comment docs.

@peppelinux
Copy link
Member

peppelinux commented Sep 6, 2020

Is there any possibility to handle also #278 in this PR?

@c00kiemon5ter
Copy link
Member

I don't think we should handle #278 here. I also don't certificate-passwords are widely used. Let's keep this issue about loading certificates only and we can revisit certificate-passwords again in the future in a different issue/PR.

@peppelinux
Copy link
Member

I marked that as closing and then closed It, now I've reopened it again. I have a presentiment that It would be a "won't fix" but my excuse in Advance, I didn't mean to be cynical

@peppelinux
Copy link
Member

peppelinux commented Dec 14, 2020

I pushed this feature here
daf2142#diff-3482e02155ca5e77a0034bd27c342d3951400f11ceea80ed7d2169e8f458f857R105

Probably it would belong to this PR.
Don't know which PR would be merged first, so, let me know your though, eventually I can push it also in this PR

@c00kiemon5ter
Copy link
Member

closed by a924435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-release should become part of the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x509 certificate with trailing whitespace doesn't load
3 participants