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

[WIP] OpenSSL RSA Bindings #5777

Closed
wants to merge 1 commit into from
Closed

[WIP] OpenSSL RSA Bindings #5777

wants to merge 1 commit into from

Conversation

CImrie
Copy link

@CImrie CImrie commented Mar 6, 2018

Adds RSA bindings to OpenSSL as mentioned in #3941

One slight nuance - it can't export to PEM format using a passphrase as OpenSSL fails without any error when performing this. Otherwise works similarly to the Ruby API but without aliases.


pkey.public_key.public?.should be_true
end
it "can export to PEM format" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Typical style is to treat it blocks like def methods and leave a line between each.

@CImrie CImrie closed this Mar 18, 2018
@straight-shoota
Copy link
Member

Why close this? @CImrie

@CImrie
Copy link
Author

CImrie commented Mar 18, 2018

Hey @straight-shoota - At the moment I've pulled out a shard instead so that issues can be ironed out much quicker (independently from crystal stdlib updates) with the intention of it being merged in later as a PR. Does that seem sensible?

@bararchy
Copy link
Contributor

@CImrie something as critical as SSL/TLS security should reside in the stdlib

@RX14
Copy link
Contributor

RX14 commented Mar 18, 2018

but this isn't SSL/TLS.

Eventually OpenSSL module will go and be put into a shard as per #5215, and we'll just have a small private openssl binding enough to do TLS and hmac/sha1/etc. Common operations only.

I'm happy to merge this though because it'll make pulling out the OpenSSL shard easier.

@straight-shoota
Copy link
Member

Sure. Just wanted to know what's up. @RX14 I'm not sure if it makes sense to merge if it's still work in progress. Could be added later in a new PR either to stdlib or when OpenSSL is extracted to a shard.

@RX14
Copy link
Contributor

RX14 commented Mar 18, 2018

For now, removing OpenSSL is a pipe dream. So i'd rather not refuse PRs that fit into the stdlib as is because of something that might happen in the future. Until someone puts in the effort to refactor out a generic TLS etc. API there's no reason to refuse contributions.

@straight-shoota
Copy link
Member

100% agree. I meant that this contribution is maybe not ready yet. At least that's how I understood @CImrie's statement.

@RX14
Copy link
Contributor

RX14 commented Mar 18, 2018

Ah I see what you mean now. No, I wasn't proposing to merge this PR, I was proposing to merge a future PR once this happens:

with the intention of it being merged in later as a PR.

@bararchy
Copy link
Contributor

bararchy commented Dec 5, 2018

@CImrie what ever happened to this external shard PR ? :)
Do you still work on it?

@CImrie
Copy link
Author

CImrie commented Dec 5, 2018

Hi @bararchy,
I've shelved the project I was working on with crystal so I'm not actively using it at the moment (very much hope to change that again).

I did complete the bindings for what I needed (verifying RSA-signed JWT tokens) but packaged it up in an external shard - https://github.com/randomstate/openssl_ext

Is that of any use to you?

@bararchy
Copy link
Contributor

bararchy commented Dec 5, 2018

@CImrie that's cool to know, I hope we see you in crystal land again soon, I'll go over the project , thanks for your time

@CImrie
Copy link
Author

CImrie commented Dec 5, 2018

@bararchy If you are using it for JWT, you can take a look at https://github.com/randomstate/jwt . You can use that library for verification/validation of the tokens or as inspiration if you need anything more bespoke.

Believe me, I'm trying to find any way I can to start building projects in crystal again 😅

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.

None yet

4 participants