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

Add bindings for CZMQ security primitives #125

Merged
merged 5 commits into from
Oct 8, 2014
Merged

Add bindings for CZMQ security primitives #125

merged 5 commits into from
Oct 8, 2014

Conversation

phuedx
Copy link
Contributor

@phuedx phuedx commented May 4, 2014

Add bindings for CZMQ security primitives

This PR adds bindings for the ZeroMQ security primitives, zcert, and zauth (exposed as ZMQCert and ZMQAuth respectively), which are introduced in @hintjens' Using ZeroMQ Security part 1 and part 2.

I've also converted all but the last of the examples from part 2 to PHP (see grasslands.php, strawhouse.php, woodhouse.php, stonehouse.php, and ironhouse.php in the examples directory).

Breaking changes

None.

Notes

@@ -36,6 +36,7 @@
#include "main/php_ini.h"

#include <zmq.h>
#include <czmq.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this should be a conditional dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely. I wouldn't remove the WIP tag without that being the case. First though, I'm going to focus on the ZMQAuth and ZMQCurve classes.

@mkoppanen
Copy link
Collaborator

Added some comments!

@phuedx
Copy link
Contributor Author

phuedx commented May 20, 2014

I'll remove the WIP status when I've documented the ZMQCert and ZMQAuth classes in api.php and answered your question about the return value of zcert_public_txt (and similar functions in CZMQ).

@phuedx phuedx changed the title [WIP] Add bindings for ZeroMQ security primitives [WIP] Add bindings for CZMQ security primitives May 21, 2014
@phuedx phuedx changed the title [WIP] Add bindings for CZMQ security primitives Add bindings for CZMQ security primitives May 25, 2014
@phuedx
Copy link
Contributor Author

phuedx commented May 28, 2014

ZMQCert#__construct now throws when zcert_new returns NULL, which is indicative of libsodium not being installed.

@phuedx
Copy link
Contributor Author

phuedx commented Jun 27, 2014

git merge authApiExperiment WFM. Where are the merge conflicts? Show 'em to me!

@wysman
Copy link
Contributor

wysman commented Jun 30, 2014

Merge conflicts are in zmq.c at the top of the file, near

  • #ifdef PHP_ZMQ_PTHREADS
  • #ifdef HAVE_CZMQ_2

Maybe add a empty line can help

@phuedx
Copy link
Contributor Author

phuedx commented Jun 30, 2014

Thanks @wysman!

@wysman
Copy link
Contributor

wysman commented Jul 1, 2014

Why do you use shadow context in ZMQAuth, and not directly the real context ?

@wysman
Copy link
Contributor

wysman commented Jul 1, 2014

I have a issue on PHP 5.3.10
The macro "#define object_properties_init(zo, class_type)" is define after the use of it in :

  • php_zmq_cert_new
  • php_zmq_auth_new

It's produce a "undefined symbol: object_properties_init" at execution time.
Move the macro just after #include on the top of file will fix it

@wysman
Copy link
Contributor

wysman commented Jul 1, 2014

In the private header,
You test that the major version is upper 2, but the actual version of CZMQ is 2, so ZMQCert & ZMQAuth are not compiled

@phuedx
Copy link
Contributor Author

phuedx commented Jul 4, 2014

Thank you for your review @wysman. I think that I've addressed your last two points.

@phuedx
Copy link
Contributor Author

phuedx commented Jul 4, 2014

@wysman As for your first point: rather than acting on typeless pointers, which libzmq uses, the zauth API accepts only zctx_t* (see the zctx API). At the time of writing, creating a shadow context with the zctx_shadow_zmq_ctx function was the only method I saw of bridging the APIs.

<?php

$cert = new ZMQCert();
$cert->save('/tmp/cert');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would not be portable. Can you use DIR instead?

this = (php_zmq_cert *) zend_object_store_get_object(getThis() TSRMLS_CC);
public_key = zcert_public_key(this->zcert);

RETURN_STRINGL((char *) public_key, 32, 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

does public_key need to be freed here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

zcert_public_key returns a pointer to the associated property of the zcert_t struct. With that in mind, I don't think public_key should be freed (and RETURN_STRINGL(..., 1) is correct). This is also the case for zcert_secret_key, zcert_public_txt, and zcert_secret_txt.

@phuedx
Copy link
Contributor Author

phuedx commented Sep 29, 2014

Sorry for the delay in addressing your review @mkoppanen. I think I've covered it all.

// TODO (phuedx, 2014-05-16): CZMQ now supports GSSAPI (see
// zauth_configure_gssapi).

default:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unknown auth type should probably throw an exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@mkoppanen
Copy link
Collaborator

Added two comments. Minor changes and after that I am happy to merge! Amazing work

The ZMQCert class wraps the CZMQ zcert API.

Instances of the ZMQCert class can:

* have their public and private text inspected, either as 32-byte binary
  strings or Z85 armoured strings
* be tested for equality
* be cloned
* store and retrieve metadata
* have their public and private test saved to and loaded from disk
  be applied to a ZMQSocket
The ZMQAuth class wraps the CZMQ zauth API.

Instances of the ZMQAuth class can:

* install a ZAP handler for a ZMQContext
* whitelist/blacklist IP addresses
* use plain or curve authentication for one or more domains
grasslands.php, strawhouse.php, woodhouse.php, stonehouse.php, and
ironhouse.php are faithful translations of Pieter Hintjens' examples of
the same names, which he originally published as part of his blog post
Using ZeroMQ Security (part 2)[0].

[0] http://hintjens.com/blog:49
@phuedx
Copy link
Contributor Author

phuedx commented Oct 8, 2014

Thanks @mkoppanen! I also removed a few erroneous errnos – say that three times fast – that were still in the ZMQCert and ZMQAuth code.

@mkoppanen
Copy link
Collaborator

Waiting for travis to finish and then merging. Looking all green this far!

@mkoppanen
Copy link
Collaborator

travis/script.sh: line 187: test: =: unary operator expected

@mkoppanen
Copy link
Collaborator

Seems like we have one failure:

https://travis-ci.org/mkoppanen/php-zmq/builds/37392813

@phuedx
Copy link
Contributor Author

phuedx commented Oct 8, 2014

I'll fix up travis/script.sh in another PR as it's not affecting the build result, it's just warning that = "true" isn't valid syntax.

mkoppanen added a commit that referenced this pull request Oct 8, 2014
Add bindings for CZMQ security primitives
@mkoppanen mkoppanen merged commit 644fa8e into zeromq:master Oct 8, 2014
@mkoppanen
Copy link
Collaborator

Merged. Thanks!

@wysman
Copy link
Contributor

wysman commented Oct 8, 2014

Congrats !

@phuedx
Copy link
Contributor Author

phuedx commented Oct 8, 2014

YEAH!!!1one

@phuedx phuedx deleted the authApiExperiment branch October 8, 2014 13:49
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.

3 participants