Repository navigation
Conversation
ca_fingerprint was checked on a separate, unverified socket and the
connection was accepted when any certificate in the presented chain
matched. A server could append the genuine public CA certificate to a
leaf it signed itself and pass. The check also never covered the
connections used for requests, and the docs told users to pair it with
`ssl: { verify: false }`.
Fetch the chain once before the first request, take the certificate
matching the fingerprint, and rebuild the transport with it as the
only trust anchor: an X509::Store for Faraday plus a PEM file for
adapters that only take a CA file (Curb, Manticore, libcurl-based
Faraday adapters). TLS verification stays on, so chain, expiry and
host name are checked on every connection. Users who want to skip
those checks pass `ssl: { verify: false }` explicitly, now also
honoured by Curb.
Refs elastic/security#6842
Assisted-by: Pi Coding Agent (claude-opus-5-5)
artem-shelkovnikov
left a comment
There was a problem hiding this comment.
I left a couple comments, but I wanted to also check the design intent.
My main question is the use-case of ca_fingerprint - my understanding it's only for self-signed certs on dev Elasticsearch clusters, so no real usage in production?
| set_meta_header # from include MetaHeader | ||
| @transport_class.new(hosts: @hosts, options: @arguments) do |faraday| | ||
| faraday.adapter(@arguments[:adapter]) | ||
| @block&.call faraday |
There was a problem hiding this comment.
This was called once before, and now will be called twice per client.
In practice it will have negative effect if somebody who initialises the client does code with side-effects in their &block - I looked around GH a little and did not find anyone doing this, but good to consider?
| store.flags = OpenSSL::X509::V_FLAG_PARTIAL_CHAIN # the pinned certificate may be an intermediate | ||
| store.add_cert(ca) | ||
| # Curb, Manticore and libcurl-based Faraday adapters only accept a CA file | ||
| ca_file = Tempfile.new(['ca_fingerprint', '.pem']) |
There was a problem hiding this comment.
This has potential for lingering temp files if process is killed rather than exits and GCs everything correctly. Again, should not be a big issues cause these are public certs and are small. Plus I'm really not sure if there's a way around this since actual HTTP clients need a file, not in-memory structure.
| transport.connections.connections.each do |connection| | ||
| unless connection.host[:scheme] == 'https' | ||
| raise Elastic::Transport::Transport::Error, 'CA fingerprinting can\'t be configured over http' | ||
| def build_transport |
There was a problem hiding this comment.
Side-effect potential: if somebody modifies transport options after creating a client, the changes will be reset, for instance:
c.transport.logger = my_logger;
c.transport.reload_connections = 5; # default is 10000
c.transport.resurrect_after = 7; # default is 60
c.perform_request('GET', '/')This repo, for instance, will get a regression: https://github.com/toptal/chewy/blob/master/lib/chewy/config.rb#L72-L80 (logger being reset)
Again, not sure how serious that is, but IMO we should not cause this problem.
There was a problem hiding this comment.
Yep, swapping out client.transport is a problem. Anything set on the transport after Client.new gets dropped, and any code holding a reference to the old transport keeps using unpinned connections, and we don't want that chewy regression.
Proposal: keep pinning lazy (on the first request, as it is now), but update the existing transport instead of building a new one:
- Write the pinned
cert_store/ca_fileinto the options. The transport already shares that hash with the client, so it sees the change. - Replace the transport's connection collection with fresh connections.
The transport object stays the same, so user changes survive and references stay valid.
Moving pinning into Client.new would put a network call in the constructor and make it fail if the cluster is unreachable, which goes against the goal of keeping ca_fingerprint easy to set up.
Two catches:
__rebuild_connectionswon't work as-is. It keeps any existing connection that's==to a new one, andConnection#==only compares protocol, host, port and attributes. The old connections would survive without any error. The collection needs to be fully replaced, with an assurance that requests go through the pinned connections.- Manticore builds its client once in
initialize, so it also needsbuild_clientcalled again.
This also takes care of the block concern in the other thread. Rebuilding connections still runs the Faraday block once per host, but that's already what happens today on every sniff or reload_on_failure, so it's not a huge change.
| ca_file.write(ca.to_pem) | ||
| ca_file.flush | ||
| @arguments[:transport_options][:ssl] = | ||
| (@arguments[:transport_options][:ssl] || {}).merge(cert_store: store, ca_file: ca_file.path) |
There was a problem hiding this comment.
Looks like we're silently overriding cert_store here if the user provided one, we should not.
There was a problem hiding this comment.
Interesting side-effect here is that there's ca_path that can be passed here, and will result in trusting both places: ca_file AND ca_path at the same time. This might be a problem IMO.
There was a problem hiding this comment.
Both points are valid.
The security team's guidance is that the certificate matched by ca_fingerprint should be the only certificate the client trusts.
ca_pathpoints at a folder of trusted certificates. Right now we leave it in place, so the client trusts our pinned cert plus everything in that folder. If that folder is the system cert store (e.g./etc/ssl/certs), the client trusts any public certificate authority, and the pin does almost nothing. Let's fix this before merge.cert_store+ca_fileare other ways of trusting the certs they point to. We replace them with the pinned cert, which is correct for a pin, but we do it without telling the user. Someone who set both might assume their own trust settings still apply.
What if, when ca_fingerprint is set together with any of cert_store, ca_file or ca_path, raise an ArgumentError in Client.new explaining that ca_fingerprint can't be combined with other trust settings? This matches what we already do for ca_fingerprint + a custom transport.
This will be a breaking change for anyone combining these today. The PR already changes how verify: false behaves and removes Connection#verified, so I'd rather fail loudly in the same release than quietly ignore or override settings. We're comfortable introducing breaking changes between minors when absolutely necessary; since minor versions follow the same versioning schema as the stack, we can't just bump the major, and security concerns are more important than strictly following semver. Also, overriding and logging a warning would be too easy to miss, and the pin would not pin.
Unfortunately, no. There's a comment on the internal security discussion (so can't link here) that points out that providing a fingerprint is a common solution when running ES inside a container, where mounting an enterprise cert might be a heavier lift. |
Summary
Makes
ca_fingerprintsecure the connections that actually carry requests. Before the first request, the client fetches the server's certificate chain once, takes the certificate matching the fingerprint, and rebuilds the transport with that certificate as the only trust anchor. TLS verification stays on, so OpenSSL checks the chain, expiry and host name on every data connection. Verification is never turned off by the client. Users who want to skip those checks passssl: { verify: false }explicitly, following the JS client precedent.Intention
The old check opened a separate probe socket with an unverified context, accepted the connection if any certificate in the presented chain matched, and then set
connection.verified = true. A server could append the genuine public CA certificate to a leaf it signed itself and pass. The probe also never covered the connections used for requests, and the docs told users to combineca_fingerprintwithssl: { verify: false }, which left the data connections with no verification at all.Changes
Expand to see the list of changes
Client#pin_ca_certificatereplacesvalidate_ca_fingerprints. It still raisesElastic::Transport::Transport::Errorwith the existing messages when a host isn't https or no certificate matches. The pin is anOpenSSL::X509::Storeholding only the matching certificate (withV_FLAG_PARTIAL_CHAIN, so an intermediate can be pinned) passed astransport_options[:ssl][:cert_store], plus a PEM tempfile passed asca_filefor adapters that can't take a store (Curb, Manticore through top-level:ssl, and libcurl-based Faraday adapters). Faraday needs the store as well: with onlyca_file, its Net::HTTP adapter adds the file to the system default store.Client#initializemoved intobuild_transportso the pinned transport can be rebuilt with the same options and block. Sniffed and reloaded connections inherit the pin from the options.ca_fileandverify: falsefromtransport_options[:ssl], sinceClient.newgives Curb users no other way to set either.ca_fingerprintcombined with a custom:transportinstance raisesArgumentError, because the pin can't be applied to a transport the client didn't build.Connection#verified, fixes the@option ca_fingerprintdoc comment, and adds a CHANGELOG entry.Related
Notes
Important
Users following the current docs pass
ssl: { verify: false }withca_fingerprint. After this change that combination disables all verification, including the pin, so elastic/elasticsearch-ruby#3079 should ship with the release.Connection#verifiedis removed from the public API.Expand to see the notes
Test changes
CA Fingerprinting when setting a ca_fingerprint validates CA fingerprints on perform request. It stubbedTCPSocketandSSLSocketand assertedConnection#verified, which tested the probe approach this PR removes. It also wrotecertificate.crtandcertificate.keyinto the working directory.net_http) and Curb: matching fingerprint works, leaf with the genuine CA appended is rejected, expired certificate is rejected, host name mismatch is rejected, explicitverify: falseis honoured, and a wrong fingerprint raises. A Faraday-only spec checks that the data connection'sssloptions carry the pinned store and CA file.ArgumentErrorwith a custom transport instance.when using an http hostandwhen not setting a ca_fingerprintspecs are unchanged.Test results
Ruby 3.3, no Elasticsearch running:
rspecgives 677 examples, 24 failures with Faraday 2 and 675 examples, 24 failures with Faraday 1 (Gemfile-faraday1.gemfile). The 24 failures are the same list as onmain(23 need a live cluster, plusbase_spec.rb:138).rake test:unit: 115 tests, 0 failures. The new specs fail onmainfor the matching, expired, host name,verify: falseand store cases.Risks and open questions
ssl: { ca_file: }as the only trust source.verify: false(existing adapter behaviour, needsverify_hostname: false).client.transport. Changes made directly to connection objects before the first request are lost, and references to the old transport stay unpinned. Doing it in the constructor would avoid that, but would add network I/O toClient.new.authored by Pi Coding Agent (claude-opus-5-5)