Re: [Patch] mod_crypto: A general purpose crypto filter with HLS support

[Graham Leggett]

On 12 Dec 2014, at 12:59 AM, Yann Ylavic  wrote:

>> Incorporating the above, How about this?
> 
> Looks fine, modulo the above (didn't figure out before):

All of this has been included.

I’ve committed this in r1752099 so that further development can occur on trunk.

Regards,
Graham
—

Re: [Patch] mod_crypto: A general purpose crypto filter with HLS support

[Graham Leggett]

On 10 Dec 2014, at 12:22 AM, Yann Ylavic ylavic@gmail.com wrote:

 +rv = ap_get_brigade(f-next, ctx-tmp, mode, block,
 ctx-remaining);
 +
 
 But if you want to keep the following apr_bucket_read() nonblocking,
 you could do :
 
 if (APR_BRIGADE_EMPTY(ctx-tmp) {
rv = ap_get_brigade(f-next, ctx-tmp, mode, block, ctx-remaining);
 }
 
 and then below :
 
 rv = apr_bucket_read(e, data, size, block);
 if (APR_STATUS_IS_EAGAIN(rv)) {
if (APR_BRIGADE_EMPTY(ctx-bb) {
return rv;
}
break:
 }
 if (APR_SUCCESS != rv) {
return rv;
 }
 do_crypto(f, (unsigned char *) data, size, 0);
 ...

It has always seemed wrong to me to assume what is in the brigade from an 
earlier filter (ie that the bucket will be memory resident and therefore non 
blocking).

Incorporating the above, How about this?

Regards,
Graham
—


httpd-mod_crypto-3.patch
Description: Binary data

Re: [Patch] mod_crypto: A general purpose crypto filter with HLS support

[Yann Ylavic]

On Thu, Dec 11, 2014 at 3:31 PM, Graham Leggett minf...@sharp.fm wrote:
[]
 It has always seemed wrong to me to assume what is in the brigade from an 
 earlier filter (ie that the bucket will be memory resident and therefore non 
 blocking).

Input filters' passed-in brigades usually have the lifetime of the
underlying connection/request.
On the client side, that's not an issue, everything is over when
c-pool is destroyed, and ctx-tmp will never be used.
On the backend side (should this filter be used that way), the proxy
module is supposed take care of the passed-out brigades' lifetime,
since f-r and f-c (hence ctx-tmp) may last shorter than the client
side, still if no one has asked for this data before they are
destroyed, they are lost and won't be used anymore either.

Ideally we should use bb-bucket_alloc (and, as request_rec, something
like bb-baton if it had existed :) ) instead of f-r and
f-c-bucket_alloc for creating ctx-bb/tmp, so that mod_proxy would
not need to setaside/transform buckets' lifetime when forwarding
responses, but that's probably another story...


 Incorporating the above, How about this?

Looks fine, modulo the above (didn't figure out before):

+static apr_status_t crypto_in_filter(ap_filter_t *f, apr_bucket_brigade *bb,
+ap_input_mode_t mode, apr_read_type_e block, apr_off_t readbytes)
+{
[]
+
+/* if our buffer is empty, read off the network until the buffer is full */
+if (APR_BRIGADE_EMPTY(ctx-bb)) {
[]
+
+while (!ctx-seen_eos  ctx-remaining  0) {
[]
+/* if an error was received, bail out now. If the error is
+ * EAGAIN and we have not yet seen an EOS, we will definitely
+ * be called again, at which point we will send our buffered
+ * data. Instead of sending EAGAIN, some filters return an
+ * empty brigade instead when data is not yet available. In
+ * this case, we drop through and pass buffered data, if any.
+ */
+if (APR_STATUS_IS_EAGAIN(rv)) {
+   if (APR_BRIGADE_EMPTY(ctx-bb)) {
+   return rv;
+   }
+   break;
+}

Maybe :
if (APR_STATUS_IS_EAGAIN(rv)
|| (sv == APR_SUCCESS
 block == APR_NONBLOCK_READ
 APR_BRIGADE_EMPTY(ctx-tmp))) {
   if (APR_BRIGADE_EMPTY(ctx-bb)) {
   return rv;
   }
   break;
}
to be consitent with the comment.

+if (APR_SUCCESS != rv) {
+   return rv;
+}
+
+for (e = APR_BRIGADE_FIRST(ctx-tmp);
+e != APR_BRIGADE_SENTINEL(ctx-tmp);
+e = APR_BUCKET_NEXT(e)) {

while (!APR_BRIGADE_EMPTY(ctx-tmp)) {
e = APR_BRIGADE_FIRST(ctx-tmp);

since we can't use APR_BUCKET_NEXT(e) after apr_bucket_delete(e) below.

[]
+
+apr_bucket_delete(e);
+
+}
+}
+}
+
[]
+
+return rv;

Here we may still have APR_STATUS_IS_EAGAIN(rv), but some buffered
stuff to return, so :
return APR_SUCCESS;

+}

Regards,
Yann.

Re: [Patch] mod_crypto: A general purpose crypto filter with HLS support

[Graham Leggett]

On 01 Dec 2014, at 7:45 PM, Yann Ylavic ylavic@gmail.com wrote:

 OK, the other way around then (I thought osize was the block size), so
 it can read more bytes than asked to.
 This may also be unexpected by the caller...
 
 At least for the nonblocking case, I think we should return what we
 have so far (if any).

Something like this?

Regards,
Graham
—


httpd-mod_crypto-2.patch
Description: Binary data

Re: [Patch] mod_crypto: A general purpose crypto filter with HLS support

[Yann Ylavic]

Hi Graham,

On Tue, Dec 9, 2014 at 5:50 PM, Graham Leggett minf...@sharp.fm wrote:
 On 01 Dec 2014, at 7:45 PM, Yann Ylavic ylavic@gmail.com wrote:

 OK, the other way around then (I thought osize was the block size), so
 it can read more bytes than asked to.
 This may also be unexpected by the caller...

 At least for the nonblocking case, I think we should return what we
 have so far (if any).

 Something like this?

Regarding the input filter wrt nonblocking, I'd rather do it this way
(modifications inline) :

+static apr_status_t crypto_in_filter(ap_filter_t *f, apr_bucket_brigade *bb,
+ap_input_mode_t mode, apr_read_type_e block, apr_off_t readbytes)
+{
+apr_bucket *e, *after;
+apr_status_t rv = APR_SUCCESS;
+crypto_ctx *ctx = f-ctx;
+
+if (!ctx-tmp) {
+ctx-tmp = apr_brigade_create(f-r-pool, f-c-bucket_alloc);
+}
+
+/* just get out of the way of things we don't want. */
+if (mode != AP_MODE_READBYTES) {
+return ap_get_brigade(f-next, bb, mode, block, readbytes);
+}
+
+/* if our buffer is empty, read off the network until the buffer is full */
+if (APR_BRIGADE_EMPTY(ctx-bb)) {
+ctx-remaining = ctx-osize;
+ctx-written = 0;
+
+while (!ctx-seen_eos  ctx-remaining  0) {
+const char *data;
+apr_size_t size = 0;
+
+rv = ap_get_brigade(f-next, ctx-tmp, mode, block,
ctx-remaining);
+
+/* if an error was received, bail out now. If the error is
+ * EAGAIN and we have not yet seen an EOS, we will definitely
+ * be called again, at which point we will send our buffered
+ * data. Instead of sending EAGAIN, some filters return an
+ * empty brigade instead when data is not yet available. In
+ * this case, we drop through and pass buffered data, if any.
+ */
+if (!APR_STATUS_IS_EAGAIN(rv)  APR_SUCCESS != rv) {
+return rv;
+}

Here:
if (APR_STATUS_IS_EAGAIN(rv) || APR_BRIGADE_EMPTY(ctx-tmp)) {
if (APR_BRIGADE_EMPTY(ctx-bb)) {
return rv:
}
break;
}
if (APR_SUCCESS != rv) {
return rv;
}

+
+for (e = APR_BRIGADE_FIRST(ctx-tmp);
+e != APR_BRIGADE_SENTINEL(ctx-tmp);
+e = APR_BUCKET_NEXT(e)) {
+
+/* if we see an EOS, we are done */
+if (APR_BUCKET_IS_EOS(e)) {
+
+/* handle any leftovers */
+do_crypto(f, NULL, 0, 1);
+apr_brigade_write(ctx-bb, NULL, NULL,
+(const char *) ctx-out, ctx-written);
+
+APR_BUCKET_REMOVE(e);
+APR_BRIGADE_INSERT_TAIL(ctx-bb, e);
+ctx-seen_eos = 1;
+break;
+}
+
+/* flush buckets clear the buffer */
+if (APR_BUCKET_IS_FLUSH(e)) {
+APR_BUCKET_REMOVE(e);
+APR_BRIGADE_INSERT_TAIL(ctx-bb, e);
+break;
+}
+
+/* pass metadata buckets through */
+if (APR_BUCKET_IS_METADATA(e)) {
+APR_BUCKET_REMOVE(e);
+APR_BRIGADE_INSERT_TAIL(ctx-bb, e);
+continue;
+}
+
+/* read the bucket in, pack it into the buffer */
+rv = apr_bucket_read(e, data, size, block);
+if (APR_SUCCESS == rv || APR_STATUS_IS_EAGAIN(rv)) {

And here instead :
rv = apr_bucket_read(e, data, size, APR_BLOCK_READ);
if (APR_SUCCESS == rv) {
since nothing should block (likely not a socket or pipe bucket).
Otherwise we'd have to keep this bucket to restart from it on the next call...

+
+do_crypto(f, (unsigned char *) data, size, 0);
+if (!ctx-remaining || APR_STATUS_IS_EAGAIN(rv)) {
+apr_brigade_write(ctx-bb, NULL, NULL,
+(const char *) ctx-out, ctx-written);
+}
+
+apr_bucket_delete(e);
+}
+else {
+return rv;
+}
+
+}
+}
+}
+
+/* give the caller the data they asked for from the buffer */
+apr_brigade_partition(ctx-bb, readbytes, after);
+e = APR_BRIGADE_FIRST(ctx-bb);
+while (e != after) {
+if (APR_BUCKET_IS_EOS(e)) {
+/* last bucket read, step out of the way */
+ap_remove_input_filter(f);
+}
+APR_BUCKET_REMOVE(e);
+APR_BRIGADE_INSERT_TAIL(bb, e);
+e = APR_BRIGADE_FIRST(ctx-bb);
+}
+
+/* clear the content length */
+if (!ctx-clength) {
+ctx-clength = 1;
+apr_table_unset(f-r-headers_in, 

Re: [Patch] mod_crypto: A general purpose crypto filter with HLS support

[Yann Ylavic]

On Tue, Dec 9, 2014 at 10:56 PM, Yann Ylavic ylavic@gmail.com wrote:
 +static apr_status_t crypto_in_filter(ap_filter_t *f, apr_bucket_brigade *bb,
 +ap_input_mode_t mode, apr_read_type_e block, apr_off_t readbytes)
 +{
[]
 +/* if our buffer is empty, read off the network until the buffer is full 
 */
 +if (APR_BRIGADE_EMPTY(ctx-bb)) {
[]
 +while (!ctx-seen_eos  ctx-remaining  0) {
 +const char *data;
 +apr_size_t size = 0;
 +

Also I think we should :
apr_brigade_cleanup(ctx-tmp);
here.

 +rv = ap_get_brigade(f-next, ctx-tmp, mode, block,
 ctx-remaining);
 +

Re: [Patch] mod_crypto: A general purpose crypto filter with HLS support

[Yann Ylavic]

On Tue, Dec 9, 2014 at 11:03 PM, Yann Ylavic ylavic@gmail.com wrote:
 On Tue, Dec 9, 2014 at 10:56 PM, Yann Ylavic ylavic@gmail.com wrote:
 +static apr_status_t crypto_in_filter(ap_filter_t *f, apr_bucket_brigade *bb,
 +ap_input_mode_t mode, apr_read_type_e block, apr_off_t readbytes)
 +{
 []
 +/* if our buffer is empty, read off the network until the buffer is 
 full */
 +if (APR_BRIGADE_EMPTY(ctx-bb)) {
 []
 +while (!ctx-seen_eos  ctx-remaining  0) {
 +const char *data;
 +apr_size_t size = 0;
 +

 Also I think we should :
 apr_brigade_cleanup(ctx-tmp);
 here.

Hmm, no, this one seems not necessary.

 +rv = ap_get_brigade(f-next, ctx-tmp, mode, block,
 ctx-remaining);
 +

But if you want to keep the following apr_bucket_read() nonblocking,
you could do :

if (APR_BRIGADE_EMPTY(ctx-tmp) {
rv = ap_get_brigade(f-next, ctx-tmp, mode, block, ctx-remaining);
}

and then below :

rv = apr_bucket_read(e, data, size, block);
if (APR_STATUS_IS_EAGAIN(rv)) {
if (APR_BRIGADE_EMPTY(ctx-bb) {
return rv;
}
break:
}
if (APR_SUCCESS != rv) {
return rv;
}
do_crypto(f, (unsigned char *) data, size, 0);
...

Re: [Patch] mod_crypto: A general purpose crypto filter with HLS support

[Yann Ylavic]

Hi Graham,

nice module, looks very useful.

A few (first glance) questions :

- The input filter seems to read and return blocksize bytes at once,
couldn't it read up to readbytes - reabytes % blocksize, or even
readbytes with retained buckets?
It seems that buffering (at most blocksize[ - 1]) would benefit the
output filter too (FLUSH).

- The IV length seems to be forcibly corresponding to the cipher's
blocksize, this is not applicable to all ciphers though.

- The following is used several times in exec_pass_conf_binary() and
looks buggy :

+if (len  size) {
+b = apr_palloc(r-pool, size);
+memset(b, 0, size - len);
+[fn](b + size - len, arg, strlen(arg));
+}
+else {
+b = apr_palloc(r-pool, len);
+[fn](b, arg, strlen(arg), 1,
+NULL);
+b += size - len;

size - len is = 0 here, maybe len - size?
Also, maybe allocate size bytes only since the first len - size are ignored.
Finally, when len != size, why not use a key-type passphrase? (that
would probably better be configurable though).

+}
+*k = b;

Regards,
Yann.


On Mon, Dec 1, 2014 at 2:02 AM, Graham Leggett minf...@sharp.fm wrote:
 Hi all,

 I have attached a proof of concept module that teaches httpd to support 
 symmetrical encryption, initially to support on-the-fly HLS encryption for 
 video streaming.

 This requires the apr-crypto-secretkey patch that I just posted to the APR 
 list.

 This module also potentially solves a problem like this one: 
 http://serverfault.com/questions/372588/decrypting-aes-files-in-an-apache-module

 Regards,
 Graham
 —

Re: [Patch] mod_crypto: A general purpose crypto filter with HLS support

[Yann Ylavic]

On Mon, Dec 1, 2014 at 11:03 AM, Yann Ylavic ylavic@gmail.com wrote:
 A few (first glance) questions :

 - The input filter seems to read and return blocksize bytes at once,
 couldn't it read up to readbytes - reabytes % blocksize, or even
 readbytes with retained buckets?
 It seems that buffering (at most blocksize[ - 1]) would benefit the
 output filter too (FLUSH).

 - The IV length seems to be forcibly corresponding to the cipher's
 blocksize, this is not applicable to all ciphers though.

 - The following is used several times in exec_pass_conf_binary() and
 looks buggy :

 +if (len  size) {
 +b = apr_palloc(r-pool, size);
 +memset(b, 0, size - len);
 +[fn](b + size - len, arg, strlen(arg));
 +}
 +else {
 +b = apr_palloc(r-pool, len);
 +[fn](b, arg, strlen(arg), 1,
 +NULL);
 +b += size - len;

 size - len is = 0 here, maybe len - size?
 Also, maybe allocate size bytes only since the first len - size are ignored.
 Finally, when len != size, why not use a key-type passphrase? (that
 would probably better be configurable though).

 +}
 +*k = b;

Sorry, I forgot one question :
- What is the purpose of the handler returning the secret key?
Get it through a SSL channel for example?
At first glance it looks quite scary ;)


 Regards,
 Yann.

Re: [Patch] mod_crypto: A general purpose crypto filter with HLS support

[Graham Leggett]

On 01 Dec 2014, at 12:11 PM, Yann Ylavic ylavic@gmail.com wrote:

 Sorry, I forgot one question :
 - What is the purpose of the handler returning the secret key?
 Get it through a SSL channel for example?
 At first glance it looks quite scary ;)

In HLS, the key is protected via some kind of login mechanism, and is handed 
out to authorised people over a secure channel. This handler makes this easy to 
serve that key, particularly if the key is derived via the expression API.

The key is provided by a hook, so you could extend the module to support 
rotating keys and other enhancements if the expression API isn’t enough.

Regards,
Graham
—

Re: [Patch] mod_crypto: A general purpose crypto filter with HLS support

[Yann Ylavic]

On Mon, Dec 1, 2014 at 4:08 PM, Graham Leggett minf...@sharp.fm wrote:
 On 01 Dec 2014, at 12:03 PM, Yann Ylavic ylavic@gmail.com wrote:

 - The input filter seems to read and return blocksize bytes at once,
 couldn't it read up to readbytes - reabytes % blocksize, or even
 readbytes with retained buckets?
 It seems that buffering (at most blocksize[ - 1]) would benefit the
 output filter too (FLUSH).

 We buffer up to CryptoSize bytes (default: 128k) on both the input and output 
 filters, there is quite a song and dance to get this right.

OK, the other way around then (I thought osize was the block size), so
it can read more bytes than asked to.
This may also be unexpected by the caller...

At least for the nonblocking case, I think we should return what we
have so far (if any).