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

Adds support for the 'get_finished' and 'get_peer_finished' functions #250

Merged
merged 3 commits into from
Oct 7, 2019

Conversation

movitto
Copy link
Contributor

@movitto movitto commented Apr 29, 2019

Returns latest 'finished' messages sent and received.

@ioquatix
Copy link
Member

Can you rebase on master and explain why these functions are useful?

@ioquatix
Copy link
Member

@movitto maybe add a simple example to the documentation, would be sufficient to explain how to use it and why.

@movitto
Copy link
Contributor Author

movitto commented Jun 17, 2019

@ioquatix thanks for taking a look at this. I just rebased and force pushed, the branch should now be up to date.

We need this to incorporate it in a ruby implementation of the rippled (server behind the XRP blockchain/crypto-currency) peer-communication protocol. During the initial handshake, rippled uses the get_finished and get_peer_finished functions as the basis of a unique "session signature" used to authenticate the session. I can't attest to the design considerations that went into using these functions in that capacity (I don't work for ripple labs, nor was I involved in those discussions) but for the time being we'll need access to it from ruby to complete the handshake.

You can see how we use it in our XRP client library.

Thanks again for looking at this and feel free to let me know if there is anything else I can do to get this in!

@ioquatix
Copy link
Member

I think there is one issue - this won't compile on Windows because even thought we have C99, we don't have support for VLA on Windows compiler (wtf Microsoft).

char buf[len+1];

Can probably be replaced with

char * buffer = ALLOCA_N(char, len+1);

@mame do you mind commenting if this is the right approach?

@@ -2295,6 +2295,54 @@ ossl_ssl_get_verify_result(VALUE self)
return INT2NUM(SSL_get_verify_result(ssl));
}

/*
* call-seq:
* ssl.peer_finished => "finished message"
Copy link
Member

Choose a reason for hiding this comment

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

Rename this to finished_message

if(len == 0)
return Qnil;

char buf[len];
Copy link
Member

Choose a reason for hiding this comment

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

Use ALLOCA_N as suggested.


/*
* call-seq:
* ssl.get_peer_finished => "peer finished message"
Copy link
Member

Choose a reason for hiding this comment

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

Rename this to peer_finished_message

@ioquatix
Copy link
Member

Sorry to make this more complicated for what is a fairly simple feature, but some tests would be great too.

I mentioned in the review should we call this socket.finished or socket.finished_message. To me, it's a bit confusing because finished isn't a noun. That's the only reason why I suggested finished_message.

@movitto
Copy link
Contributor Author

movitto commented Jun 25, 2019

@ioquatix ok cool, I addressed all feedback:

  • 'peer_finished' is now known as 'finished_message'
  • 'get_peer_finished' is now known as 'peer_finished_message'
  • VLAs have been replaced with ALLOCA_N(char, len+1);
  • test has been included verifying equality of client/server finished_message / peer_finished_message (the 'start_server' test helper had to be extended to accept a callback so that we could access server socket)
  • rebased against master and tidy'd up the patchset

Let me know if there is anything else!

@ioquatix
Copy link
Member

If you can rebase this on master and all tests pass I will merge.

Callback will be invoked with new ssl connection upon acceptance
by server. Default is empty proc.
@ioquatix ioquatix merged commit f7fc8c1 into ruby:master Oct 7, 2019
@ioquatix
Copy link
Member

ioquatix commented Oct 7, 2019

Thanks for your effort!

@movitto
Copy link
Contributor Author

movitto commented Oct 7, 2019

@ioquatix thanks for merging!

@movitto movitto deleted the get_finished branch October 7, 2019 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants