add acl feature #1016

i110 · 2016-08-05T03:17:55Z

This adds a feature which makes it easy to write access control handler, as below

mruby.handler |
  acl {
    allow { addr.start_with?("192.168.") }
    deny { ! addr.match?(/^110\.110\./) && user_agent.match(/curl/i) }
    redirect("https://example.com", 301) { path == "/redirect" }
    response(403, {}, ["you are evil!"]) { user_agent.match(/evil/i) }
  }
file.dir: examples/doc_root

H2O::ACL module will be automatically included, which has only one method acl.
Also add a setup to test mruby handlers. If you want to run ruby handler tests, do make mruby-test and execute t/00unit.mruby.t
To avoid miss-configuration, add some checks of return value from configuration. The details are as follows.

When you use acl in a handler code, acl must be called only once, and the return value of the code is exactly acl 's return value (i.e. you must wrap whole handler's code by acl block)

See the following example:

This setting is danger because acl setting is absolutely ignored (so check it and abort if so)

mruby.hanlder |
  acl {
    deny { addr.start_with?("110.110.") } # meaningless
  }
  Htpasswd.new("/path/to/.htpasswd", "realm")

You can correct it as the below.

mruby.hanlder |
  acl {
    deny { addr.start_with?("110.110.") }
    Htpasswd.new("/path/to/.htpasswd", "realm")
  }

or separate it into multple handlers.

mruby.hanlder |
  acl {
    deny { addr.start_with?("110.110.") }
  }
mruby.hanlder |
  Htpasswd.new("/path/to/.htpasswd", "realm")

kazuho · 2016-08-08T01:08:25Z

share/h2o/mruby/acl.rb

+        end
+
+        def satisfy?(env)
+           return @cond.nil? || MatchingBlock.new(env).instance_eval(&@cond)


Is my understanding that satisfy? is called for every request? If so, please move the call to instance_eval to somewhere during the initialization phase.

To generalize, I think we should refrain from calling slow functions (e.g. instance_eval) while processing each request.

As we discussed, we have to call satisfy? method for every requests because whether the request match conditions or not cannot be decided without each request information.
I guess that not so many acl conditions will appear, so it won't be performance issue.

kazuho · 2016-08-08T01:49:26Z

Thank you for the PR.

I like it very much, since the DSL looks very attractive (and easy to use). Please take a look at my in-line comments.

i110 · 2016-08-08T07:37:35Z

lib/handler/configurator/mruby.c

        fprintf(stderr, "%s: no memory\n", H2O_MRUBY_MODULE_NAME);
        abort();
    }
-    int ok = !mrb_nil_p(h2o_mruby_compile_code(mrb, config, errbuf));


ここマクロの中で2回コンパイル走ってた罠

i110 · 2016-08-08T07:41:03Z

@kazuho all issues are fixed.

kazuho · 2016-08-12T00:59:03Z

.gitmodules

 	url = https://github.com/h2o/cache-digest.js.git
+[submodule "misc/mruby-mtest"]
+	path = misc/mruby-mtest
+	url = https://github.com/iij/mruby-mtest.git


Please use misc/dump-github-repository.pl to dump mruby-mtest into misc/mruby-mtest. Run the perl script without arguments to find out how to use the program.

The reason we should do so is because tarballs created by GitHub will not include submodules; files required for building and testing H2O should never be a git-submodule.

kazuho · 2016-08-12T01:14:28Z

Thank you for the updates. I've left two comments in-line, PTAL.

…b8b3bb0dcf2a0966 () at misc/mruby-mtest

i110 · 2016-08-15T03:17:19Z

@kazuho fixed the points you said.

it is unclear to me why we'd want to return values from each directive

Before this fix, acl can use plain callable returned value like the following:

acl {
  deny { path.match/../ }
  allow { addr.start_with?("192.") }
  proc {|env| [200, {}, []] } # this proc will be called when other directives don't match their conditions
}

The covering test cases were: 3346d3c#diff-1f0e213b76131d8d4093df0099c9d190R95

But as you said, it is a bit unclear behavior for the users and makes impl complicated, so I removed this behavior.
Now we can use use directive in such case:

acl {
  deny { path.match/../ }
  allow { addr.start_with?("192.") }
  use proc {|env| [200, {}, []] }
}

I think this is more intuitive.

kazuho · 2016-08-16T03:42:20Z

lib/handler/mruby.c

+    /* require and include built-in libraries */
+    h2o_mruby_eval_expr(mrb, "require \"h2o.rb\"\n"
+                             "require \"acl.rb\"\n"
+                             "include H2O::ACL\n");


Just one more thing...

How about doing all of the following?

renaming h2o.rb to bootstrap.rb

require and include the acl code from bootstrap.rb

kazuho · 2016-08-16T05:22:18Z

Maybe merge the contents of h2o.rb to bootstrap.rb?

i110 · 2016-08-16T05:26:27Z

@kazuho I created new bootstrap.rb file and require h2o.rb in it instead of renaming, because I feel this is more clear about the initialization process.

kazuho · 2016-08-17T04:16:21Z

lib/handler/mruby.c

+
+    /* require core modules and include built-in libraries */
+    h2o_mruby_eval_expr(mrb, "require \"preloads.rb\"");
+    h2o_mruby_assert(mrb);


Can we expect adequate information emitted to the log in case either requiring preloads.rb or doing the requires in preloads.rb fails?

If the answer is yes, I think we can merge this PR right away.

kazuho · 2016-08-17T06:26:22Z

LGTM. Thank you for all your efforts.

Will merge once you finish applying clang-format.

i110 · 2016-08-17T07:07:58Z

@kazuho Thank YOU for all your kind comments.
I applied clang-format for c files, and it didn't change anything wihout annoying header ordering.
So, pleaes go on.

Ichito Nagata added 3 commits August 5, 2016 11:40

add acl feature

3346d3c

add perl module installation to .travis.yml for testing mruby handlers

682fe6d

add license text

3be506a

kazuho reviewed Aug 8, 2016
View reviewed changes

Ichito Nagata added 2 commits August 8, 2016 14:06

refine mruby testing setup

4899a23

check mruby configration in ruby script

9f415c1

i110 reviewed Aug 8, 2016
View reviewed changes

remove unnecessary file

7986111

kazuho reviewed Aug 12, 2016
View reviewed changes

Ichito Nagata added 2 commits August 15, 2016 11:49

extract https://github.com/iij/mruby-mtest @ d9f257a00427cac1a96df5a0…

e3c4bac

…b8b3bb0dcf2a0966 () at misc/mruby-mtest

make acl directives not to return value

a36e3de

kazuho reviewed Aug 16, 2016
View reviewed changes

create bootstrap.rb

398e736

rename bootstrap.rb -> preloads.rb and h2o.rb -> bootstrap.rb

86cf44c

kazuho reviewed Aug 17, 2016
View reviewed changes

output adequate error message while loading preloads.rb

1a041fe

kazuho merged commit 219ccfa into h2o:master Aug 17, 2016

i110 mentioned this pull request Aug 22, 2016

add acl document #1034

Merged

add acl feature #1016

add acl feature #1016

Uh oh!

Conversation

i110 commented Aug 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kazuho Aug 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

i110 Aug 8, 2016

Choose a reason for hiding this comment

Uh oh!

kazuho commented Aug 8, 2016

Uh oh!

i110 Aug 8, 2016

Choose a reason for hiding this comment

Uh oh!

i110 commented Aug 8, 2016

Uh oh!

kazuho Aug 12, 2016

Choose a reason for hiding this comment

Uh oh!

kazuho commented Aug 12, 2016

Uh oh!

i110 commented Aug 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kazuho Aug 16, 2016

Choose a reason for hiding this comment

Uh oh!

kazuho commented Aug 16, 2016

Uh oh!

i110 commented Aug 16, 2016

Uh oh!

kazuho Aug 17, 2016

Choose a reason for hiding this comment

Uh oh!

kazuho commented Aug 17, 2016

Uh oh!

i110 commented Aug 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

i110 commented Aug 5, 2016 •

edited

Loading

kazuho Aug 8, 2016 •

edited

Loading

i110 commented Aug 15, 2016 •

edited

Loading