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.

Sign up for GitHub

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

Jump to bottom

feat: Handle x509 certs in HTTP Client #142

Merged
merged 6 commits into from
Oct 2, 2023
Merged

feat: Handle x509 certs in HTTP Client #142

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
52deec6
feat: handle-x509-certs
gmolki Sep 13, 2023
5328408
refactor: x509 env vars
gmolki Sep 14, 2023
9797fd9
Add faraday & faraday-retry in test env
gmolki Sep 19, 2023
9626f23
test: x509 certificates
gmolki Sep 19, 2023
7820898
test/fix: cover different openssl error handling
gmolki Sep 19, 2023
ec96c2a
refactor(test): remove SSL_CERT_DIR env setup in test
gmolki Sep 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions lib/pact_broker/client/hal/http_client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,12 @@ def perform_request request, uri
# See https://github.com/pact-foundation/pact-ruby-standalone/issues/57
http.ca_file = ENV['SSL_CERT_FILE'] if ENV['SSL_CERT_FILE'] && ENV['SSL_CERT_FILE'] != ''
http.ca_path = ENV['SSL_CERT_DIR'] if ENV['SSL_CERT_DIR'] && ENV['SSL_CERT_DIR'] != ''

if custom_x509_certificate?
http.cert = OpenSSL::X509::Certificate.new(x509_cert_file)
http.key = OpenSSL::PKey::RSA.new(x509_cert_key_file)
end

if disable_ssl_verification?
if verbose?
$stdout.puts("SSL verification is disabled")
Expand Down Expand Up @@ -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 😄

ENV['X509_CERT_ENABLED'] == 'true'
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.

end

def x509_cert_key_file
File.read(ENV['X509_CERT_KEY_FILE_PATH'])
end

def disable_ssl_verification?
ENV['PACT_DISABLE_SSL_VERIFICATION'] == 'true' || ENV['PACT_BROKER_DISABLE_SSL_VERIFICATION'] == 'true'
end
Expand Down
Loading