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

add method to import PEM formatted PKCS8 keys #64

Merged
merged 2 commits into from
Jan 29, 2020

Conversation

Goirad
Copy link
Contributor

@Goirad Goirad commented Jan 20, 2020

I have added a method to import PEM formatted keys. Ideally, the other method would be changed to import_pkcs8_der, but that would require a major version bump.

Also of note, windows does not validate the PEM headers, and only checks that the header matches the footer. It is unclear to me whether this crate is the place to add that logic, or whether it would be sufficient to add rudimentary checks on the input string in the import_pkcs8_pem method.

Another alternative would be add something like a parse_pkcs8_pem method and leave it to the user of this library to call that first before then calling import_pkcs8 on the result. I would be happy to use that approach instead and include the header checking there if that is a preferred structure.

let mut len = 0;
let res = wincrypt::CryptStringToBinaryA(pem.as_ptr() as ntdef::LPCSTR,
pem.len() as winapi::DWORD,
wincrypt::CRYPT_STRING_BASE64_ANY,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be CRYPT_STRING_BASE64HEADER, I think (same below)

Copy link
Owner

Choose a reason for hiding this comment

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

I do not think so, parsing should work for PEM files with & without header.

src/crypt_prov.rs Outdated Show resolved Hide resolved
src/crypt_prov.rs Outdated Show resolved Hide resolved
@steffengy
Copy link
Owner

I think windows only checks if the header begins with a "BEGIN".
I'd think I'd prefer the import_pkcs8_pem approach and to fullfill the contract of the name some rudimentary checks (if it starts with "----BEGIN" it has to be "-----BEGIN PRIVATE KEY-----" and has to end with "-----END PRIVATE KEY-----")
What do you think?

@Goirad
Copy link
Contributor Author

Goirad commented Jan 21, 2020

Sounds good to me, what do you think of renaming import_pkcs8 to import_pkcs8_der?

@jethrogb
Copy link
Contributor

What happens if you try to pass in a PKCS1 key?

@Goirad
Copy link
Contributor Author

Goirad commented Jan 22, 2020

Turns out it rejects PKCS1 keys. I've added a test to that effect. Also it seems tests are failing for unrelated reasons.

@steffengy
Copy link
Owner

Sounds good to me, what do you think of renaming import_pkcs8 to import_pkcs8_der?

I think import_pkcs8_pem is better for consistency reasons. In all places I found we currently have pem explicitly as suffix and omit DER.

@Goirad
Copy link
Contributor Author

Goirad commented Jan 23, 2020

Is there anything else that you want me to change for this PR?

@steffengy
Copy link
Owner

@Goirad It's fine from my side, maybe @sfackler wants some changes before this fits into how you intend to use it in native-tls - atleast that's what I gathered.
(Referencing sfackler/rust-native-tls#147)

@steffengy steffengy merged commit 046ca8c into steffengy:master Jan 29, 2020
@Goirad
Copy link
Contributor Author

Goirad commented Feb 4, 2020

@steffengy Can we get a release?

@steffengy
Copy link
Owner

@Goirad Sure, done.

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