Allow OAuth::Consumer to take TLS context

[sempervictus]

Addresses issue #4381

Oauth consumer objects determine their TLS configuration strictly
as a boolean, then pass it to HTTP clients which are capable of
more complex configuration for use cases such as ignoring peer
certificate validation, client certificate configuration, etc.

Remove the strict Bool typecasting for the @tls instance variable
in OAuth::Consumer, replacing it with initializer functionality
which encapsulates the scheme resolution for default value setting
or takes explicit Bool | OpenSSL::SSL::Context::Client parameters.

[RX14]

Please ensure that your changes compile before you send a pull request.

[sempervictus]

@RX14: They do, in the context of a shard anyway, as i exported the OAuth namespace into an OAuthmp namespace in my project, it compiles and runs, though i've had to make other changes to adjust for the slightly non-standard OAuth service in use. Compiling Crystal itself on Arch with grsecurity and a bunch of other changes can get nasty - PAX needs to be off or carefully exempted.
Far as the cause of failure - by the time @scheme is compared to the string "https," it should already be instantiated. If the constructor is unable to handle this, we can monkey patch the PR to move that comparison into the method body, but this approach works in Ruby and other languages, meaning we probably want have the constructor assess all static call params, then proceed to evaluate the dynamic ones after that first pass.

[RX14]

@sempervictus You shouldn't need the compile crystal itself to run the standard library test suite. Just use make std_spec with the latest compiler release and it should be fine.

[RX14]

As for the compile error, it does look weird. It might be a compiler bug. You'll probably have to extract it out into a field in the constructor, like tls : Bool | OpenSSL::SSL::Context::Client | Nil = nil then do @tls = tls.nil? ? @scheme == "https" : tls with a note about why.

[sempervictus]

Ah, cool, thank you for the tip - will take a bit to get spun up with the language specific idiosyncrasies (I miss interactive introspection in a REPL like you wouldn't believe, ICR ain't quite it). Will run in my project to test as you suggested this evening, but if you'd like I can just move the resolution of the ivar into the method body since that seems to be what's causing Travis to fail spec. Also, is the project convention not to use Travis as a testbed for PRs? Lastly, the oauth header generated as a hash with a string embedded seems to be getting rejected by our python2 oauth provider. Works with python oauth client, so I'm thinking something is off in the param builder here, or the python service.

…

-------- Original Message -------- From: Chris Hobbs <notifications@github.com> Sent: Friday, May 5, 2017 11:37 AM To: crystal-lang/crystal <crystal@noreply.github.com> Subject: Re: [crystal-lang/crystal] Allow OAuth::Consumer to take TLS context (#4382) CC: RageLtMan <rageltman@sempervictus.com>,Mention <mention@noreply.github.com>

@sempervictus You shouldn't need the compile crystal itself to run the standard library test suite. Just use `make std_spec` with the latest compiler release and it should be fine. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #4382 (comment)

[RX14]

@sempervictus crystal play is good for an interactive playground, gdb/lldb can replace pry for debugging purposes but they're not quite the same. It likely never will be.

Sure you can use travis but it's easier for most people to run the specs locally if they have crystal already checked out. A lot of people don't bother to check the travis status after they make their PR and need a nudge.

[sempervictus]

@RX14: So i'm thinking this is actually a compiler bug:

Error: instance variable '@scheme' was used before it was initialized in one of the 'initialize' methods, rendering it nilable

where @scheme, only capable of being a String, and having a default value provided, or taking any other potential value passed, would still be a String, is compared to the String "https." In my limited understanding of compiler safety, this should be a "safe" operation since we must be comparing the same data types, even if they're empty.

What i find a bit more troubling, is that by setting up @tls as a union of Nil | Bool | OpenSSL::SSL::Context::Client during initialization of the Consumer as discussed above, we're breaking a later call passing @tls to HTTP::Client.new as seen in:

 make std_spec
Using /usr/bin/llvm-config [version=4.0.0]
./bin/crystal build  -o .build/std_spec spec/std_spec.cr
Using compiled compiler at .build/crystal
Error in spec/std_spec.cr:2: while requiring "./std/**"

require "./std/**"
^

in spec/std/oauth/consumer_spec.cr:47: instantiating 'OAuth::Consumer#get_request_token()'

    consumer.get_request_token(oauth_callback: "foo.bar.baz")
             ^~~~~~~~~~~~~~~~~

in src/oauth/consumer.cr:74: instantiating 'post(Nil, Nil, Hash(String, String), String)'

    post(nil, nil, {"oauth_callback" => oauth_callback}, @request_token_uri) do |response|
    ^~~~

in src/oauth/consumer.cr:141: argument 'tls' already specified

      client = HTTP::Client.new @host, @port, tls: @tls
                            ^~~

make: *** [Makefile:106: .build/std_spec] Error 1

Smells (to me anyway) of the instance variable @tls being a different type of union than the one expected by the HTTP::Client.

The following code compiles and runs the spec just fine:

 class OAuth::Consumer
                  @request_token_uri : String = "/oauth/request_token",
                  @authorize_uri : String = "/oauth/authorize",
                  @access_token_uri : String = "/oauth/access_token",
                 @tls : Bool | OpenSSL::SSL::Context::Client = false )
    @tls = @scheme == "https" unless @tls

though i'm assuming a bit of Ruby-ish inferential behavior in that snippet, of which i'm not yet sure. Specifically the notion that @tls being a Union is "truthy," or will evaluate to True in the case of being passed true or an OpenSSL::SSL::Context::Client object, but not false. The actual commit i just pushed takes the approach of using #is_a?(OpenSSL::SSL::Context::Client), which seems less "elegant" than the snippet above.

[RX14]

Your code doesn't allow passing tls: false when scheme is https. I'm not sure why you'd want to do that but I think it should be possible. Simply don't use the @tls shorthand in the constructor arguments and pass it as a normal argument, and set it's default only if it's nil.

[Sija]

I'd suggest positive check instead in the vein of @scheme == "https" if @tls.is_a?(Bool).

[RX14]

This is still incorrect as per my comment above.

[sempervictus]

@RX14: Check the commit log, i specifically explain that the logic here is not sound because of the Union handling problem when calling HTTP::Client.new using the three member Union of Nil | Bool | OpenSSL::SSL::Context::Client since HTTP::Client's constructor expects a Union with only Bool | OpenSSL::SSL::Context::Client members. I'm pretty sure this is a compiler level error - my original implementation was clean and retained logic consistency for assigning @tls. I will note that today you simply cant set @tls to false, so while the condition we both noticed is problematic for good programmers (this is how bugs and security flaws are born), it doesn't cause a functional regression, and should be fixed by allowing the original design of this PR to work in the initializer's arg processing stack.
@Sija: This is a positive check, for the OpenSSL::SSL::Context::Client member in the Union.
@ALL: So the hack here would be something like

...
    @tls_ctx : Nil | OpenSSL::SSL::Context::Client = nil)
  @tls = @tls_ctx.nil? ? @scheme == "https" : @tlx_ctx
...

Sorry for the eye sore, but given the logic constraint, union type check, and initializer arg misbehavior, this might have to be the "safe until required functionality is in place lower in the stack" approach

[RX14]

@sempervictus The hack you mentioned is much better written as

    tls : Nil | OpenSSL::SSL::Context::Client | Bool = nil)
  @tls = tls.nil? ? @scheme == "https" : tls

and that's what I'd send in as my fix, at least until this compiler bug gets fixed.

[sempervictus]

@RX14: We cant add a nil to the union, its passed to another initializer in http client which can't take the altered passed argument - union type check succeeds, or we could potentially pass a nil to http client which should fault..

[RX14]

@sempervictus You absolutely can if you remove nil from the union before it's set to @tls. Which is exactly what tls.nil? ? @scheme == "https" : tls should do, the type of that statement should be OpenSSL::SSL::Context::Client | Bool, which doesn't change the type of @tls at all.

[sempervictus]

That's a strange nuance, implicitly changing the type of a variable during conditional evaluation. I'll test your version this evening. Thanks for the enlightening insight, will take me a few cycles to get up to speed, but having read through the tracker and stdlib, it looks like there's a good deal of work I can tackle...

[sempervictus]

Just noticed the one character difference... local variables and instance variables can share names. Grand. Still prevents passing a boolean option for the tls setting, which is counter to the normal logic, but probably the lesser of evils available to us.

[RX14]

@sempervictus if you just add Bool to the type of the constructor argument, passing booleans should work...

[sempervictus]

Sorry for the lag, got lost in the churn :(. Implemented as you requested @RX14. Thanks for prodding me through this.
Rebased on master and squashed, tests show:

Pending:
  Math Functions for computing quotient and remainder
  Thread::ConditionVariable signals
  Thread::ConditionVariable broadcasts
  Thread::ConditionVariable waits and send signal

Finished in 21.75 seconds
5081 examples, 0 failures, 0 errors, 4 pending

[mverzilli]

Looks like the build is failing because of formatting. Would run crystal tool format to get that fixed?

[sempervictus]

CI failure on the last commit is unrelated - OSX seems to not have been able to acquire LLVM:

Error: Failed to download resource "llvm"

[sempervictus]

@RX14: seems to be passing CI, any blockers remaining?

[mverzilli]

There are couple of things from my point of view:

1. It itches me quite a bit that up to now the API of the consumer was agnostic from OpenSSL, and now we're tying the implementation to a very specific OpenSSL class. Sounds like we need a little more design to not expose that implementation detail.
2. This PR is adding functionality but not tests. Even if we agree on the design, I'd still expect at least one spec checking the consumer with an OpenSSL::SSL::Context::Client.

[RX14]

@mverzilli We do a similar thing in HTTP::Client so the change to accept a TLS context isn't without precedent. Personally, I think we should design some sort of openssl-agnostic TLS api and only expose that in the stdlib. We then implement a openssl-backed version of that API and extract the old openssl module to a shard which people who need advanced features can use. Then, everywhere we use TLS, we can have a nice crystally options class which people can pass in. Obviously beyond the scope of this PR.

[mverzilli]

Agreed. Still we would need at least one test to exercise the new option (even more important considering that eventually we'll have to break backwards compatibility to "agnosticize" the design).

[RX14]

Windows's built in SSL would probably be a good test of that. Windows support will likely be here in some fashion by the time fiddling with SSL apis is at the top of the todo list.

[sempervictus]

Wholly agreed on abstraction of TLS functions to a shim lib for interface with the multiple back ends available (win, libre, etc).
I'll get a test pushed here in the meantime as doing the middle ware is way out of scope and I'm not yet familiar with the core devs' coding preferences for such bodies of work (hell, still getting some of the rubyisms out of the way).

[bararchy]

@sempervictus are you still working on the specs? ;)
if not, we should either adopt or close this PR for the sake of cleaning.