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

restructure certs/keys; add key/cert generation #18

Merged
merged 2 commits into from
Nov 19, 2014

Conversation

lunixbochs
Copy link
Contributor

I moved this pull request into a branch so I can mash multiple changes into my master.

I split pem.go into key.go and cert.go, and added methods for manipulating certificates and generating RSA keys. This provides enough functionality to generate a RSA keys, make a CA, and issue a certificate with constraints. cert_test.go demonstrates this process.

NID is also now an exposed type.

@lunixbochs
Copy link
Contributor Author

I'm bolting the GC patch onto this pull request because it depends on my Cert/Key reorganization.

@jtolio
Copy link
Member

jtolio commented Nov 13, 2014

thanks for all your work on this! this is looking fantastic, just a few (hopefully) minor things

@lunixbochs lunixbochs force-pushed the keygen branch 2 times, most recently from e5a8fe2 to 1ecbe19 Compare November 13, 2014 23:46
@lunixbochs
Copy link
Contributor Author

New commits up with the comments addressed.

@lunixbochs
Copy link
Contributor Author

Fresh commits addressing the new comments.

@lunixbochs
Copy link
Contributor Author

Anything else I need to fix on this? I'm working on another branch that depends on this PR and it'll be cleaner if I don't just keep tacking on commits here.

@jtolio
Copy link
Member

jtolio commented Nov 15, 2014

whoops sorry, spaced looking at it, looking now

@jtolio
Copy link
Member

jtolio commented Nov 15, 2014

minor nits, sorry i'm so bad at getting them all the first time through. i think aside from the missing free these three are ready to go.

@lunixbochs lunixbochs force-pushed the keygen branch 2 times, most recently from 7a3252f to 8def9ca Compare November 15, 2014 01:08
@lunixbochs
Copy link
Contributor Author

New commits up, but the ca.Sign() test started failing sometime in the past day. Looking into it.

@lunixbochs
Copy link
Contributor Author

Okay, so there's a bigger problem. X509_sign() had a return value > 1 (2 I think), which wasn't an error. Should we standardize on errors being <= 0? Do any OpenSSL functions return <= -1?

@lunixbochs
Copy link
Contributor Author

I did some digging. It's much safer to compare against 0 on all the functions I checked (every function I changed in this patch), than to compare against 1.

Some of the calls return pointers, lengths, etc. I'm dropping the commit, and this should be safe to merge otherwise.

@lunixbochs
Copy link
Contributor Author

I looked up pretty much all of the OpenSSL functions we're checking for error values and categorized them by which values they return: https://gist.github.com/lunixbochs/33aa6ebba56981d94a9c

It actually looks like some of our calls are erroneously using != 1 where they should be using == 0. Unless there are any instances you know where OpenSSL will return a number < 0 or != 0 for error, I move OpenSSL error handling should be globally changed to if ret == 0 and if ret == nil.

@lunixbochs
Copy link
Contributor Author

Based on this, I've added a patch to standardize returns on == 0 instead of != 1. I've manually checked every single function affected, and made sure it used 0 for returning error values.

@jtolio
Copy link
Member

jtolio commented Nov 15, 2014

i think the main trouble is that openssl return values are inconsistent about what means success. there have been some high profile security vulnerabilities based on assumptions around return codes. i don't think we'll find a broad policy that works in every case, but my overarching desire is to be as strict as possible in each case about what means success

sorry, i've been and am away from my computer, but i'll hop on later to try and help sort out the return code thing. in the mean time let's leave otherwise pr-untouched functions as-is

@jtolio
Copy link
Member

jtolio commented Nov 15, 2014

in cases of length, maybe we should check for >0 as success? some negative return values are undocumented. thanks for putting the gist together

@jtolio
Copy link
Member

jtolio commented Nov 15, 2014

actually, i changed my mind about leaving stuff as-is. this gist is really thorough. if you could just make sure negative values are treated as errors too i'd be happy

@lunixbochs
Copy link
Contributor Author

I switched to <= 0 and 0 >=, tests pass.

@lunixbochs
Copy link
Contributor Author

I have a fix for #4 waiting to PR once this lands, though I should probably write a test for it.

@jtolio
Copy link
Member

jtolio commented Nov 19, 2014

sorry to be so slow on getting to you on these things, and sorry to be so picky on a topic that is fundamentally not really your main motivation on this issue.

@jtolio
Copy link
Member

jtolio commented Nov 19, 2014

basically, i want to be as strict as possible in interpreting success

NID is also now an exposed type
@lunixbochs
Copy link
Contributor Author

I'm pulling the extra commit and only changing return handling for functions I touched. Here's the diff: https://gist.github.com/lunixbochs/49fc48fad6e02bac7703

jtolio added a commit that referenced this pull request Nov 19, 2014
restructure certs/keys; add key/cert generation
@jtolio jtolio merged commit a6e28b4 into spacemonkeygo:master Nov 19, 2014
@jtolio
Copy link
Member

jtolio commented Nov 19, 2014

thank you so much for being so patient with me. this is great, and you jumped through way more hoops than i should have put you through. :)

merlin-northern pushed a commit to merlin-northern/openssl that referenced this pull request Aug 26, 2020
MEN-3877: Protect dynamic c structures from accidental free
nathan454 pushed a commit to nathan454/openssl that referenced this pull request Nov 30, 2022
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.

2 participants