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

crypto: implement privateEncrypt/publicDecrypt #625

Closed
wants to merge 2 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Jan 27, 2015

Fix #477

@indutny indutny added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 27, 2015
@indutny
Copy link
Member Author

indutny commented Jan 27, 2015

cc @bnoordhuis

@@ -3603,6 +3603,12 @@ bool PublicKeyCipher::Cipher(const char* key_pem,
if (EVP_PKEY_CTX_set_rsa_padding(ctx, padding) <= 0)
goto exit;

/* Reset md for signatures, we are doing raw RSA ops anyway */
if (EVP_PKEY_CTX_get_operation(ctx) == EVP_PKEY_OP_SIGN) {
if (EVP_PKEY_CTX_set_signature_md(ctx, nullptr) <= 0)
Copy link
Member

Choose a reason for hiding this comment

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

I admit I don't understand why it's necessary to clear the signature hash function. Can you explain? Preferably with a longer, in-code comment? :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops forgot to remove it, it is no longer needed. :) This piece was failing because PSS padding mode set sha1 md and rsa_pmeth.c was ensuring that the input length matches the md output length. However, PSS does not really work with all these shit, so I replaced it with PKCS1, and PKCS1 does not set md.

@bnoordhuis
Copy link
Member

LGTM if the CI is happy.

@indutny
Copy link
Member Author

indutny commented Jan 27, 2015

Landed in 43c127c, thank you!

@indutny indutny closed this Jan 27, 2015
@indutny indutny deleted the fix/gh-477 branch January 27, 2015 23:02
indutny added a commit that referenced this pull request Jan 27, 2015
PR-URL: #625
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Fix #477
@indutny
Copy link
Member Author

indutny commented Jan 27, 2015

Arrgh, in 87e62bd

@indutny
Copy link
Member Author

indutny commented Jan 27, 2015

@bnoordhuis this is a new feature, I wonder if we should do v1.1.x with it? cc @rvagg

@rvagg
Copy link
Member

rvagg commented Jan 27, 2015

Yes, we have a bunch of stuff queued up with semver-minor and it'd be great to land a bunch of them. Got a proposal for how we handle that? Do we just merge them in to v1.x and jump straight to 1.1.0 or do we make a branch so we can test stuff out first? I think that we're inevitably going to have at least some batching of semver-minor merges into the future so having a solid process in place for doing that would be nice.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 27, 2015

Since minor version changes shouldn't be breaking, we should be able to merge and bump the version. If we wanted to test it out first, would it be much effort to branch and release as nightlies?

@indutny
Copy link
Member Author

indutny commented Jan 27, 2015

@cjihrig I totally agree with you on this. @rvagg plus, I already merged this patch :)

@rvagg
Copy link
Member

rvagg commented Jan 27, 2015

ok then! 1.1.0 it is, we'd best revisit all those semver-minor patches and see what can/should be merged.

@cjihrig the situation I wouldn't mind avoiding is that we have so many feature additions staggered that we're always bumping minor, hence the desire for a bit of batching so we have a chance to get patch releases out and the list of features doesn't feel so unstable to users. This is just my personal opinion of course and I know others feel differently. And, yes it's easy enough to do nightlies, perhaps the best process for now is to allow at least a couple of days after merging semver-minor additions before cutting a release, to allow people to test things in nightlies.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 27, 2015

@rvagg I agree that we should batch minor bumps. I guess at least two questions would be 1) how long we should wait between minor bumps, and 2) how long to leave them in nightlies (is a week too long considering we've had multiple releases in less than that before). Maybe something for tomorrow's TC meeting?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants