-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
get peer certificates and signatures #8005
Conversation
Could we add |
Yeah I think that makes sense, I’ll give it a shot soon and see what it
looks like.
…On Mon, Jul 29, 2019 at 10:43 Steph ***@***.***> wrote:
Could we add Certificate#signature_algorithm and
Certificate#digest(algorithm) instead and move the SCRAM specific logic
outside the stdlib?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#8005?email_source=notifications&email_token=AAAAPNJLKMCEYRBDFPSMX2TQB4T3VA5CNFSM4IHNH6F2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3BO7WQ#issuecomment-516091866>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAAPNOEXPNYOHHSOABFEF3QB4T3VANCNFSM4IHNH6FQ>
.
|
Updated with it split into two methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of mostly small stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to have a positive spec on `signature_algorithm'.
Maybe this could help: https://github.com/sfackler/rust-openssl/blob/487963d17a71dd7f4b239d7284c623f1c6a0b64b/openssl/src/x509/tests.rs#L303
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is fine to merge.
Maybe add a TODO
in the #signature_algorithm
spec.
@will if the usage of this PR has been tested in crystal-pg, i'm fine to merge for 0.30.0. Others might want to wait for .1, but since a shard depends on this i'm inclined to merge sooner rather than later. |
Darwin specs are failing due to missing symbols:
Since this PR adding and not changing is easy to patch the library until work is done to make the CI in happy. |
@RX14 I have it on a branch for crystal-pg, but it's not merged yet. I wanted to wait for the changes to make it in before committing to it. The branch is over at https://github.com/will/crystal-pg/commits/scram_cbind @bcardiff the darwin failures are strange to me. I develop on mac, and both openssl 1.0.2 and 1.1.0 work fine for me:
|
Is it possible that the mac ci build isn't actually setting up openssl 1.0.2 by exporting pkg_config_path? When I don’t do that on mac everything fails to link at all (even before these changes), so I would have expected more things to fail sooner than this patch, but I don’t see "pkg_config" in the circle yaml. |
The linking is likely succeeding on your laptop because both openssl libraries are making their way into the library search path. |
Hm, the implementation requires getting info out of the X509 struct. There isn't an easy way to do that in crystal except to redefine the entire struct in crystal, right? Like you cant just get one field out. It looks like in 1.0.2, the field is only the second one in https://github.com/openssl/openssl/blob/OpenSSL_1_0_2-stable/crypto/x509/x509.h#L270, and it's in the same spot today https://github.com/openssl/openssl/blob/faa9dcd4d468441422254ab2d887bb267e0245b6/crypto/include/internal/x509_int.h#L161 but not all the other fields are the same. 1.0.2 is from January 22, 2015, what are the chances we can drop support for it? |
Sorry I haven't followed up on this in a while. I'm unsure of what changes to make to move this along, or what I should be doing here. |
There's no chance we can drop old openssl support, you'll probably have to get it from the struct directly, depending on openssl version. |
Rebased this. Curious what the current CI situation will be since the last time. |
CI looks mostly good, but we'll still need support for OpenSSL 1.0.x. Unfortunately we don't have automatic tests set up against older OpenSSL versions. AArch64 times out which doesn't happen in master, so it's likely caused by this change. |
Is this still true? The OpenSSL project itself dropped support for 1.0.2 on 2019-12-31. Also 1.1.0 (though that version works with this patch) was dropped earlier on 2019-09-11. The only supported current OpenSSL version is 1.1.1 which will last until 2023-09-11. If it is still true, is there a timeline for how far beyond the library being unsupported by its maintainers that Crystal will support it? I'm okay doing the work here, but it'd be unfortunate if Crystal has to support dead openssl versions forever. Also would it be sufficient to only have these bindings and functions available for >= 1.1.0 versions? Or does it need to work for 1.0.2 also? I stopped work on this because I couldn’t figure out how to get a single field out of a struct with crystal's c biding support. The only thing I could think of was to manually read some fixed number of bytes offset, which felt gross, but maybe that's the only way.
Do you have any tips on debugging this? |
Even if the openssl project has dropped support, 1.0.x is still the supported version in older distributions like Ubuntu xenial which has LTS til April. Regarding Aarch64, I guess the first step would be to find out where exactly the spec execution breaks. I don't think you can SSH into a Github actions runner. Best to use native hardware for this. I have an Raspberry Pi laying around somewhere, maybe I can take a stab at this. |
I'm taking a stab at it on an ec2 arm instance. I'm not really sure if I'm using docker right or anything, but it seems like I got some of the tests to run and build at least. I'm not sure, but the issue may be
Now have to figure out why my change would have caused that, if that is the actual issue and not a byproduct or something. |
Maybe found it, trying this, but squashed it into the commit that introduced the test diff --git c/spec/std/openssl/ssl/socket_spec.cr i/spec/std/openssl/ssl/socket_spec.cr
index fdf16d686..f4265cd0a 100644
--- c/spec/std/openssl/ssl/socket_spec.cr
+++ i/spec/std/openssl/ssl/socket_spec.cr
@@ -47,7 +47,7 @@ private alias Server = OpenSSL::SSL::Socket::Server
private alias Client = OpenSSL::SSL::Socket::Client
private def socket_test(server_tests, client_tests)
- tcp_server = TCPServer.new(0)
+ tcp_server = TCPServer.new("127.0.0.1", 0)
server_context, client_context = ssl_context_pair
OpenSSL::SSL::Server.open(tcp_server, server_context) do |server| It looks like the localhost address was added, in these tests, and my small test refactor didn't pick it up in the rebase. But not sure why that'd be a problem only on ARM |
I tried building on a clean xenial instance, to avoid the problem I was having earlier with multiple openssl libs on the same machine. However I did not get the same 'cant find symbol' errors that were happening on the old Darwin builds from before. Instead it was these errors, which happened both with and without my patch
maybe support for 1.0.2 is already broken in crystal? I am surprised that the linking error didn't happen though |
Awesome! The error message Side note: Please do not squash and force push fixes for better traceability of the intermediary changes. |
It looks like #9831 already introduced optional features that don't work with openssl < 1.1, so I'd say this PR can be accepted as is. We should probably consider documenting the support levels somewhere, though. Would probably be good to add the required openssl version at least to the method docs. |
In #9831 the policy was to allow functionality that will work only on certain version as long as
I tried to use 1.0.2 via nix and things build fine there. ( In debian:9 de openssl is 1.0.1 and there I got
I guess something similar will happen with libressl, I am not sure. It's fine if we support the propsed features in this PR only with openssl (again, as long as things compile if it's not used) In order to move forward I think the following things are needed.
|
Using libressl (replacing openssl with libressl and
|
To clarify 2. the partially supported methods should either a) warn + return a stub value or b) raise (no need to warn in this case). |
Hi @bcardiff, I just did your suggestions. Sorry this took so long. I was trying to get nix set up having never used it before, then ran into a lot of issues with that, then other irl stuff came up and it fell off my radar. Please let me know if this looks acceptable. |
…r message Co-authored-by: Johannes Müller <[email protected]>
@bcardiff can you please take another look at this? Happy to do any other changes you think if there is anything |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow why the Client#peer_certificate
method is specialized to raise on nil while the base implementation is nilable.
Besides that, on my end the PR can be merged as is.
(Sorry I've been a bit disconnected)
If you're a client, there will always be a peer certificate (the one the server is using), but if you are a server there might not be a peer cert (the client may or may not have one). So I added
No problem, thanks for taking a look. Let me know if you want me to rebase and/or squash down to a single commit or anything. |
We'll squash on merge. Merging master would be good before merging. I can do that myself. |
Thank you @will |
There are two patches here, that go along with doing SCRAM channel binding, with this draft pr in will/crystal-pg#177 using them as monkey patches.
The
OpenSSL::SSL::Socket#peer_certificate
I think for sure makes sense in the standard library, but theOpenSSL::X509::Certificate#signature
, I'm not as sure. The upgrade of MD5 and SHA1 while, required in https://tools.ietf.org/html/rfc5929#section-4.1 is maybe not general purpose. The upgrade could be made optional. Then again maybe nothing else needs these signatures, so I don't know. Also I couldn't figure out how to get a test to go all the way through the method, since I don't see a way to make aOpenSSL::X509::Certificate
with a signature.