Skip to content

Imp work with unencrypted pem certificates#25

Open
blaggacao wants to merge 7 commits intoknowark:masterfrom
xoe-labs:imp-work-with-unencrypted-pem-certificates
Open

Imp work with unencrypted pem certificates#25
blaggacao wants to merge 7 commits intoknowark:masterfrom
xoe-labs:imp-work-with-unencrypted-pem-certificates

Conversation

@blaggacao
Copy link
Copy Markdown
Contributor

@blaggacao blaggacao commented Dec 15, 2018

Instead transform the p12 files into passwordless PEM data, eg:
openssl pkcs12 -in persona_juridica_pruebas_vigente.p12 -out newfile.pem --passin pass:persona_juridica_pruebas -nokeys -clcerts

-nokeys           Don't output private keys
-clcerts          Only output client certificates

That's exactly the certificate as needed and consumed by

It seems that p12 does not work properly. It gives an OpenSSL lib error.

@blaggacao blaggacao force-pushed the imp-work-with-unencrypted-pem-certificates branch 3 times, most recently from 47dda09 to 4fb8178 Compare December 15, 2018 22:00
@blaggacao
Copy link
Copy Markdown
Contributor Author

As it seems Andes pkcs12 certificates are latin-1 encoded, wtf?

@blaggacao blaggacao force-pushed the imp-work-with-unencrypted-pem-certificates branch 3 times, most recently from b3c907a to 339a788 Compare December 16, 2018 00:52
@blaggacao
Copy link
Copy Markdown
Contributor Author

Generating pem cert and key is exactly 2 commands away, for using a better supported standard and dropping OpenSSL, I guess it's worth it.

openssl pkcs12 -in certificate.p12 -out cert.pem -nokeys -clcerts
openssl pkcs12 -in certificate.p12 -out key.pem -nodes -nocerts

As a side-effect, get rid of (deprecated) pyOpenSSL dependency.

Rather provide the library with a standard pem certificate.
Maybe support DER binary format one day?
@blaggacao blaggacao force-pushed the imp-work-with-unencrypted-pem-certificates branch from 339a788 to 70d5411 Compare December 16, 2018 00:58
@blaggacao
Copy link
Copy Markdown
Contributor Author

If there really is a need to support PKCS12 out of this library, let's rather do a wrapper? I feel handling such old format within the interiors of the library does cry for problems...

Copy link
Copy Markdown
Contributor Author

@blaggacao blaggacao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some doubts about what I did...

issuer_name = ', '.join(
attr.rfc4514_string() for attr
in reversed(certificate_object.issuer._attributes)
).encode('ascii')
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tebanep I guessed ascii is the right byte encoding. Can you confirm? Should that go somewhere else? (eg. encoder)

@@ -0,0 +1,29 @@
Bag Attributes
localKeyID: 0E B2 55 CC 89 54 77 79 8B A4 18 B3 78 25 52 04 30 48 97 AD
subject=/C=DE/ST=Bavaria/L=Munich/O=MIT-xperts GmbH/OU=TEST CA/CN=testbox.mit-xperts.com/emailAddress=info@mit-xperts.com
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My hometown 😉

issuer_name = signer._prepare_issuer_name(certificate)
assert issuer_name == (
b'emailAddress=info@mit-xperts.com,CN=itv.mit-xperts.com,'
b'1.2.840.113549.1.9.1=info@mit-xperts.com,CN=itv.mit-xperts.com,'
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This OID seems to be the correct representation of an email adress, this is what is also used by the dian exemplification:

<ds:X509IssuerName>
C=CO,L=Bogota D.C.,O=Andes SCD.,OU=Division de certificacion entidad final,CN=CA ANDES SCD S.A. Clase II,1.2.840.113549.1.9.1=#1614696e666f40616e6465737363642e636f6d2e636f
</ds:X509IssuerName>

@blaggacao
Copy link
Copy Markdown
Contributor Author

Why all this? Because someone like Andes has obscure latin-1 encoding as their .p12 encoding which I guess is not handled gracefully by Odoo Binary fields. With PEM, we handle plain text instead.

@blaggacao blaggacao force-pushed the imp-work-with-unencrypted-pem-certificates branch from 699a53d to 469e08f Compare December 16, 2018 01:44
@blaggacao blaggacao force-pushed the imp-work-with-unencrypted-pem-certificates branch from c87671f to 5a806ec Compare December 16, 2018 02:30
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 16, 2018

Codecov Report

Merging #25 into master will decrease coverage by 0.2%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
- Coverage     100%   99.79%   -0.21%     
==========================================
  Files          94       94              
  Lines        1954     1978      +24     
  Branches       96       99       +3     
==========================================
+ Hits         1954     1974      +20     
- Misses          0        2       +2     
- Partials        0        2       +2
Impacted Files Coverage Δ
facturark/signer/encrypter.py 100% <100%> (ø) ⬆️
facturark/api.py 100% <100%> (ø) ⬆️
facturark/__main__.py 100% <100%> (ø) ⬆️
facturark/signer/resolver.py 100% <100%> (ø) ⬆️
facturark/signer/signer.py 97.64% <90%> (-2.36%) ⬇️

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 50cbf98...5a806ec. Read the comment docs.

@blaggacao
Copy link
Copy Markdown
Contributor Author

@tebanep I guess the test diff is due to the back port from cryptography==2.5, there is no sense at all in covering it with unit tests. It can be just removed after cryptography==2.5 will be released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant