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

feat: Handle x509 certs in HTTP Client #142

Conversation

gmolki
Copy link
Contributor

@gmolki gmolki commented Sep 13, 2023

Goal of this PR is to enable the usage of x509 certificates in http requests.

Related PR in pact-support: pact-foundation/pact-support#99

@gmolki gmolki changed the title feat: handle-x509-certs feat: Handle x509 certs in HTTP Client Sep 13, 2023
@mefellows
Copy link
Member

mefellows commented Sep 13, 2023

Thanks for this! I think env vars are a nice touch, but I wonder if having CLI options for these would be a preferred starting point for consistency with other flags?

In any case, it's a niche case, so if this supports your need then that shouldn't stop this from going through, assuming we're happy with the env var naming and approach.

In terms of consistency with env var naming, the convention is PACT_<THE_ENV_VAR>.

So X509_CERT_ENABLED would be PACT_X509_CERT_ENABLED and so on.

@bethesque bethesque self-requested a review September 13, 2023 23:35
@@ -127,6 +133,18 @@ def verbose?
verbose || ENV["VERBOSE"] == "true"
end

def custom_x509_certificate?
Copy link
Member

Choose a reason for hiding this comment

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

Is this useful? Can we just use the presence of the env vars to work out whether to set the cert/key or not, the same way we do for the SSL cert env vars?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, good idea!

I have renamed the method and handled the presence validation for the env vars in the method.
I think it's better to have a single method with this logic rather than have it in two lines repeated.
Although, if you prefer to remove this method I can move the logic to each line 😄

end

def x509_cert_file
File.read(ENV['X509_CERT_FILE_PATH'])
Copy link
Member

Choose a reason for hiding this comment

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

As Matt said, we do usually prefix all env vars with PACT_. We use SSL_CERT_FILE and SSL_CERT_DIR because they are pre-existing Unix standards, however.

There's no such standard for x509 certificates that I can find though, so we need to decide whether to make it match the SSL cert env vars, or the rest of the PACT env vars. The SSL env vars and the x509 env vars are likely to be used together, aren't they, given that this is about doing mutual certificate authentication. Given this is the client cert (not the server cert) it might make sense to include that in the name.
I think the most consistent approach would be to call them X509_CLIENT_CERT_FILE and X509_CLIENT_KEY_FILE.

We don't have command line arguments for the SSL certificates, so I don't think we need to add them for the x509 certs.

Copy link
Member

Choose a reason for hiding this comment

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

These are client certificates, that's correct. I agree with including that in the name for clarity.

I don't see anywhere in this code that requires the client to confirm the certificate (authenticity) of the server - so perhaps that's forthcoming, or not required for the use case.

i.e. as it stands, a malicious server could present a valid certificate and if it's in the CLI's CA then there are no client-side checks to ensure the requests are going to the right place.

@bethesque
Copy link
Member

Testing is going to be a bit fiddly, but we can copy from some prior art

Here's a test that executes a request to a webrick server set up with SSL

https://github.com/pact-foundation/pact_broker/blob/1bb6088da7790c20405f0774ae497b94d6915774/spec/integration/webhooks/certificate_spec.rb#L16

This is the file it uses to run the server:

https://github.com/pact-foundation/pact_broker/blob/master/spec/support/ssl_webhook_server.rb

Looks like there is a verify client option :SSLVerifyClient => OpenSSL::SSL::VERIFY_FAIL_IF_NO_PEER_CERT | OpenSSL::SSL::VERIFY_PEER, referenced in this stack overflow issue https://stackoverflow.com/questions/54009531/ruby-webrick-server-not-able-to-verify-client-certificate

I'm guessing we would need to add SSLClientCA and SSLVerifyClient and maybe SSLVerifyDepth to the ssl_webhook_server.rb example to set it up for this scenario.

@YOU54F
Copy link
Member

YOU54F commented Sep 19, 2023

Just kicked off CI, thanks for the updates post review Gerard

@gmolki
Copy link
Contributor Author

gmolki commented Sep 19, 2023

Just kicked off CI, thanks for the updates post review Gerard

Thanks @YOU54F.
I just pushed a new commit fixing some failing tests due to different OpenSSL versions handling errors differently.

@YOU54F
Copy link
Member

YOU54F commented Sep 19, 2023

ty, fyi, you can ignore the failing pact and can-i-deploy test, they are failing as due to it being a PR from a outside collab, you can't retrieve secrets, in order to login to the pact broker for the tests, The other tests should be a healthy indicator though 👍🏾

before(:all) do
@pipe = IO.popen("bundle exec ruby ./spec/support/ssl_server.rb")
ENV['SSL_CERT_FILE'] = "./spec/fixtures/certificates/ca_cert.pem"
ENV['SSL_CERT_DIR'] = "./spec/fixtures/certificates/"
Copy link
Member

Choose a reason for hiding this comment

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

If SSL_CERT_FILE is set, is SSL_CERT_DIR necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!
I thought both were required, but seems like with the SSL_CERT_FILE is enough.

it "fails raising SSL error" do
expect { do_get }
.to raise_error { |error|
expect([OpenSSL::SSL::SSLError, Errno::ECONNRESET]).to include(error.class)
Copy link
Member

Choose a reason for hiding this comment

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

That's curious. It could raise either error? Can you tell me more about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like so.
I ran the server in debug mode to check why this happening, and I got the following output by running it in:

ubuntu-latest ruby v3.1
  [2023-09-20 08:52:01] INFO  WEBrick::HTTPServer#start: pid=508 port=4444
  |     with valid x509 client certificates
  |       succeeds
  |     when invalid x509 certificates are set
  | [2023-09-20 08:52:01] ERROR OpenSSL::SSL::SSLError: SSL_accept returned=1 errno=0 peeraddr=127.0.0.1:49104 state=error: certificate verify failed (self-signed certificate)
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `accept'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `block (2 levels) in start_thread'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/utils.rb:258:in `timeout'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:300:in `block in start_thread'
  | [2023-09-20 08:52:01] ERROR OpenSSL::SSL::SSLError: SSL_accept returned=1 errno=0 peeraddr=127.0.0.1:49106 state=error: certificate verify failed (self-signed certificate)
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `accept'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `block (2 levels) in start_thread'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/utils.rb:258:in `timeout'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:300:in `block in start_thread'
  | ERROR: Error making request - Errno::ECONNRESET Connection reset by peer /Users/jobandtalent/jobandtalent/pact_broker-client/lib/pact_broker/client/hal/http_client.rb:87:in `block (2 levels) in perform_request', attempt 1 of 5
  | [2023-09-20 08:52:06] ERROR OpenSSL::SSL::SSLError: SSL_accept returned=1 errno=0 peeraddr=127.0.0.1:49112 state=error: certificate verify failed (self-signed certificate)
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `accept'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `block (2 levels) in start_thread'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/utils.rb:258:in `timeout'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:300:in `block in start_thread'
  | [2023-09-20 08:52:06] ERROR OpenSSL::SSL::SSLError: SSL_accept returned=1 errno=0 peeraddr=127.0.0.1:49122 state=error: certificate verify failed (self-signed certificate)
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `accept'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `block (2 levels) in start_thread'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/utils.rb:258:in `timeout'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:300:in `block in start_thread'
  | ERROR: Error making request - Errno::ECONNRESET Connection reset by peer /Users/jobandtalent/jobandtalent/pact_broker-client/lib/pact_broker/client/hal/http_client.rb:87:in `block (2 levels) in perform_request', attempt 2 of 5
  | [2023-09-20 08:52:12] ERROR OpenSSL::SSL::SSLError: SSL_accept returned=1 errno=0 peeraddr=127.0.0.1:43724 state=error: certificate verify failed (self-signed certificate)
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `accept'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `block (2 levels) in start_thread'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/utils.rb:258:in `timeout'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:300:in `block in start_thread'
  | [2023-09-20 08:52:12] ERROR OpenSSL::SSL::SSLError: SSL_accept returned=1 errno=0 peeraddr=127.0.0.1:43736 state=error: certificate verify failed (self-signed certificate)
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `accept'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `block (2 levels) in start_thread'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/utils.rb:258:in `timeout'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:300:in `block in start_thread'
  | ERROR: Error making request - Errno::ECONNRESET Connection reset by peer /Users/jobandtalent/jobandtalent/pact_broker-client/lib/pact_broker/client/hal/http_client.rb:87:in `block (2 levels) in perform_request', attempt 3 of 5
  | [2023-09-20 08:52:17] ERROR OpenSSL::SSL::SSLError: SSL_accept returned=1 errno=0 peeraddr=127.0.0.1:43752 state=error: certificate verify failed (self-signed certificate)
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `accept'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `block (2 levels) in start_thread'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/utils.rb:258:in `timeout'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:300:in `block in start_thread'
  | [2023-09-20 08:52:17] ERROR OpenSSL::SSL::SSLError: SSL_accept returned=1 errno=0 peeraddr=127.0.0.1:43764 state=error: certificate verify failed (self-signed certificate)
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `accept'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `block (2 levels) in start_thread'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/utils.rb:258:in `timeout'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:300:in `block in start_thread'
  | ERROR: Error making request - Errno::ECONNRESET Connection reset by peer /Users/jobandtalent/jobandtalent/pact_broker-client/lib/pact_broker/client/hal/http_client.rb:87:in `block (2 levels) in perform_request', attempt 4 of 5
  | [2023-09-20 08:52:22] ERROR OpenSSL::SSL::SSLError: SSL_accept returned=1 errno=0 peeraddr=127.0.0.1:44812 state=error: certificate verify failed (self-signed certificate)
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `accept'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `block (2 levels) in start_thread'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/utils.rb:258:in `timeout'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:300:in `block in start_thread'
  | [2023-09-20 08:52:22] ERROR OpenSSL::SSL::SSLError: SSL_accept returned=1 errno=0 peeraddr=127.0.0.1:44818 state=error: certificate verify failed (self-signed certificate)
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `accept'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `block (2 levels) in start_thread'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/utils.rb:258:in `timeout'
  |       /opt/hostedtoolcache/Ruby/3.1.4/x64/lib/ruby/gems/3.1.0/gems/webrick-1.8.1/lib/webrick/server.rb:300:in `block in start_thread'
  | ERROR: Error making request - Errno::ECONNRESET Connection reset by peer /Users/jobandtalent/jobandtalent/pact_broker-client/lib/pact_broker/client/hal/http_client.rb:87:in `block (2 levels) in perform_request', attempt 5 of 5
  |       fails raising SSL error
ubuntu-latest ruby v2.7
with valid x509 client certificates
|       succeeds
|     when invalid x509 certificates are set
| [2023-09-20 08:59:59] ERROR OpenSSL::SSL::SSLError: SSL_accept returned=1 errno=0 state=error: certificate verify failed (self signed certificate)
|       /opt/hostedtoolcache/Ruby/2.7.8/x64/lib/ruby/gems/2.7.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `accept'
|       /opt/hostedtoolcache/Ruby/2.7.8/x64/lib/ruby/gems/2.7.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `block (2 levels) in start_thread'
|       /opt/hostedtoolcache/Ruby/2.7.8/x64/lib/ruby/gems/2.7.0/gems/webrick-1.8.1/lib/webrick/utils.rb:258:in `timeout'
|       /opt/hostedtoolcache/Ruby/2.7.8/x64/lib/ruby/gems/2.7.0/gems/webrick-1.8.1/lib/webrick/server.rb:300:in `block in start_thread'
| [2023-09-20 08:59:59] ERROR OpenSSL::SSL::SSLError: SSL_accept returned=1 errno=0 state=error: certificate verify failed (self signed certificate)
|       /opt/hostedtoolcache/Ruby/2.7.8/x64/lib/ruby/gems/2.7.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `accept'
|       /opt/hostedtoolcache/Ruby/2.7.8/x64/lib/ruby/gems/2.7.0/gems/webrick-1.8.1/lib/webrick/server.rb:302:in `block (2 levels) in start_thread'
|       /opt/hostedtoolcache/Ruby/2.7.8/x64/lib/ruby/gems/2.7.0/gems/webrick-1.8.1/lib/webrick/utils.rb:258:in `timeout'
|       /opt/hostedtoolcache/Ruby/2.7.8/x64/lib/ruby/gems/2.7.0/gems/webrick-1.8.1/lib/webrick/server.rb:300:in `block in start_thread'
|       fails raising SSL error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like depending on the OS and ruby version it handles the verification error differently

https://github.com/ruby/net-http/blob/beb20c036dce1b14b8e78c012c3537957aaf6590/test/net/http/test_https.rb#L242

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, thanks for digging into that.

@bethesque
Copy link
Member

Can you check for me what the verify_mode is when the disable ssl verification is not set? I can't tell what the default is from the docs at https://docs.ruby-lang.org/en//3.2/Net/HTTP.html

Screenshot 2023-09-20 at 9 13 19 am

@gmolki
Copy link
Contributor Author

gmolki commented Sep 20, 2023

Can you check for me what the verify_mode is when the disable ssl verification is not set? I can't tell what the default is from the docs at https://docs.ruby-lang.org/en//3.2/Net/HTTP.html

Screenshot 2023-09-20 at 9 13 19 am

In case the disable_ssl_verification is not set, if the use_ssl option is set to true then http.start would default the value to OpenSSL::SSL::VERIFY_PEER

Screenshot 2023-09-20 at 09 50 06

net-http docs

Additionally, library code that applies this default can be found here

@bethesque bethesque merged commit c3aa8dc into pact-foundation:master Oct 2, 2023
13 of 15 checks passed
@bethesque
Copy link
Member

Can you add docs under the SSL cert section here https://github.com/pact-foundation/pact-ruby-standalone/blob/master/README.md for the pact-ruby-standalone and under the "Using a custom certificate" section here https://github.com/pact-foundation/pact-ruby-cli/blob/master/README.md#using-a-custom-certificate for the pact-cli

@bethesque
Copy link
Member

bethesque commented Oct 22, 2023

@gmolki the specs for this have suddenly started failing. Can you have a look please? https://github.com/pact-foundation/pact_broker-client/actions/runs/6606534979/job/17942830227#step:5:858

@bethesque
Copy link
Member

I've worked out what it is - it's the new version of rack that was introduced by the pact mock service. Fixed it.

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.

4 participants