WIP: http_filter: REST-based external authentication filter #2621

kflynn · 2018-02-15T16:09:57Z

Description:
This is the external authentication filter that's been powering Ambassador's authentication mechanism for months now. It's used by many - perhaps most - Ambassador installations, including production use at Datawire and elsewhere.

So clearly this PR is a bit overdue, for which I apologize. [ ;) ] That's doubly true in light of Tigera's recent work: Ambassador's auth mechanism is very different from the mechanism that external_authz.proto embodies, and it seems like a discussion is warranted.

Distinctions Between Methods

The big distinctions between Ambassador's auth and the external_authz.proto mechanism are:

The obvious: Ambassador's auth is currently REST-based rather than gRPC-based.

However, we have had requests for gRPC as well as REST; supporting both would probably be a good thing.

When an Ambassador auth service indicates a failure, the response from the auth service is handed back to the client.

"The response" includes HTTP status code, headers, body, the works. This has turned out to be very important for our users, as it provides a lot of flexibility and control.

For example, at Datawire, Ambassador fronts a dashboard service, which is to be accessed by a web browser, and we currently use Auth0 for the heavy lifting of authentication. Giving the auth service control over the response to the client allows us to respond to an unauthenticated request with a 301 to redirect the browser to Auth0's login widget, which makes for a much cleaner user experience... but we can just hand a 401 to a program that would be confused by the redirect.

When the external auth service indicates a success, the filter can copy headers from the auth service response into the client's request before passing it upstream.

Which headers are copied can be configured; only headers specifically permitted are ever copied. This has also turned out to offer a lot of flexibility: for example, the auth service can insert a header indicating what level of access a the authenticated user has, and that header can trigger a routing decision. Or, as we do at Datawire, the extauth service can inject a token into the request after a successful first-time authentication.

Protocol Details

The protocol between the extauth filter and the extauth service is very simple:

Any request that arrives at the filter is sent on to the auth service.
- The HTTP method, URL, and headers other than Content-Length are forwarded unchanged.
- The Content-Length header is given a value of 0.
- No body is sent.
If the auth service responds with an HTTP status of 200, the request is allowed to continue.
- Any allowed_headers listed in the extauth filter config are copied from the auth service's response into the request before it continues upstream.
- Note well that only 200 will approve the request; any other 2yz response is not an approval.
If the auth service responds with an HTTP status other than 200 (including other 2yz responses), the request is not allowed to continue.
- The full response from the auth service - headers, body, everything - is sent as the response to the client.

It's worth noting, given the above, that:

The auth service must be prepared to handle any HTTP method that the client will use.
The auth service can, and is expected to, immediately return a 200 for requests that don't require authentication, but
At present, all requests will be checked with the auth service.

Future Work

Under consideration but not part of this PR:

Performance work. The most obvious thing here is to cache results for a configurable time to cut down on traffic to the auth service; we've prioritized correctness ahead of that, though.
Support for multiple auth services. We've had requests for this, too.

Risk Level: Medium

Testing:
This is WIP because we need to add coverage testing! CI tests should all be passing, though.

Docs Changes: needed but not written until we sort out the path forward [ :) ]
Release Notes: N/A

Signed-off-by: Flynn <flynn@datawire.io>

…fying headers OK to be added. Signed-off-by: Flynn <flynn@datawire.io>

Signed-off-by: Flynn <flynn@datawire.io>

…hat's what. Signed-off-by: Flynn <flynn@datawire.io>

…d not the map. Sigh. Signed-off-by: Flynn <flynn@datawire.io>

Signed-off-by: Flynn <flynn@datawire.io>

…ixes emissary-ingress/emissary#154) Signed-off-by: Flynn <flynn@datawire.io>

SIgned-off-by: Flynn <flynn@datawire.io>

dnoe · 2018-02-15T19:15:46Z

Thanks for getting this PR up for discussion, and the super detailed PR description. Tagging @saumoh here, hopefully we can kick off some discussion on the various authentication filter approaches here.

It looks like some CI tests are failing although I didn't look very deeply at them. If you can investigate those in the meantime that'd be great.

htuch · 2018-02-15T19:36:09Z

@louiscryan @wora @mandarjog @kyessenov @spikecurtis @rshriram Looping in folks who were involved in the earlier ext_auth PRs. I think we should have folks take a pass over the design here and offer up thoughts on how we can address the use case you describe with ideally a single ext auth approach.

spikecurtis · 2018-02-15T20:03:53Z

Thanks @kflynn !

Another way to slice the big distinctions between Ambassador's auth and the external_authz.proto mechanism is that Ambassador's auth is handling both authentication (authn) and authorization (authz), whereas the attempt with external_authz.proto is only to handle the latter.

Many of the use cases you describe that Ambassador's filter supports are around sophisticated authentication flows: redirect unauthenticated users, attach authentication tokens, attach security claims, etc. This stuff is super useful!

Moving forward I think it is important for us to take a modular approach and keep authentication and authorization separate. On the Istio project we are prototyping authentication filters and associated policies---I would love to see those efforts converged with the use cases described here.

kflynn · 2018-02-15T20:54:50Z

@dnoe Yup, looking at CI failures now. I'm actually a bit confused by the Clang build failures -- I ran the tests with --config=clang-asan; should that not have forced a rebuild with Clang?

In any case, I can definitely reproduce the failure here (with bazel build -c dbg --config=clang --verbose_failures //source/exe:envoy-static) and am testing the fix now. Sigh. :)

Signed-off-by: Flynn <flynn@datawire.io>

kflynn · 2018-02-16T06:38:13Z

@spikecurtis Hearing more about what Istio's been exploring sounds great -- having Istio and Ambassador play nicely on this would be a really good thing. If realtime seems easiest there, happy to jump on a call or set up a face visit.

And yes, it was a deliberate decision to allow Ambassador's auth to span both authn and authz -- while separating them certainly has advantages (especially in composability), allowing a single filter to span both avoids a fair amount of complexity, both in the code and in the user experience. It'll be very interesting to see if there's a good way to get the best of both worlds there.

spikecurtis · 2018-02-16T18:29:41Z

@kflynn Some Istio docs about authn (note you might need to join Istio Dev Group to view)

louiscryan · 2018-02-16T19:11:41Z

I tend to agree with Flynn here that there is utility in allowing this functionality to span authn & authz. While we may try to promote as much authn functionality inline into Envoy that will not scale to all use-cases. Similarly the remote authorization mechanism does need to be able to influence the error response to the client in ways that we are unlikely to want to configure into Envoy directly. If this means that 'authorization' is too narrow a name for whats in Envoy today we should rename it

…

On Thu, Feb 15, 2018 at 10:38 PM, Flynn ***@***.***> wrote: @spikecurtis <https://github.com/spikecurtis> Hearing more about what Istio's been exploring sounds great -- having Istio and Ambassador play nicely on this would be a really good thing. If realtime seems easiest there, happy to jump on a call or set up a face visit. And yes, it was a deliberate decision to allow Ambassador's auth to span both authn and authz -- while separating them certainly has advantages (especially in composability), allowing a single filter to span both avoids a fair amount of complexity, both in the code and in the user experience. It'll be very interesting to see if there's a good way to get the best of both worlds there. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2621 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIoKPDmtcHM6jBIUMCUKZOYgayG-Pjf7ks5tVSJagaJpZM4SHFyv> .

jsedgwick

First pass on C++ nits. Won't speak with authority as to the non-language-related content of the diff as I'm new to the repo (this is my first review), but it seems peculiar to have to do an auth RPC on every request.

jsedgwick · 2018-02-16T18:21:11Z

source/common/http/filter/extauth.cc

+namespace Http {
+
+/**
+ * A pass-through filter that talks to an external authn/authz service.


Nit: Copy-pasting these (excellent) comments in header and source is destined for mismatch. Perhaps refer to one from the other?

jsedgwick · 2018-02-16T18:22:49Z

source/common/http/filter/extauth.cc

+ */
+
+static LowerCaseString header_to_add(std::string("x-ambassador-calltype"));
+static LowerCaseString value_to_add(std::string("extauth-request"));


Doesn't trigger here because of the literal, but be careful of most vexing parse. Prefer uniform initialization.
https://en.wikipedia.org/wiki/Most_vexing_parse#Uniform_initialization_syntax
static LowerCaseString header_to_add{std::string{"x-ambassador-calltype"}};
Applies elsewhere in this diff

jsedgwick · 2018-02-16T18:24:26Z

source/common/http/filter/extauth.cc

+  // ...and hang onto a pointer to the original request headers.
+  //
+  // Note that using a reference here won't work. Move semantics are the
+  // root of this issue, I think.


Yes, you generally want to avoid reference members, it's easier to have dangling references when you can't explicitly null/null check them.

Oh, I see reference members are pretty common in envoy. I stand by my comment but convention is convention...

jsedgwick · 2018-02-16T18:24:56Z

source/common/http/filter/extauth.cc

+  // RequestMessageImpl to hold all the details, and start it off as a
+  // copy of the incoming request's headers.
+
+  MessagePtr reqmsg(new RequestMessageImpl(HeaderMapPtr{new HeaderMapImpl(headers)}));


Uniform initialization

jsedgwick · 2018-02-16T18:27:53Z

source/common/http/filter/extauth.h

+/**
+ * ExtAuth filter itself.
+ */
+class ExtAuth : Logger::Loggable<Logger::Id::filter>,


If you do want private inheritance, do so explicitly

jsedgwick · 2018-02-16T18:41:17Z

source/common/http/filter/extauth.cc

+  if (!config_->allowed_headers_.empty()) {
+    // Yup. Let's see if any of them are present.
+
+    for (std::string allowed_header : config_->allowed_headers_) {


const ref? you're copying it into key, this copy is unnecessary

jsedgwick · 2018-02-16T18:41:33Z

source/common/http/filter/extauth.cc

+        if (!value.empty()) {
+          // Not empty! Copy it into our request_headers_.
+
+          std::string valstr(value.c_str());


uniform init

jsedgwick · 2018-02-16T21:27:23Z

source/common/http/filter/extauth.cc

+static LowerCaseString header_to_add(std::string("x-ambassador-calltype"));
+static LowerCaseString value_to_add(std::string("extauth-request"));
+
+ExtAuth::ExtAuth(ExtAuthConfigConstSharedPtr config) : config_(config) {}


std::move the pointer in, especially if this filter is constructed per-request

How about ExtAuth::ExtAuth(const ExtAuthConfigConstSharedPtr& config) : config_(config) {} ?

That still copies the shared pointer. Passing by value and moving avoids the copy in this ctor. The caller can move their copy into the ctor if possible.

jsedgwick · 2018-02-16T21:44:36Z

source/common/http/filter/extauth.cc

+
+  auth_request_ =
+      config_->cm_.httpAsyncClientForCluster(config_->cluster_)
+          .send(std::move(reqmsg), *this, Optional<std::chrono::milliseconds>(config_->timeout_));


Hmm, our Optional should be able to infer the type here. I'll look into either adding that or ideally switching to abseil/folly/whatever

Very good point. It looks like that there is already a TODO for replacing optional. Would be possible to address this in a separated PR?

envoy/include/envoy/common/optional.h

Line 9 in 38d8271

* TODO: Replace with https://github.com/abseil/abseil-cpp/blob/master/absl/types/optional.h

Yeah I'll find/create an issue and get to it eventually..

jsedgwick · 2018-02-16T21:55:32Z

source/common/http/filter/extauth.cc

+  // What did we get back from the auth server?
+
+  uint64_t response_code = Http::Utility::getResponseStatus(response->headers());
+  std::string response_body(response->bodyAsString());


move body? generally fix extra copies of response body/headers in this method as you shouldn't need any

rshriram · 2018-02-17T04:45:50Z

"The response" includes HTTP status code, headers, body, the works. This has turned out to be very important for our users, as it provides a lot of flexibility and control.

This is the part I like. Though I am not sure shipping an entire html body as part of the response is helpful all the time. One option might be to indicate a template file to be used as a body.

I haven’t read the code yet, but for something like this, it might be useful to have a route level opaque metadata that triggers the filter, such that people can enable this for a few URIs (eg /login) out of all the routes. (Might alleviate the need for caching in short term).

kflynn · 2018-02-18T17:02:30Z

@jsedgwick Thanks for checking over the code! More comments inline as we dig into those points.

@louiscryan, @rshriram Yeah -- in practice, the flexibility has proven a huge win, and we haven't seen much evidence that shipping the HTML body for the error case is a big problem yet. Admittedly we tend not to send large HTML bodies: mostly for us it's a 401 with a WWW-Authenticate header, or a 301.

Finally, @spikecurtis, thanks for the pointers! I'm already a member of the istio-dev group, but I need to reread those and think more about abstracting beyond Istio...

wora · 2018-02-18T18:30:37Z

I recommend the filter to be configurable. The set of headers used for auth should be decided by the operators, not blindly forward everything. For common cases, you only need a few headers. Regarding the HTML response, that should be configurable as well. Let's say you are using Facebook or Google for authentication, do you really want to trust them to return HTML to your clients, which could be an IoT device or a financial partner of your business. Everyone wants to have more control over others, it doesn't mean everyone else should trust them. Injecting HTML with JavaScript could be its own disaster in the worst case scenario.

Signed-off-by: Gabriel <gsagula@gmail.com>

gsagula · 2018-02-20T16:56:20Z

@jsedgwick Thank you for the code review. I will be working on this PR from now on. Fixes will be pushed later this evening (PST).

Signed-off-by: Gabriel <gsagula@gmail.com>

gsagula · 2018-02-22T16:22:12Z

source/common/http/filter/extauth.cc


  ENVOY_STREAM_LOG(trace, "ExtAuth Auth responded with code {}", *callbacks_, response_code);

-  if (!response_body.empty()) {
+  if (!response->body()->length()) {


I forgot to roll back this. I will do it on the next pass.

gsagula · 2018-02-22T16:23:31Z

@jsedgwick Feel free to take another look. Thanks!

jsedgwick

LGTM except one thing, code wise. I will try to learn the context and give semantic feedback as well, though it looks like you're getting that from a few angles already :)

jsedgwick · 2018-02-22T18:54:17Z

source/server/config/http/extauth_config.cc

+      json_config.getStringArray("allowed_headers", true), prefix});
+
+  return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void {
+    callbacks.addStreamDecoderFilter(std::make_shared<Http::ExtAuth>(config));


so after the change to ExtAuth ctor, this is where you can std::move config around to have zero-copy behavior

return [config = std::move(config)](Http::FilterChainFactoryCallbacks& callbacks) mutable -> void { callbacks.addStreamDecoderFilter(std::make_shared<Http::ExtAuth>(std::move(config)))); }

Thanks, @jsedgwick. I was a bit confused about where I should do the move, but that makes perfect sense.

htuch · 2018-02-22T19:00:36Z

Met with @kflynn and @dpn today, the plan to move forward with this is to get some agreement at the design level on how this and ext_authz relate. @kflynn volunteered to write up the use cases that inspired this approach to auth filtering in a Google Doc; hopefully the Istio folks can extend and we can agree on the core use cases that need to be addressed. Ideally we can converge solutions and be able to cover all use cases.

wora · 2018-02-22T19:08:31Z

Please share the doc with me when it is ready. I want to make sure the design of the API meets practical requirements. Thanks.

kflynn · 2018-02-22T20:39:46Z

I'll post a link here when the doc ready, yeah.

Also, in case there's any confusion: @gsagula is working with me on code cleanup. :)

mattklein123 · 2018-02-23T01:45:15Z

@louiscryan @wora @mandarjog @kyessenov @spikecurtis @rshriram @gsagula @kflynn @htuch @dnoe @saumoh et al:

My general feeling on this PR and functionality in general is that we should merge into the existing extauth HTTP filter that @saumoh has been working on. This would involve:

Enhancing the existing gRPC API to send/receive the needed data to cover what is done here.
Also enabling the extauth filter to optionally speak REST/JSON instead of binary gRPC, all driven by the same proto. (Basically how we do xDS).

I think this is the best way forward as I really don't want to end up with 2 auth filters and I see no real reason we can't make everyone happy with the right configuration and API.

So along those lines my recommendation is to:

Close this PR for now.
Do the design doc and send to envoy-dev@.
We can flesh out everything there and then make the final call on whether to merge or not.

Anyhow feel free to leave this open if it helps but that's my suggestion on how to proceed FWIW. Thank you very much for working on this @kflynn @gsagula I think this is going to be great for the community.

kflynn · 2018-02-23T20:00:34Z

@mattklein123, thanks for chiming in! Let's leave this open while I get the draft of the design doc done? There are a couple of points I'd like to make sure are clear before we close this one.

For the record, we're definitely not opposed to merging filters. :) We do want to make sure that the folks already using Ambassador's extauth have a smooth path forward, though. Thanks!

gsagula · 2018-02-23T21:27:32Z

@kflynn I'll let you guys decide the design. Meanwhile, If you would like to share the doc with me, it helps in case I need to jump in the code. Thanks!

spikecurtis · 2018-02-23T21:49:52Z

I'm with @mattklein123 and don't see a reason we can't make everyone happy with the right design.

What I care about with the separation between authN and authZ is primarily about being able to compose them.

@kflynn happy to help where I can with the design. Ping me if you want to have any design discussions or get in front of a virtual whiteboard.

louiscryan · 2018-02-24T00:59:24Z

Same here and for @mjog

…

On Fri, Feb 23, 2018 at 1:49 PM, Spike Curtis ***@***.***> wrote: I'm with @mattklein123 <https://github.com/mattklein123> and don't see a reason we can't make everyone happy with the right design. What I care about with the separation between authN and authZ is primarily about being able to compose them. @kflynn <https://github.com/kflynn> happy to help where I can with the design. Ping me if you want to have any design discussions or get in front of a virtual whiteboard. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2621 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIoKPOnDJn-BaXsHE6ALoF5GmO1myN7vks5tXzKHgaJpZM4SHFyv> .

kflynn · 2018-03-05T20:21:22Z

OK, at long last, design doc is here: https://docs.google.com/document/d/1L3aRk9yaT6Lsb4epI6rdjCdStOuwTfhoeSP82zunkGg/edit?usp=sharing

@htuch @dnoe @mattklein123 @louiscryan @spikecurtis @mandarjog @saumoh and anyone else interested... :)

kflynn · 2018-03-05T20:46:39Z

(Looks like the sharing settings weren't correct earlier -- fixed now, I think.)

spikecurtis · 2018-03-05T22:37:22Z

@kflynn Thanks!

I've made a couple comments inline on the doc where differences in design goals jumped out at me.

It feels like the next step is to propose something specific for how the existing ext_authz API might be extended for these use cases (or, conclude that it makes more sense to have an entirely separate API). Again, happy to jump on a call and spitball some ideas.

kflynn · 2018-03-07T15:38:43Z

@spikecurtis How about send me email at flynn@datawire.io and we'll see about getting something scheduled? Thanks!

louiscryan · 2018-03-07T18:28:23Z

I think it makes sense to have a broader discussion. Flynn if you're willing to coordinate I would appreciate it.

…

On Mon, Mar 5, 2018 at 2:37 PM Spike Curtis ***@***.***> wrote: @kflynn <https://github.com/kflynn> Thanks! I've made a couple comments inline on the doc where differences in design goals jumped out at me. It feels like the next step is to propose something specific for how the existing ext_authz API might be extended for these use cases (or, conclude that it makes more sense to have an entirely separate API). Again, happy to jump on a call and spitball some ideas. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2621 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIoKPPy4wV1gVXgK5QPATJKNyGi5dl6mks5tbb4pgaJpZM4SHFyv> .

htuch · 2018-03-07T19:24:44Z

It would be good to provide a synopsis of how this conversation goes at the next Envoy community call (next Tue). Alternatively, we can have the discussion there, but I have a feeling this is > 20 minutes in scope.

kflynn · 2018-03-07T20:12:22Z

I'll coordinate, yeah. @htuch -- definitely agreed! Even if we haven't been able to have the call by Tuesday ( ;) ) I can talk about the situation and where we are.

kflynn · 2018-03-07T20:15:10Z

In other news: we have the design doc open, and it's pretty clear that we have both a desire to work out a way to meet everyone's goals with one mechanism, and an appreciation that we have some work to do to get there. So I'm going to close this PR tomorrow. Further discussion can center around the doc for the moment, and the meeting to come.

If anyone thinks that this'll make it harder for them to be heard, or for us to get to a good outcome, sing out!

…awire/extauth

junr03 · 2018-03-12T16:54:48Z

@kflynn you mentioned closing this PR in your last comment, so I will go ahead and close. If please comment if you would like to re open.

* wasm file gen * build and push * clean up * add check option * update prow script mode * update prow script * update * add a make target * fix * update wasm gen script * generate stats plugin wasm * clean up * fix log

Flynn added 9 commits February 13, 2018 12:01

Build extauth filter as part of Envoy core

0dfa5a9

Signed-off-by: Flynn <flynn@datawire.io>

Support path_prefix for extauth.

845ad8b

Signed-off-by: Flynn <flynn@datawire.io>

Support adding headers from the auth server, with configuration speci…

25fe00a

…fying headers OK to be added. Signed-off-by: Flynn <flynn@datawire.io>

Correctly support invalidating the route cache when adding headers.

e578680

Signed-off-by: Flynn <flynn@datawire.io>

Rename request to reqmsg just for my own sanity in keeping track of w…

1f45772

…hat's what. Signed-off-by: Flynn <flynn@datawire.io>

Invert the allowed-header check so that we're iterating the vector an…

8d95f25

…d not the map. Sigh. Signed-off-by: Flynn <flynn@datawire.io>

Add comments, remove dead code, clean up logging.

891006a

Signed-off-by: Flynn <flynn@datawire.io>

Do not overwrite the Host header when talking to the extauth service (f…

b7ce768

…ixes emissary-ingress/emissary#154) Signed-off-by: Flynn <flynn@datawire.io>

Re-sort for check_format. Sigh.

70e29a5

SIgned-off-by: Flynn <flynn@datawire.io>

Let's see about fixing the clang errors from CI.

0f18d0b

Signed-off-by: Flynn <flynn@datawire.io>

Merge remote-tracking branch 'upstream/master' into datawire/extauth

69d1d97

jsedgwick reviewed Feb 16, 2018

View reviewed changes

filter_auth: initial code review changes

7a0d470

Signed-off-by: Gabriel <gsagula@gmail.com>

Gabriel added 5 commits February 21, 2018 20:44

filter_auth: code review changes

3e020b6

Signed-off-by: Gabriel <gsagula@gmail.com>

filter_auth: changed server config to use std make_shared

44ddcb9

Signed-off-by: Gabriel <gsagula@gmail.com>

filter_auth: fixed sanitizer errors

e332b8a

Signed-off-by: Gabriel <gsagula@gmail.com>

filter_auth: request_headers_ initialization

8585489

Signed-off-by: Gabriel <gsagula@gmail.com>

forcing ci.

f36184e

Signed-off-by: Gabriel <gsagula@gmail.com>

gsagula reviewed Feb 22, 2018

View reviewed changes

jsedgwick reviewed Feb 22, 2018

View reviewed changes

richarddli mentioned this pull request Feb 26, 2018

Create an Envoy PR for the extauth module emissary-ingress/emissary#212

Closed

Merge branch 'datawire/extauth' of github.com:datawire/envoy into dat…

546f6bb

…awire/extauth

junr03 closed this Mar 12, 2018

kflynn mentioned this pull request Mar 15, 2018

Fold Ambassador auth functionality into ext_authz #2828

Closed

8 tasks

WIP: http_filter: REST-based external authentication filter #2621

WIP: http_filter: REST-based external authentication filter #2621

Conversation

kflynn commented Feb 15, 2018

Distinctions Between Methods

Protocol Details

Future Work

dnoe commented Feb 15, 2018

htuch commented Feb 15, 2018

spikecurtis commented Feb 15, 2018

kflynn commented Feb 15, 2018

kflynn commented Feb 16, 2018

spikecurtis commented Feb 16, 2018

louiscryan commented Feb 16, 2018 via email

jsedgwick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsagula Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshriram commented Feb 17, 2018

kflynn commented Feb 18, 2018

wora commented Feb 18, 2018 via email

gsagula commented Feb 20, 2018

gsagula Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

gsagula commented Feb 22, 2018

jsedgwick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Feb 22, 2018

wora commented Feb 22, 2018 via email

kflynn commented Feb 22, 2018

mattklein123 commented Feb 23, 2018 • edited Loading

kflynn commented Feb 23, 2018

gsagula commented Feb 23, 2018

spikecurtis commented Feb 23, 2018

louiscryan commented Feb 24, 2018 via email

kflynn commented Mar 5, 2018

kflynn commented Mar 5, 2018

spikecurtis commented Mar 5, 2018

kflynn commented Mar 7, 2018

louiscryan commented Mar 7, 2018 via email

htuch commented Mar 7, 2018

kflynn commented Mar 7, 2018

kflynn commented Mar 7, 2018

junr03 commented Mar 12, 2018

gsagula Feb 20, 2018 •

edited

Loading

gsagula Feb 22, 2018 •

edited

Loading

mattklein123 commented Feb 23, 2018 •

edited

Loading